git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN

From
SZEDER Gábor <szeder.dev@gmail.com>
Date
Jan 29, 2024, 20:22 UTC
Message-ID
<20240129202201.GA9612@szeder.dev>
In-Reply-To
<20240129031816.GA2433899@coredump.intra.peff.net>
On Sun, Jan 28, 2024 at 10:18:16PM -0500, Jeff King wrote:
Show 69 quoted lines
> 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.

Show 29 quoted lines
> 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
> 
> 
Previous: Jeff KingNext: Junio C Hamano
Message 3 of 24 in “some unit-test Makefile polishing”
  1. 0/2 some unit-test Makefile polishingJeff King, Jan 29, 2024
  2. 1/2 Makefile: use order-only prereq for UNIT_TEST_BINJeff King, Jan 29, 2024
  3. SZEDER GáborJan 29, 2024
  4. Junio C HamanoJan 29, 2024
  5. Jeff KingJan 30, 2024
  6. 2/2 t/Makefile: get UNIT_TESTS list from C sourcesJeff King, Jan 29, 2024
  7. Patrick SteinhardtJan 29, 2024
  8. Jeff KingJan 29, 2024
  9. Adam DinwoodieJan 29, 2024
  10. Junio C HamanoJan 30, 2024
  11. Jeff KingJan 30, 2024
  12. Adam DinwoodieJan 31, 2024
  13. Jeff KingJan 30, 2024
  14. Junio C HamanoJan 29, 2024
  15. 0/3 some unit-test Makefile polishingJeff King, Jan 30, 2024
  16. 1/3 Makefile: use mkdir_p_parent_template for UNIT_TEST_BINJeff King, Jan 30, 2024
  17. 2/3 Makefile: remove UNIT_TEST_BIN directory with "make clean"Jeff King, Jan 30, 2024
  18. 3/3 t/Makefile: get UNIT_TESTS list from C sourcesJeff King, Jan 30, 2024
  19. Junio C HamanoJan 31, 2024
  20. Phillip WoodFeb 1, 2024
  21. Junio C HamanoFeb 2, 2024
  22. Johannes SchindelinFeb 2, 2024
  23. Junio C HamanoFeb 3, 2024
  24. Jeff KingFeb 4, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.