[PATCH 0/2] some unit-test Makefile polishing

[PATCH 0/2] some unit-test Makefile polishing

[Jeff King]

The patches fixes two small hiccups I found with the unit-tests. Neither
is a show-stopper, but mostly just small quality-of-life fixes.

  [1/2]: Makefile: use order-only prereq for UNIT_TEST_BIN
  [2/2]: t/Makefile: get UNIT_TESTS list from C sources

 Makefile   | 2 +-
 t/Makefile | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

-Peff

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

[Jeff King]

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

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

[PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Jeff King]

We decide on the set of unit tests to run by asking make to expand the
wildcard "t/unit-tests/bin/*". One unfortunate outcome of this is that
we'll run anything in that directory, even if it is leftover cruft from
a previous build. This isn't _quite_ as bad as it sounds, since in
theory the unit test executables are self-contained (so if they passed
before, they'll pass even though they now have nothing to do with the
checked out version of Git). But at the very least it's wasteful, and if
they _do_ fail it can be quite confusing to understand why they are
being run at all.

This wildcarding presumably came from our handling of the regular
shell-script tests, which match "t[0-9][0-9][0-9][0-9]-*.sh".  But the
difference there is that those are actual tracked files. So if you
checkout a different commit, they'll go away. Whereas the contents of
unit-tests/bin are ignored (so not only do they stick around, but you
are not even warned of the stale files via "git status").

This patch fixes the situation by looking for the actual unit-test
source files and then massaging those names into the final executable
names. This has two additional benefits:

  1. It will notice if we failed to build one or more unit-tests for
     some reason (wheras the current code just runs whatever made it to
     the bin/ directory).

  2. The wildcard should avoid other build cruft, like the pdb files we
     worked around in 0df903d402 (unit-tests: do not mistake `.pdb`
     files for being executable, 2023-09-25).

Our new wildcard does make an assumption that unit tests are build from
C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
from the top-level Makefile. But doing so is tricky unless we reorganize
that Makefile to split the source file lists into include-able subfiles.
That might be worth doing in general, but in the meantime, the
assumptions made by the wildcard here seems reasonable.

Signed-off-by: Jeff King <peff@peff.net>
---
I of course hit this when moving between "next" and "master" for an
up-and-coming unit-test file which sometimes failed.

 t/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index b7a6fefe28..c5c6e2ef6b 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual
-- 
2.43.0.797.g29b680fc68

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3289 bytes --]

On Sun, Jan 28, 2024 at 10:19:33PM -0500, Jeff King wrote:
> We decide on the set of unit tests to run by asking make to expand the
> wildcard "t/unit-tests/bin/*". One unfortunate outcome of this is that
> we'll run anything in that directory, even if it is leftover cruft from
> a previous build. This isn't _quite_ as bad as it sounds, since in
> theory the unit test executables are self-contained (so if they passed
> before, they'll pass even though they now have nothing to do with the
> checked out version of Git). But at the very least it's wasteful, and if
> they _do_ fail it can be quite confusing to understand why they are
> being run at all.
> 
> This wildcarding presumably came from our handling of the regular
> shell-script tests, which match "t[0-9][0-9][0-9][0-9]-*.sh".  But the
> difference there is that those are actual tracked files. So if you
> checkout a different commit, they'll go away. Whereas the contents of
> unit-tests/bin are ignored (so not only do they stick around, but you
> are not even warned of the stale files via "git status").
> 
> This patch fixes the situation by looking for the actual unit-test
> source files and then massaging those names into the final executable
> names. This has two additional benefits:
> 
>   1. It will notice if we failed to build one or more unit-tests for
>      some reason (wheras the current code just runs whatever made it to
>      the bin/ directory).
> 
>   2. The wildcard should avoid other build cruft, like the pdb files we
>      worked around in 0df903d402 (unit-tests: do not mistake `.pdb`
>      files for being executable, 2023-09-25).
> 
> Our new wildcard does make an assumption that unit tests are build from
> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
> from the top-level Makefile. But doing so is tricky unless we reorganize
> that Makefile to split the source file lists into include-able subfiles.
> That might be worth doing in general, but in the meantime, the
> assumptions made by the wildcard here seems reasonable.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I of course hit this when moving between "next" and "master" for an
> up-and-coming unit-test file which sometimes failed.
> 
>  t/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/t/Makefile b/t/Makefile
> index b7a6fefe28..c5c6e2ef6b 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
>  TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
>  CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
>  CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))

Wouldn't we have to honor `$X` on Windows systems so that the unit tests
have the expected ".exe" suffix here?

Other than this question the patch series looks good to me, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Jeff King]

On Mon, Jan 29, 2024 at 12:26:42PM +0100, Patrick Steinhardt wrote:

> > -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> > +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> > +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> > +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
> 
> Wouldn't we have to honor `$X` on Windows systems so that the unit tests
> have the expected ".exe" suffix here?

Hmm, good point. It seems like the answer should obviously be "yes", but
Windows CI seemed to pass all the same (and I checked that it indeed ran
the unit tests). Do we only get the $X suffix for MSVC builds or
something? Looks like maybe cygwin, as well.

I imagine the solution is just:

diff --git a/t/Makefile b/t/Makefile
index c5c6e2ef6b..9b9b30f559 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -43,7 +43,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
 UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
-UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
 UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)

but it looks like we might need to include config.mak.uname, as well. It
would be nice to identify a build that actually needs it so I can
confirm that the fix works.

-Peff

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

[SZEDER Gábor]

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

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Adam Dinwoodie]

On Mon, 29 Jan 2024 at 17:49, Jeff King wrote:
>
> On Mon, Jan 29, 2024 at 12:26:42PM +0100, Patrick Steinhardt wrote:
>
> > > -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> > > +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> > > +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> > > +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
> >
> > Wouldn't we have to honor `$X` on Windows systems so that the unit tests
> > have the expected ".exe" suffix here?
>
> Hmm, good point. It seems like the answer should obviously be "yes", but
> Windows CI seemed to pass all the same (and I checked that it indeed ran
> the unit tests). Do we only get the $X suffix for MSVC builds or
> something? Looks like maybe cygwin, as well.

Cygwin will automatically append ".exe" when doing directory listings;
a check if the file "a" exists will return true on Cygwin if "a" or
"a.exe" exists; a glob for "a*" in a directory containing files "a1"
and "a2.exe" will return "a1" and "a2". This causes problems in some
edge cases, but it means *nix scripts and applications are much more
likely to work without any Cygwin-specific handling. I *think* this
logic is carried downstream to MSYS2 and thence to Git for Windows.

As a result, I'm not surprised this worked without handling $X, but I
don't think there's any harm in adding it either.

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Our new wildcard does make an assumption that unit tests are build from

"build" -> "built", probably.

> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
> from the top-level Makefile. But doing so is tricky unless we reorganize
> that Makefile to split the source file lists into include-able subfiles.
> That might be worth doing in general, but in the meantime, the
> assumptions made by the wildcard here seems reasonable.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I of course hit this when moving between "next" and "master" for an
> up-and-coming unit-test file which sometimes failed.

Thanks.  globbing the build products is indeed sloppy for all the
reasons you mentioned.  Will queue.

>  t/Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index b7a6fefe28..c5c6e2ef6b 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
>  TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
>  CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
>  CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
> -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
> +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
> +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))
> +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
>  
>  # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
>  # checks all tests in all scripts via a single invocation, so tell individual

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

[Junio C Hamano]

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)
 

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

>> Hmm, good point. It seems like the answer should obviously be "yes", but
>> Windows CI seemed to pass all the same (and I checked that it indeed ran
>> the unit tests). Do we only get the $X suffix for MSVC builds or
>> something? Looks like maybe cygwin, as well.
>
> Cygwin will automatically append ".exe" when doing directory listings;
> a check if the file "a" exists will return true on Cygwin if "a" or
> "a.exe" exists; a glob for "a*" in a directory containing files "a1"
> and "a2.exe" will return "a1" and "a2". This causes problems in some
> edge cases, but it means *nix scripts and applications are much more
> likely to work without any Cygwin-specific handling. I *think* this
> logic is carried downstream to MSYS2 and thence to Git for Windows.

Interesting, especially that "a*" is globbed to "a2" and not
"a2.exe".

> As a result, I'm not surprised this worked without handling $X, but I
> don't think there's any harm in adding it either.

OK.

I wonder if something like this is sufficient?  I am not sure if we
should lift the building of t/unit-tests/* up to the primary Makefile
to mimic the way stuff related to test-tool are built and linked.
That way, we do not have to contaminate t/Makefile with compilation
related stuff that we didn't need originally.

 t/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git c/t/Makefile w/t/Makefile
index b7a6fefe28..010ce083b1 100644
--- c/t/Makefile
+++ w/t/Makefile
@@ -6,6 +6,7 @@ include ../shared.mak
 # Copyright (c) 2005 Junio C Hamano
 #
 
+include ../config.mak.uname
 -include ../config.mak.autogen
 -include ../config.mak
 
@@ -42,7 +43,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$X,$(UNIT_TEST_SOURCES))
+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual

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

[Jeff King]

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

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Jeff King]

On Mon, Jan 29, 2024 at 09:31:11PM +0000, Adam Dinwoodie wrote:

> > Hmm, good point. It seems like the answer should obviously be "yes", but
> > Windows CI seemed to pass all the same (and I checked that it indeed ran
> > the unit tests). Do we only get the $X suffix for MSVC builds or
> > something? Looks like maybe cygwin, as well.
> 
> Cygwin will automatically append ".exe" when doing directory listings;
> a check if the file "a" exists will return true on Cygwin if "a" or
> "a.exe" exists; a glob for "a*" in a directory containing files "a1"
> and "a2.exe" will return "a1" and "a2". This causes problems in some
> edge cases, but it means *nix scripts and applications are much more
> likely to work without any Cygwin-specific handling. I *think* this
> logic is carried downstream to MSYS2 and thence to Git for Windows.
> 
> As a result, I'm not surprised this worked without handling $X, but I
> don't think there's any harm in adding it either.

Ah, that makes sense. I do think it will work under the conditions you
laid out, but I don't know if there are platforms where that's not the
case. I'll add the $(X) just to be sure (as that really feels more
consistent with what the top-level Makefile is doing anyway).

Thanks.

-Peff

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Jeff King]

On Mon, Jan 29, 2024 at 04:27:59PM -0800, Junio C Hamano wrote:

> > As a result, I'm not surprised this worked without handling $X, but I
> > don't think there's any harm in adding it either.
> 
> OK.
> 
> I wonder if something like this is sufficient?
> [...]

Yeah, that matches the patch I came up with locally. I'll roll that into
v2.

> I am not sure if we
> should lift the building of t/unit-tests/* up to the primary Makefile
> to mimic the way stuff related to test-tool are built and linked.
> That way, we do not have to contaminate t/Makefile with compilation
> related stuff that we didn't need originally.

I didn't quite understand this comment, though. The building of
t/unit-tests _is_ in the top-level Makefile. It's just here in the
recursive Makefile that we need to have the list of built programs.

-Peff

[PATCH v2 0/3] some unit-test Makefile polishing

[Jeff King]

On Sun, Jan 28, 2024 at 10:15:40PM -0500, Jeff King wrote:

> The patches fixes two small hiccups I found with the unit-tests. Neither
> is a show-stopper, but mostly just small quality-of-life fixes.

And here's another iteration based on the feedback from v1. It uses the
mkdir_p template mentioned by Gábor, fixes the $(X) issue mentioned by
Patrick, and adds a new patch to handle the directory in "make clean".

No range diff, as range-diff refuses to admit that the patches are
related (presumably because even though the changes are small, the
original patches were also tiny).

  [1/3]: Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN
  [2/3]: Makefile: remove UNIT_TEST_BIN directory with "make clean"
  [3/3]: t/Makefile: get UNIT_TESTS list from C sources

 Makefile   | 10 ++++------
 t/Makefile |  5 ++++-
 2 files changed, 8 insertions(+), 7 deletions(-)

-Peff

[PATCH v2 1/3] Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN

[Jeff King]

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 making the directory as part of the build recipe for
the programs, using the template from 0b6d0bc924 (Makefiles: add and use
wildcard "mkdir -p" template, 2022-03-03).

Helped-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 Makefile | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 0f748a52e6..228f34a3fe 100644
--- a/Makefile
+++ b/Makefile
@@ -3868,10 +3868,8 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS
 
 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)
 
-- 
2.43.0.797.g29b680fc68

[PATCH v2 2/3] Makefile: remove UNIT_TEST_BIN directory with "make clean"

[Jeff King]

We remove $(UNIT_TEST_PROGS), but that leaves the automatically
generated "bin" dir they reside in. And once we start cleaning that,
there is no point in removing the individual programs, as they'll by
wiped out by the recurse "rm".

Signed-off-by: Jeff King <peff@peff.net>
---
The layout of the clean rule is kind of weird, in that we put only
sort-of related things together on their own $(RM) line. I suspect a lot
more of this could be lumped together, reducing the total number of
shells and rm invocations needed, but I guess nobody really cares that
much about the runtime cost of "make clean".

 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 228f34a3fe..b5ae34ea7b 100644
--- a/Makefile
+++ b/Makefile
@@ -3680,14 +3680,14 @@ 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)
 	$(RM) headless-git.o
 	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
-	$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
+	$(RM) $(TEST_PROGRAMS)
 	$(RM) $(FUZZ_PROGRAMS)
 	$(RM) $(SP_OBJ)
 	$(RM) $(HCC)
-- 
2.43.0.797.g29b680fc68

[PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources

[Jeff King]

We decide on the set of unit tests to run by asking make to expand the
wildcard "t/unit-tests/bin/*". One unfortunate outcome of this is that
we'll run anything in that directory, even if it is leftover cruft from
a previous build. This isn't _quite_ as bad as it sounds, since in
theory the unit tests executables are self-contained (so if they passed
before, they'll pass even though they now have nothing to do with the
checked out version of Git). But at the very least it's wasteful, and if
they _do_ fail it can be quite confusing to understand why they are
being run at all.

This wildcarding presumably came from our handling of the regular
shell-script tests, which use $(wildcard t[0-9][0-9][0-9][0-9]-*.sh).
But the difference there is that those are actual tracked files. So if
you checkout a different commit, they'll go away. Whereas the contents
of unit-tests/bin are ignored (so not only do they stick around, but you
are not even warned of the stale files via "git status").

This patch fixes the situation by looking for the actual unit-test
source files and then massaging those names into the final executable
names. This has two additional benefits:

  1. It will notice if we failed to build one or more unit-tests for
     some reason (wheras the current code just runs whatever made it to
     the bin/ directory).

  2. The wildcard should avoid other build cruft, like the pdb files we
     worked around in 0df903d402 (unit-tests: do not mistake `.pdb`
     files for being executable, 2023-09-25).

Our new wildcard does make an assumption that unit tests are built from
C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
from the top-level Makefile. But doing so is tricky unless we reorganize
that Makefile to split the source file lists into include-able subfiles.
That might be worth doing in general, but in the meantime, the
assumptions made by the wildcard here seems reasonable.

Note that we do need to include config.mak.uname either way, though, as
we need the value of $(X) to compute the correct executable names (which
would be true even if we had acess to the top-level's UNIT_TEST_PROGRAMS
variable).

Signed-off-by: Jeff King <peff@peff.net>
---
This is actually pretty easy to test on Linux if you just set "X"
yourself.  I confirmed that it is needed for "make X=.exe unit-tests" to
work, and that fudging an "X = .exe" line into config.mak.uname, as
would happen on real platforms works with the extra include.

 t/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index b7a6fefe28..281f4c3534 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -6,6 +6,7 @@ include ../shared.mak
 # Copyright (c) 2005 Junio C Hamano
 #
 
+-include ../config.mak.uname
 -include ../config.mak.autogen
 -include ../config.mak
 
@@ -42,7 +43,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
 TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
 CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
 CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)
+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))
+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
 
 # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
 # checks all tests in all scripts via a single invocation, so tell individual
-- 
2.43.0.797.g29b680fc68

Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources

[Adam Dinwoodie]

On Tue, 30 Jan 2024 at 00:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Adam Dinwoodie <adam@dinwoodie.org> writes:
>
> >> Hmm, good point. It seems like the answer should obviously be "yes", but
> >> Windows CI seemed to pass all the same (and I checked that it indeed ran
> >> the unit tests). Do we only get the $X suffix for MSVC builds or
> >> something? Looks like maybe cygwin, as well.
> >
> > Cygwin will automatically append ".exe" when doing directory listings;
> > a check if the file "a" exists will return true on Cygwin if "a" or
> > "a.exe" exists; a glob for "a*" in a directory containing files "a1"
> > and "a2.exe" will return "a1" and "a2". This causes problems in some
> > edge cases, but it means *nix scripts and applications are much more
> > likely to work without any Cygwin-specific handling. I *think* this
> > logic is carried downstream to MSYS2 and thence to Git for Windows.
>
> Interesting, especially that "a*" is globbed to "a2" and not
> "a2.exe".

My error, sorry! I've just double-checked and Cygwin's globbing will
report the file with the .exe extension. I clearly misremembered how
this works.

Having looked up a bit more of the implementation is simply that, if
Cygwin tries to open a file named "x" and doesn't find it, it will
attempt to open "x.exe" before it returns the failure. This means that
scripts that call (say) `/usr/bin/env bash` or `cat` or `[ "$x" = "$y"
]`  or whatever will broadly Just Work(TM) rather than needing to be
rewritten with the extension added. But the behaviour only applies
when Cygwin is looking for a specific filename.

Re: [PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   1. It will notice if we failed to build one or more unit-tests for
>      some reason (wheras the current code just runs whatever made it to
>      the bin/ directory).

"whereas"

> would be true even if we had acess to the top-level's UNIT_TEST_PROGRAMS

"access"

I've typofixed them while queuing; no need to resend the patch only
to fix it.

Thanks.

Re: [PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources

[Phillip Wood]

Hi Peff

On 30/01/2024 05:40, Jeff King wrote:
> We decide on the set of unit tests to run by asking make to expand the
> wildcard "t/unit-tests/bin/*". One unfortunate outcome of this is that
> we'll run anything in that directory, even if it is leftover cruft from
> a previous build. This isn't _quite_ as bad as it sounds, since in
> theory the unit tests executables are self-contained (so if they passed
> before, they'll pass even though they now have nothing to do with the
> checked out version of Git). But at the very least it's wasteful, and if
> they _do_ fail it can be quite confusing to understand why they are
> being run at all.
> 
> This wildcarding presumably came from our handling of the regular
> shell-script tests, which use $(wildcard t[0-9][0-9][0-9][0-9]-*.sh).
> But the difference there is that those are actual tracked files. So if
> you checkout a different commit, they'll go away. Whereas the contents
> of unit-tests/bin are ignored (so not only do they stick around, but you
> are not even warned of the stale files via "git status").
> 
> This patch fixes the situation by looking for the actual unit-test
> source files and then massaging those names into the final executable
> names. This has two additional benefits:
> 
>    1. It will notice if we failed to build one or more unit-tests for
>       some reason (wheras the current code just runs whatever made it to
>       the bin/ directory).

The downside to this is that if there are any cruft C files lying about 
t/unit-tests we'll fail to run the unit tests. In the past we've avoided 
using wildcard rules on C sources to avoid problems like this[1]. This 
change may well be the lesser of two evils but a test run that fails due 
to a cruft C file cannot be fixed by "make clean && make" whereas that 
will fix the problem of a stale test executable.

>    2. The wildcard should avoid other build cruft, like the pdb files we
>       worked around in 0df903d402 (unit-tests: do not mistake `.pdb`
>       files for being executable, 2023-09-25).
> 
> Our new wildcard does make an assumption that unit tests are built from
> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS
> from the top-level Makefile. But doing so is tricky unless we reorganize
> that Makefile to split the source file lists into include-able subfiles.
> That might be worth doing in general, but in the meantime, the
> assumptions made by the wildcard here seems reasonable.

Using UNIT_TEST_PROGRAMS would definitely be a nicer approach long term.

Best Wishes

Phillip

[1] https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/

Re: [PATCH v2 0/3] some unit-test Makefile polishing

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jan 28, 2024 at 10:15:40PM -0500, Jeff King wrote:
>
>> The patches fixes two small hiccups I found with the unit-tests. Neither
>> is a show-stopper, but mostly just small quality-of-life fixes.
>
> And here's another iteration based on the feedback from v1. It uses the
> mkdir_p template mentioned by Gábor, fixes the $(X) issue mentioned by
> Patrick, and adds a new patch to handle the directory in "make clean".
>
> No range diff, as range-diff refuses to admit that the patches are
> related (presumably because even though the changes are small, the
> original patches were also tiny).
>
>   [1/3]: Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN
>   [2/3]: Makefile: remove UNIT_TEST_BIN directory with "make clean"
>   [3/3]: t/Makefile: get UNIT_TESTS list from C sources

Merging this topic seems to stop all "win test (n)" jobs at GitHub
CI, which is puzzling (I would have understood a broken build, but
that is not what we are seeing).

https://github.com/git/git/actions/runs/7748054008 is a run of 'next'
that is broken.

https://github.com/git/git/actions/runs/7748547579 is a run of 'seen~1'
with this topic reverted (the ps/reftable-backend topic is excluded),
which seems to pass.

Does it ring a bell, anybody?

Thanks.

Re: [PATCH v2 0/3] some unit-test Makefile polishing

[Johannes Schindelin]

Hi Junio,

On Thu, 1 Feb 2024, Junio C Hamano wrote:

> https://github.com/git/git/actions/runs/7748054008 is a run of 'next'
> that is broken.
>
> https://github.com/git/git/actions/runs/7748547579 is a run of 'seen~1'
> with this topic reverted (the ps/reftable-backend topic is excluded),
> which seems to pass.
>
> Does it ring a bell, anybody?

Yes, it does ring a clear bell over here.
https://github.com/git/git/actions/runs/7748054008/job/21130098985#step:5:81
points to the culprit:

    fatal: not a git repository (or any of the parent directories): .git
    make: *** [../config.mak.uname:753: vcxproj] Error 128

The line 753 of that file (as can be seen at
https://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)
is the first statement of the `vcxproj` target, executing `update-refresh`:

    vcxproj:
        # Require clean work tree
        git update-index -q --refresh && \
        git diff-files --quiet && \
        git diff-index --cached --quiet HEAD --
        [...]

This means that `vcxproj` is executed. And the explanation is in
https://github.com/git/git/actions/runs/7748054008/job/21130098985#step:5:78,
which runs `make --quiet -C t 'T=<long-list-of-files>'`, crucially
_without_ specifying any Makefile rule, and the `vcxproj` rule happens to
be the first one that is defined on Windows, so it's used by default.

One workaround would be to remove the `vcxproj` rule (which by now has
been safely superseded by the CMake support we have in
`contrib/buildsystems/`).

But the safer way would be to insert these two lines at the beginning of
`t/Makefile` (cargo-culted from the top-level `Makefile`):

    # The default target of this Makefile is...
    all::

Ciao,
Johannes

Re: [PATCH v2 0/3] some unit-test Makefile polishing

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> The line 753 of that file (as can be seen at
> https://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)

Ouch.  When it is laid out like this it is very obvious why this is
broken, and what its workaround should be.

Thanks.  Let's queue this on top.

------- >8 ------------- >8 ------------- >8 ------------- >8 -------
Subject: [PATCH] t/Makefile: say the default target upfront

Similar to how 2731d048 (Makefile: say the default target upfront.,
2005-12-01) added the default target to the very beginning of the
main Makefile to prevent a random rule that happens to be defined
first in an included makefile fragments from becoming the default
target, protect this Makefile the same way.

This started to matter as we started to include config.mak.uname
and that included makefile fragment does more than defining Make
macros, unfortunately.

Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 281f4c3534..2d95046f26 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -1,3 +1,6 @@
+# The default target of this Makefile is...
+all::
+
 # Import tree-wide shared Makefile behavior and libraries
 include ../shared.mak
 
@@ -52,7 +55,7 @@ UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))
 # scripts not to run the external "chainlint.pl" script themselves
 CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&
 
-all: $(DEFAULT_TEST_TARGET)
+all:: $(DEFAULT_TEST_TARGET)
 
 test: pre-clean check-chainlint $(TEST_LINT)
 	$(CHAINLINTSUPPRESS) $(MAKE) aggregate-results-and-cleanup
-- 
2.43.0-522-g2a540e432f

Re: [PATCH v2 0/3] some unit-test Makefile polishing

[Jeff King]

On Fri, Feb 02, 2024 at 05:32:42PM -0800, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The line 753 of that file (as can be seen at
> > https://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)
> 
> Ouch.  When it is laid out like this it is very obvious why this is
> broken, and what its workaround should be.
> 
> Thanks.  Let's queue this on top.
> 
> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------
> Subject: [PATCH] t/Makefile: say the default target upfront
> 
> Similar to how 2731d048 (Makefile: say the default target upfront.,
> 2005-12-01) added the default target to the very beginning of the
> main Makefile to prevent a random rule that happens to be defined
> first in an included makefile fragments from becoming the default
> target, protect this Makefile the same way.
> 
> This started to matter as we started to include config.mak.uname
> and that included makefile fragment does more than defining Make
> macros, unfortunately.
> 
> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks both of you for identifying and fixing this. I should have been
able to catch this, as it triggers with a simple "make" in the t/
directory (rather than "make prove" or "make unit-test", which is of
course what I checked).

Sorry I'm slow to chime in; I've been offline all week (and probably
will be for another few days) due to a family emergency. But hopefully
with this fix the topic is OK now.

-Peff