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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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