-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Makefile updates #234
Makefile updates #234
Conversation
Name all static analysis make targets check-*.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me despite the non 0 comments. Merge at will!
@@ -1,5 +1,5 @@ | |||
# -*- makefile -*- | |||
|
|||
dir := src/hawkmoth | |||
src_dir := src/hawkmoth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Being overly pedantic here, but this would fit better with the 1st commit. No biggie though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I made this drive-by change, and that inspired me to do the docdir
change.
@@ -15,9 +15,6 @@ endif | |||
# Otherwise, print the full command line. | |||
quiet ?= $($1) | |||
|
|||
%.html: %.rst | |||
rst2html --strict --no-raw $< > $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is being added back in the following commit. I understand the rationale, so if you want this in the history for future reference it's fine with me, just wanted to make sure it wasn't a mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confused. Where is this being added back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man... I think what happened was that I clicked on the referenced commits to have a look and then proceeded to consider one of them part of the PR 🤦♂️
Ignore me -.-
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happens to the best of us! 🤣
Makefile
Outdated
@@ -1,5 +1,7 @@ | |||
# Unconditionally make all targets; they're all phony | |||
MAKEFLAGS += --always-make |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, while I appreciate your reasons for it, I'd rather add PHONY
where it's missing. These things don't even change that often and are not too disruptive if we do find an issue every now and then.
That said, it's ok with me if you really prefer it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's just that we don't really use make to build anything. It's only for running a bunch of recipes, really. So everything is "phony". If I could get away with switching to something like https://github.com/casey/just, which is basically make but without any of the build stuff and all the historical baggage, I'd consider it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative to adding .PHONY: foo
for each target foo
is to make each target depend on a FORCE
target. See https://www.gnu.org/software/make/manual/make.html#Force-Targets
So instead of
.PHONY: clean
clean:
...
We'd have
clean: FORCE
...
That's a bit easier on the eye, perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we could end up creating rules to build packages or to do code generation... Things change. And then we'll need to add these back after someone notices that things are being regenerated when they shouldn't, which is much more puzzling to debug by the way.
always-make
would be the last assumption I would make as to why something was being regenerated when I didn't want it to. Especially as it's so far removed from the rule definition itself. In fact, I'd only use that switch explicitly and for debugging purposes. Maybe. Conversely, something doesn't run, I know where to look.
Anyway, it really is up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I assume you wrote that before my suggestion about FORCE
- compromise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative to adding
.PHONY: foo
for each targetfoo
is to make each target depend on aFORCE
target. See https://www.gnu.org/software/make/manual/make.html#Force-Targets
From the manual: Using ‘.PHONY’ is more explicit and more efficient.
😅
Seriously, if you don't like make, we should be considering meson or something like that. But as long as we're in Rome...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I assume you wrote that before my suggestion about
FORCE
- compromise?
I did. And the following one before I saw this one too. I don't think it's a good compromise by the way. This is the worst option so far: doesn't solve your issue and is equally bad from my perspective.
Edit: ok maybe a bit harsh. Not equally bad as it would at least be local to the rule definition... But you get my point I hope :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, okay! 😄
Dropped this, updated .PHONY deps, and split out the src_dir change to a separate commit.
Rename 'make check' to 'make check-style', and repurpose 'make check' to run all static analysis.
There's ancient support for quiet and verbose builds with 'make V=0' and 'make V=1', but they're only used for the clean and distclean targets. Rather than tediously fix all the other targets to use this machinery, just rip it off. They're not all that verbose anyway.
Makefile.rules has diminished so small there's not much point in keeping it around. Just merge to top level Makefile.local.
Instead of listing all the subdirs manually, find all the Makefile.local files instead.
There are Makefiles in subdirectories to allow running make in the subdirectories. It's kind of handy, so keep it, but simplify the Makefiles. Rename 'all' target to 'default' to better describe its purpose. Turns out src/hawkmoth/Makefile was not updated after the src/ addition. Fix it while at it.
Most of the releasing stuff hasn't been merged anywhere, and it's a bit haphazard. It could be improved. But the fixme comments in the Makefile aren't really helpful either. Remove them.
The old rule dates back to Python 2.7.
Make it similar to test_dir. Remove the unnecessary dir variable while at it.
Make it similar to test_dir and doc_dir. Remove the unnecessary top level dir variable while at it.
Most of our targets are phony. Add the missing .PHONY annotations.
9789e9f
to
e6bdda4
Compare
And merged. |
Long overdue cleanup of some old Makefile cruft.
First lift the two trivial and non-controversial commits from #231, and continue from there.