Makefile updates #234

jnikula · 2024-01-27T12:49:30Z

Long overdue cleanup of some old Makefile cruft.

First lift the two trivial and non-controversial commits from #231, and continue from there.

Name all static analysis make targets check-*.

BrunoMSantos

Looks good to me despite the non 0 comments. Merge at will!

BrunoMSantos · 2024-01-27T14:45:17Z

src/hawkmoth/Makefile.local

@@ -1,5 +1,5 @@
 # -*- makefile -*-

-dir := src/hawkmoth
+src_dir := src/hawkmoth


Being overly pedantic here, but this would fit better with the 1st commit. No biggie though.

I guess I made this drive-by change, and that inspired me to do the docdir change.

BrunoMSantos · 2024-01-27T14:45:33Z

Makefile.rules

@@ -15,9 +15,6 @@ endif
 # Otherwise, print the full command line.
 quiet ?= $($1)

-%.html: %.rst
-	rst2html --strict --no-raw $< > $@


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.

Confused. Where is this being added back?

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 -.-

Happens to the best of us! 🤣

BrunoMSantos · 2024-01-27T14:46:01Z

Makefile

@@ -1,5 +1,7 @@
+# Unconditionally make all targets; they're all phony
+MAKEFLAGS += --always-make


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.

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.

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?

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.

Fair enough. I assume you wrote that before my suggestion about FORCE - compromise?

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

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...

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 :)

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.

The html -> rst rule was added in commit b89cc7d ("doc: don't include README in documentation") with the idea it would be used for checking README.rst. It was superceded by the check added in commit 6d9f0a9 ("build: add check-rst target to lint rst"). Remove the rule.

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.

jnikula · 2024-01-27T18:13:49Z

And merged.

test: rename test-examples to check-examples

2770bb2

Name all static analysis make targets check-*.

jnikula requested a review from BrunoMSantos January 27, 2024 12:49

jnikula mentioned this pull request Jan 27, 2024

Type hints #231

Merged

BrunoMSantos approved these changes Jan 27, 2024

View reviewed changes

jnikula added 11 commits January 27, 2024 18:55

check: rename 'check' to 'check-style' and repurpose 'check'

6715f25

Rename 'make check' to 'make check-style', and repurpose 'make check' to run all static analysis.

build: remove Makefile.rules

3b1b656

Makefile.rules has diminished so small there's not much point in keeping it around. Just merge to top level Makefile.local.

build: include local Makefiles dynamically

07092ed

Instead of listing all the subdirs manually, find all the Makefile.local files instead.

build: remove some stale comments

214f4c7

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.

build: fix cleaning of pycache

fd45724

The old rule dates back to Python 2.7.

build: unify docdir to doc_dir

b4be2a0

Make it similar to test_dir. Remove the unnecessary dir variable while at it.

build: use src_dir instead of dir

b840bb4

Make it similar to test_dir and doc_dir. Remove the unnecessary top level dir variable while at it.

build: add missing .PHONY deps

e6bdda4

Most of our targets are phony. Add the missing .PHONY annotations.

jnikula force-pushed the makefile-updates branch from 9789e9f to e6bdda4 Compare January 27, 2024 17:13

jnikula merged commit 6ec5fbd into master Jan 27, 2024
5 checks passed

jnikula deleted the makefile-updates branch January 27, 2024 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile updates #234

Makefile updates #234

jnikula commented Jan 27, 2024

BrunoMSantos left a comment

BrunoMSantos Jan 27, 2024

jnikula Jan 27, 2024

BrunoMSantos Jan 27, 2024

jnikula Jan 27, 2024

BrunoMSantos Jan 27, 2024

jnikula Jan 27, 2024

BrunoMSantos Jan 27, 2024

jnikula Jan 27, 2024

jnikula Jan 27, 2024

BrunoMSantos Jan 27, 2024

jnikula Jan 27, 2024

BrunoMSantos Jan 27, 2024

BrunoMSantos Jan 27, 2024 •

edited

Loading

jnikula Jan 27, 2024

jnikula commented Jan 27, 2024

		@@ -1,5 +1,7 @@
		# Unconditionally make all targets; they're all phony
		MAKEFLAGS += --always-make

Makefile updates #234

Makefile updates #234

Conversation

jnikula commented Jan 27, 2024

BrunoMSantos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrunoMSantos Jan 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnikula commented Jan 27, 2024

BrunoMSantos Jan 27, 2024 •

edited

Loading