| Message ID | 20240129031816.GA2433899@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some unit-test Makefile polishing | expand |
On Sun, Jan 28, 2024 at 10:18:16PM -0500, Jeff King wrote: > We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with > "mkdir -p". And so the recipe for UNIT_TEST_PROGS, which put their > output in that directory, depend on UNIT_TEST_BIN to make sure it's > there. > > But using a normal dependency leads to weird outcomes, because the > timestamp of the directory is important. For example, try this: > > $ make > [...builds everything...] > > [now re-build one unit test] > $ touch t/unit-tests/t-ctype.c > $ make > SUBDIR templates > CC t/unit-tests/t-ctype.o > LINK t/unit-tests/bin/t-ctype > > So far so good. Now running make again should build nothing. But it > doesn't! > > $ make > SUBDIR templates > LINK t/unit-tests/bin/t-basic > LINK t/unit-tests/bin/t-mem-pool > LINK t/unit-tests/bin/t-strbuf > > Er, what? Let's rebuild again: > > $ make > SUBDIR templates > LINK t/unit-tests/bin/t-ctype > > Weird. And now we ping-pong back and forth forever: > > $ make > SUBDIR templates > LINK t/unit-tests/bin/t-basic > LINK t/unit-tests/bin/t-mem-pool > LINK t/unit-tests/bin/t-strbuf > $ make > SUBDIR templates > LINK t/unit-tests/bin/t-ctype > > What happens is that writing t/unit-tests/bin/t-ctype updates the mtime > of the directory t/unit-tests/bin. And then on the next invocation of > make, all of those other tests are now older and so get rebuilt. And > back and forth forever. > > We can fix this by using an order-only prereq. This is a GNU-ism that > tells make to only care that the dependency exists at all, and to ignore > its mtime. It was designed for exactly this sort of situation (the > documentation example even uses "mkdir"). > > We already rely on GNU make, so that's not a problem. This particular > feature was added in GNU make 3.80, released in October 2002. This is > obviously quite old by date, but it's also worth thinking about macOS, > as Apple stopped updating packages that switched to GPLv3 tools. In this > their dev tools ship GNU make 3.81, which is recent enough. > > If it is a problem, there are two alternatives: > > - we can just "mkdir -p" in the recipe to build the individual > binaries. This will mean some redundant "mkdir" calls, but only when > actually invoking the compiler. > > - we could stop making the directory on the fly, and just add it with > a .gitignore of "*". This would work fine, but might be awkward when > moving back and forth in history. A third alternative is to use $(call mkdir_p_parent_template) in the recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency and target. It will only run mkdir when needed, and it's a well established pattern in our Makefile, so you won't have to spend a paragraph or two arguing about potential problems with GNU-isms :) On a related note, 'make clean' doesn't remove this 't/unit-tests/bin' directory. > Signed-off-by: Jeff King <peff@peff.net> > --- > I may be overly paranoid about the ".gitignore" strategy. I feel like > I've been bitten by this in the past by things switching from source to > build (I think with git-remote-testgit). But that's an actual built > file. Git would probably be OK with the "bin/" directory coming and > going as a tracked entity, because the index really only cares about > the file "bin/.gitignore". Still, this make fix was easy enough. > > Makefile | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index 1a62e48759..958f4cd0bf 100644 > --- a/Makefile > +++ b/Makefile > @@ -3866,7 +3866,7 @@ fuzz-all: $(FUZZ_PROGRAMS) > $(UNIT_TEST_BIN): > @mkdir -p $(UNIT_TEST_BIN) > > -$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN) > +$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN) > $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ > $(filter %.o,$^) $(filter %.a,$^) $(LIBS) > > -- > 2.43.0.797.g29b680fc68 > >
SZEDER Gábor <szeder.dev@gmail.com> writes: > A third alternative is to use $(call mkdir_p_parent_template) in the > recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency > and target. Yeah, that sounds like a good approach in this case. > On a related note, 'make clean' doesn't remove this 't/unit-tests/bin' > directory. Not a new problem, but I did notice this, too. Thanks. Makefile | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git c/Makefile w/Makefile index 958f4cd0bf..7f035a1c9f 100644 --- c/Makefile +++ w/Makefile @@ -3676,7 +3676,7 @@ cocciclean: $(RM) contrib/coccinelle/*.cocci.patch clean: profile-clean coverage-clean cocciclean - $(RM) -r .build + $(RM) -r .build $(UNIT_TEST_BIN) $(RM) po/git.pot po/git-core.pot $(RM) git.res $(RM) $(OBJECTS) @@ -3863,10 +3863,8 @@ $(FUZZ_PROGRAMS): all fuzz-all: $(FUZZ_PROGRAMS) -$(UNIT_TEST_BIN): - @mkdir -p $(UNIT_TEST_BIN) - -$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN) +$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS + $(call mkdir_p_parent_template) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
On Mon, Jan 29, 2024 at 09:22:01PM +0100, SZEDER Gábor wrote: > > If it is a problem, there are two alternatives: > > > > - we can just "mkdir -p" in the recipe to build the individual > > binaries. This will mean some redundant "mkdir" calls, but only when > > actually invoking the compiler. > > > > - we could stop making the directory on the fly, and just add it with > > a .gitignore of "*". This would work fine, but might be awkward when > > moving back and forth in history. > > A third alternative is to use $(call mkdir_p_parent_template) in the > recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency > and target. It will only run mkdir when needed, and it's a well > established pattern in our Makefile, so you won't have to spend a > paragraph or two arguing about potential problems with GNU-isms :) Thanks, I somehow didn't know about that (and didn't find it when grepping around for similar cases, probably because "mkdir -p" no longer appears in those cases ;) ). I agree it's a better solution here. I'll send a v2 in a moment with that. (Ironically, that template requires "call" which is in make 3.81, but the commit adding it didn't discuss that at all). > On a related note, 'make clean' doesn't remove this 't/unit-tests/bin' > directory. Not the end of the world, as we do clean out the contents, so "ls-files -o" would not mention any leftover cruft. But I agree that we should strive for "make clean" to be the opposite of "make" as much as possible. I'll add in a patch to v2. -Peff
diff --git a/Makefile b/Makefile index 1a62e48759..958f4cd0bf 100644 --- a/Makefile +++ b/Makefile @@ -3866,7 +3866,7 @@ fuzz-all: $(FUZZ_PROGRAMS) $(UNIT_TEST_BIN): @mkdir -p $(UNIT_TEST_BIN) -$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN) +$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN) $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \ $(filter %.o,$^) $(filter %.a,$^) $(LIBS)
We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with "mkdir -p". And so the recipe for UNIT_TEST_PROGS, which put their output in that directory, depend on UNIT_TEST_BIN to make sure it's there. But using a normal dependency leads to weird outcomes, because the timestamp of the directory is important. For example, try this: $ make [...builds everything...] [now re-build one unit test] $ touch t/unit-tests/t-ctype.c $ make SUBDIR templates CC t/unit-tests/t-ctype.o LINK t/unit-tests/bin/t-ctype So far so good. Now running make again should build nothing. But it doesn't! $ make SUBDIR templates LINK t/unit-tests/bin/t-basic LINK t/unit-tests/bin/t-mem-pool LINK t/unit-tests/bin/t-strbuf Er, what? Let's rebuild again: $ make SUBDIR templates LINK t/unit-tests/bin/t-ctype Weird. And now we ping-pong back and forth forever: $ make SUBDIR templates LINK t/unit-tests/bin/t-basic LINK t/unit-tests/bin/t-mem-pool LINK t/unit-tests/bin/t-strbuf $ make SUBDIR templates LINK t/unit-tests/bin/t-ctype What happens is that writing t/unit-tests/bin/t-ctype updates the mtime of the directory t/unit-tests/bin. And then on the next invocation of make, all of those other tests are now older and so get rebuilt. And back and forth forever. We can fix this by using an order-only prereq. This is a GNU-ism that tells make to only care that the dependency exists at all, and to ignore its mtime. It was designed for exactly this sort of situation (the documentation example even uses "mkdir"). We already rely on GNU make, so that's not a problem. This particular feature was added in GNU make 3.80, released in October 2002. This is obviously quite old by date, but it's also worth thinking about macOS, as Apple stopped updating packages that switched to GPLv3 tools. In this their dev tools ship GNU make 3.81, which is recent enough. If it is a problem, there are two alternatives: - we can just "mkdir -p" in the recipe to build the individual binaries. This will mean some redundant "mkdir" calls, but only when actually invoking the compiler. - we could stop making the directory on the fly, and just add it with a .gitignore of "*". This would work fine, but might be awkward when moving back and forth in history. Signed-off-by: Jeff King <peff@peff.net> --- I may be overly paranoid about the ".gitignore" strategy. I feel like I've been bitten by this in the past by things switching from source to build (I think with git-remote-testgit). But that's an actual built file. Git would probably be OK with the "bin/" directory coming and going as a tracked entity, because the index really only cares about the file "bin/.gitignore". Still, this make fix was easy enough. Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)