{"thread":{"id":"60805","subject":"[PATCH 0/2] some unit-test Makefile polishing","startedAt":"2024-01-29T03:15:41Z","lastAt":"2024-02-04T04:41:14Z","messageCount":24,"participants":["Jeff King","Patrick Steinhardt","SZEDER Gábor","Adam Dinwoodie","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"487491","messageId":"20240129031540.GA2433764@coredump.intra.peff.net","threadId":"60805","inReplyTo":null,"subject":"[PATCH 0/2] some unit-test Makefile polishing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T03:15:40Z","receivedAt":"2024-01-29T03:15:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The patches fixes two small hiccups I found with the unit-tests. Neither\nis a show-stopper, but mostly just small quality-of-life fixes.\n\n  [1/2]: Makefile: use order-only prereq for UNIT_TEST_BIN\n  [2/2]: t/Makefile: get UNIT_TESTS list from C sources\n\n Makefile   | 2 +-\n t/Makefile | 4 +++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\n-Peff\n"},{"id":"487492","messageId":"20240129031816.GA2433899@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240129031540.GA2433764@coredump.intra.peff.net","subject":"[PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T03:18:16Z","receivedAt":"2024-01-29T03:18:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with\n\"mkdir -p\". And so the recipe for UNIT_TEST_PROGS, which put their\noutput in that directory, depend on UNIT_TEST_BIN to make sure it's\nthere.\n\nBut using a normal dependency leads to weird outcomes, because the\ntimestamp of the directory is important. For example, try this:\n\n  $ make\n  [...builds everything...]\n\n  [now re-build one unit test]\n  $ touch t/unit-tests/t-ctype.c\n  $ make\n      SUBDIR templates\n      CC t/unit-tests/t-ctype.o\n      LINK t/unit-tests/bin/t-ctype\n\nSo far so good. Now running make again should build nothing. But it\ndoesn't!\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-basic\n      LINK t/unit-tests/bin/t-mem-pool\n      LINK t/unit-tests/bin/t-strbuf\n\nEr, what? Let's rebuild again:\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-ctype\n\nWeird. And now we ping-pong back and forth forever:\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-basic\n      LINK t/unit-tests/bin/t-mem-pool\n      LINK t/unit-tests/bin/t-strbuf\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-ctype\n\nWhat happens is that writing t/unit-tests/bin/t-ctype updates the mtime\nof the directory t/unit-tests/bin. And then on the next invocation of\nmake, all of those other tests are now older and so get rebuilt. And\nback and forth forever.\n\nWe can fix this by using an order-only prereq. This is a GNU-ism that\ntells make to only care that the dependency exists at all, and to ignore\nits mtime. It was designed for exactly this sort of situation (the\ndocumentation example even uses \"mkdir\").\n\nWe already rely on GNU make, so that's not a problem. This particular\nfeature was added in GNU make 3.80, released in October 2002. This is\nobviously quite old by date, but it's also worth thinking about macOS,\nas Apple stopped updating packages that switched to GPLv3 tools. In this\ntheir dev tools ship GNU make 3.81, which is recent enough.\n\nIf it is a problem, there are two alternatives:\n\n  - we can just \"mkdir -p\" in the recipe to build the individual\n    binaries. This will mean some redundant \"mkdir\" calls, but only when\n    actually invoking the compiler.\n\n  - we could stop making the directory on the fly, and just add it with\n    a .gitignore of \"*\". This would work fine, but might be awkward when\n    moving back and forth in history.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI may be overly paranoid about the \".gitignore\" strategy. I feel like\nI've been bitten by this in the past by things switching from source to\nbuild (I think with git-remote-testgit). But that's an actual built\nfile. Git would probably be OK with the \"bin/\" directory coming and\ngoing as a tracked entity, because the index really only cares about\nthe file \"bin/.gitignore\". Still, this make fix was easy enough.\n\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex 1a62e48759..958f4cd0bf 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3866,7 +3866,7 @@ fuzz-all: $(FUZZ_PROGRAMS)\n $(UNIT_TEST_BIN):\n \t@mkdir -p $(UNIT_TEST_BIN)\n \n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n \n-- \n2.43.0.797.g29b680fc68\n\n"},{"id":"487493","messageId":"20240129031933.GB2433899@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240129031540.GA2433764@coredump.intra.peff.net","subject":"[PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T03:19:33Z","receivedAt":"2024-01-29T03:19:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We decide on the set of unit tests to run by asking make to expand the\nwildcard \"t/unit-tests/bin/*\". One unfortunate outcome of this is that\nwe'll run anything in that directory, even if it is leftover cruft from\na previous build. This isn't _quite_ as bad as it sounds, since in\ntheory the unit test executables are self-contained (so if they passed\nbefore, they'll pass even though they now have nothing to do with the\nchecked out version of Git). But at the very least it's wasteful, and if\nthey _do_ fail it can be quite confusing to understand why they are\nbeing run at all.\n\nThis wildcarding presumably came from our handling of the regular\nshell-script tests, which match \"t[0-9][0-9][0-9][0-9]-*.sh\".  But the\ndifference there is that those are actual tracked files. So if you\ncheckout a different commit, they'll go away. Whereas the contents of\nunit-tests/bin are ignored (so not only do they stick around, but you\nare not even warned of the stale files via \"git status\").\n\nThis patch fixes the situation by looking for the actual unit-test\nsource files and then massaging those names into the final executable\nnames. This has two additional benefits:\n\n  1. It will notice if we failed to build one or more unit-tests for\n     some reason (wheras the current code just runs whatever made it to\n     the bin/ directory).\n\n  2. The wildcard should avoid other build cruft, like the pdb files we\n     worked around in 0df903d402 (unit-tests: do not mistake `.pdb`\n     files for being executable, 2023-09-25).\n\nOur new wildcard does make an assumption that unit tests are build from\nC sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS\nfrom the top-level Makefile. But doing so is tricky unless we reorganize\nthat Makefile to split the source file lists into include-able subfiles.\nThat might be worth doing in general, but in the meantime, the\nassumptions made by the wildcard here seems reasonable.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI of course hit this when moving between \"next\" and \"master\" for an\nup-and-coming unit-test file which sometimes failed.\n\n t/Makefile | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex b7a6fefe28..c5c6e2ef6b 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n-- \n2.43.0.797.g29b680fc68\n"},{"id":"487507","messageId":"ZbeLcrjIYd4d7PaB@tanuki","threadId":"60805","inReplyTo":"20240129031933.GB2433899@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-29T11:26:42Z","receivedAt":"2024-01-29T11:26:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 28, 2024 at 10:19:33PM -0500, Jeff King wrote:\n> We decide on the set of unit tests to run by asking make to expand the\n> wildcard \"t/unit-tests/bin/*\". One unfortunate outcome of this is that\n> we'll run anything in that directory, even if it is leftover cruft from\n> a previous build. This isn't _quite_ as bad as it sounds, since in\n> theory the unit test executables are self-contained (so if they passed\n> before, they'll pass even though they now have nothing to do with the\n> checked out version of Git). But at the very least it's wasteful, and if\n> they _do_ fail it can be quite confusing to understand why they are\n> being run at all.\n> \n> This wildcarding presumably came from our handling of the regular\n> shell-script tests, which match \"t[0-9][0-9][0-9][0-9]-*.sh\".  But the\n> difference there is that those are actual tracked files. So if you\n> checkout a different commit, they'll go away. Whereas the contents of\n> unit-tests/bin are ignored (so not only do they stick around, but you\n> are not even warned of the stale files via \"git status\").\n> \n> This patch fixes the situation by looking for the actual unit-test\n> source files and then massaging those names into the final executable\n> names. This has two additional benefits:\n> \n>   1. It will notice if we failed to build one or more unit-tests for\n>      some reason (wheras the current code just runs whatever made it to\n>      the bin/ directory).\n> \n>   2. The wildcard should avoid other build cruft, like the pdb files we\n>      worked around in 0df903d402 (unit-tests: do not mistake `.pdb`\n>      files for being executable, 2023-09-25).\n> \n> Our new wildcard does make an assumption that unit tests are build from\n> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS\n> from the top-level Makefile. But doing so is tricky unless we reorganize\n> that Makefile to split the source file lists into include-able subfiles.\n> That might be worth doing in general, but in the meantime, the\n> assumptions made by the wildcard here seems reasonable.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I of course hit this when moving between \"next\" and \"master\" for an\n> up-and-coming unit-test file which sometimes failed.\n> \n>  t/Makefile | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/Makefile b/t/Makefile\n> index b7a6fefe28..c5c6e2ef6b 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n>  TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n>  CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n>  CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n> -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n> +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n> +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n> +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n\nWouldn't we have to honor `$X` on Windows systems so that the unit tests\nhave the expected \".exe\" suffix here?\n\nOther than this question the patch series looks good to me, thanks!\n\nPatrick\n"},{"id":"487530","messageId":"20240129174918.GA3765717@coredump.intra.peff.net","threadId":"60805","inReplyTo":"ZbeLcrjIYd4d7PaB@tanuki","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-29T17:49:18Z","receivedAt":"2024-01-29T17:49:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 12:26:42PM +0100, Patrick Steinhardt wrote:\n\n> > -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n> > +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n> > +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n> > +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n> \n> Wouldn't we have to honor `$X` on Windows systems so that the unit tests\n> have the expected \".exe\" suffix here?\n\nHmm, good point. It seems like the answer should obviously be \"yes\", but\nWindows CI seemed to pass all the same (and I checked that it indeed ran\nthe unit tests). Do we only get the $X suffix for MSVC builds or\nsomething? Looks like maybe cygwin, as well.\n\nI imagine the solution is just:\n\ndiff --git a/t/Makefile b/t/Makefile\nindex c5c6e2ef6b..9b9b30f559 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -43,7 +43,7 @@ TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n-UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))\n UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n\nbut it looks like we might need to include config.mak.uname, as well. It\nwould be nice to identify a build that actually needs it so I can\nconfirm that the fix works.\n\n-Peff\n"},{"id":"487543","messageId":"20240129202201.GA9612@szeder.dev","threadId":"60805","inReplyTo":"20240129031816.GA2433899@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2024-01-29T20:22:01Z","receivedAt":"2024-01-29T20:22:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Jan 28, 2024 at 10:18:16PM -0500, Jeff King wrote:\n> We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with\n> \"mkdir -p\". And so the recipe for UNIT_TEST_PROGS, which put their\n> output in that directory, depend on UNIT_TEST_BIN to make sure it's\n> there.\n> \n> But using a normal dependency leads to weird outcomes, because the\n> timestamp of the directory is important. For example, try this:\n> \n>   $ make\n>   [...builds everything...]\n> \n>   [now re-build one unit test]\n>   $ touch t/unit-tests/t-ctype.c\n>   $ make\n>       SUBDIR templates\n>       CC t/unit-tests/t-ctype.o\n>       LINK t/unit-tests/bin/t-ctype\n> \n> So far so good. Now running make again should build nothing. But it\n> doesn't!\n> \n>   $ make\n>       SUBDIR templates\n>       LINK t/unit-tests/bin/t-basic\n>       LINK t/unit-tests/bin/t-mem-pool\n>       LINK t/unit-tests/bin/t-strbuf\n> \n> Er, what? Let's rebuild again:\n> \n>   $ make\n>       SUBDIR templates\n>       LINK t/unit-tests/bin/t-ctype\n> \n> Weird. And now we ping-pong back and forth forever:\n> \n>   $ make\n>       SUBDIR templates\n>       LINK t/unit-tests/bin/t-basic\n>       LINK t/unit-tests/bin/t-mem-pool\n>       LINK t/unit-tests/bin/t-strbuf\n>   $ make\n>       SUBDIR templates\n>       LINK t/unit-tests/bin/t-ctype\n> \n> What happens is that writing t/unit-tests/bin/t-ctype updates the mtime\n> of the directory t/unit-tests/bin. And then on the next invocation of\n> make, all of those other tests are now older and so get rebuilt. And\n> back and forth forever.\n> \n> We can fix this by using an order-only prereq. This is a GNU-ism that\n> tells make to only care that the dependency exists at all, and to ignore\n> its mtime. It was designed for exactly this sort of situation (the\n> documentation example even uses \"mkdir\").\n> \n> We already rely on GNU make, so that's not a problem. This particular\n> feature was added in GNU make 3.80, released in October 2002. This is\n> obviously quite old by date, but it's also worth thinking about macOS,\n> as Apple stopped updating packages that switched to GPLv3 tools. In this\n> their dev tools ship GNU make 3.81, which is recent enough.\n> \n> If it is a problem, there are two alternatives:\n> \n>   - we can just \"mkdir -p\" in the recipe to build the individual\n>     binaries. This will mean some redundant \"mkdir\" calls, but only when\n>     actually invoking the compiler.\n> \n>   - we could stop making the directory on the fly, and just add it with\n>     a .gitignore of \"*\". This would work fine, but might be awkward when\n>     moving back and forth in history.\n\nA third alternative is to use $(call mkdir_p_parent_template) in the\nrecipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency\nand target.  It will only run mkdir when needed, and it's a well\nestablished pattern in our Makefile, so you won't have to spend a\nparagraph or two arguing about potential problems with GNU-isms :)\n\nOn a related note, 'make clean' doesn't remove this 't/unit-tests/bin'\ndirectory.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I may be overly paranoid about the \".gitignore\" strategy. I feel like\n> I've been bitten by this in the past by things switching from source to\n> build (I think with git-remote-testgit). But that's an actual built\n> file. Git would probably be OK with the \"bin/\" directory coming and\n> going as a tracked entity, because the index really only cares about\n> the file \"bin/.gitignore\". Still, this make fix was easy enough.\n> \n>  Makefile | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/Makefile b/Makefile\n> index 1a62e48759..958f4cd0bf 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3866,7 +3866,7 @@ fuzz-all: $(FUZZ_PROGRAMS)\n>  $(UNIT_TEST_BIN):\n>  \t@mkdir -p $(UNIT_TEST_BIN)\n>  \n> -$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)\n> +$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n>  \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n>  \n> -- \n> 2.43.0.797.g29b680fc68\n> \n> \n"},{"id":"487556","messageId":"CA+kUOanDydgCEax9RFu_xVXkx_LeiSPOoWiUpwAg=EVQxSDJRw@mail.gmail.com","threadId":"60805","inReplyTo":"20240129174918.GA3765717@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2024-01-29T21:31:11Z","receivedAt":"2024-01-29T21:31:46Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Mon, 29 Jan 2024 at 17:49, Jeff King wrote:\n>\n> On Mon, Jan 29, 2024 at 12:26:42PM +0100, Patrick Steinhardt wrote:\n>\n> > > -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n> > > +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n> > > +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n> > > +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n> >\n> > Wouldn't we have to honor `$X` on Windows systems so that the unit tests\n> > have the expected \".exe\" suffix here?\n>\n> Hmm, good point. It seems like the answer should obviously be \"yes\", but\n> Windows CI seemed to pass all the same (and I checked that it indeed ran\n> the unit tests). Do we only get the $X suffix for MSVC builds or\n> something? Looks like maybe cygwin, as well.\n\nCygwin will automatically append \".exe\" when doing directory listings;\na check if the file \"a\" exists will return true on Cygwin if \"a\" or\n\"a.exe\" exists; a glob for \"a*\" in a directory containing files \"a1\"\nand \"a2.exe\" will return \"a1\" and \"a2\". This causes problems in some\nedge cases, but it means *nix scripts and applications are much more\nlikely to work without any Cygwin-specific handling. I *think* this\nlogic is carried downstream to MSYS2 and thence to Git for Windows.\n\nAs a result, I'm not surprised this worked without handling $X, but I\ndon't think there's any harm in adding it either.\n"},{"id":"487558","messageId":"xmqqplxjhkqi.fsf@gitster.g","threadId":"60805","inReplyTo":"20240129031933.GB2433899@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T21:51:01Z","receivedAt":"2024-01-29T21:51:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Our new wildcard does make an assumption that unit tests are build from\n\n\"build\" -> \"built\", probably.\n\n> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS\n> from the top-level Makefile. But doing so is tricky unless we reorganize\n> that Makefile to split the source file lists into include-able subfiles.\n> That might be worth doing in general, but in the meantime, the\n> assumptions made by the wildcard here seems reasonable.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I of course hit this when moving between \"next\" and \"master\" for an\n> up-and-coming unit-test file which sometimes failed.\n\nThanks.  globbing the build products is indeed sloppy for all the\nreasons you mentioned.  Will queue.\n\n>  t/Makefile | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/Makefile b/t/Makefile\n> index b7a6fefe28..c5c6e2ef6b 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -42,7 +42,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n>  TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n>  CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n>  CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n> -UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n> +UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n> +UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%,$(UNIT_TEST_SOURCES))\n> +UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n>  \n>  # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n>  # checks all tests in all scripts via a single invocation, so tell individual\n"},{"id":"487559","messageId":"xmqqjznrhk14.fsf@gitster.g","threadId":"60805","inReplyTo":"20240129202201.GA9612@szeder.dev","subject":"Re: [PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-29T22:06:15Z","receivedAt":"2024-01-29T22:06:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> A third alternative is to use $(call mkdir_p_parent_template) in the\n> recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency\n> and target.\n\nYeah, that sounds like a good approach in this case.\n\n> On a related note, 'make clean' doesn't remove this 't/unit-tests/bin'\n> directory.\n\nNot a new problem, but I did notice this, too.\n\nThanks.\n\n Makefile | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git c/Makefile w/Makefile\nindex 958f4cd0bf..7f035a1c9f 100644\n--- c/Makefile\n+++ w/Makefile\n@@ -3676,7 +3676,7 @@ cocciclean:\n \t$(RM) contrib/coccinelle/*.cocci.patch\n \n clean: profile-clean coverage-clean cocciclean\n-\t$(RM) -r .build\n+\t$(RM) -r .build $(UNIT_TEST_BIN)\n \t$(RM) po/git.pot po/git-core.pot\n \t$(RM) git.res\n \t$(RM) $(OBJECTS)\n@@ -3863,10 +3863,8 @@ $(FUZZ_PROGRAMS): all\n \n fuzz-all: $(FUZZ_PROGRAMS)\n \n-$(UNIT_TEST_BIN):\n-\t@mkdir -p $(UNIT_TEST_BIN)\n-\n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS | $(UNIT_TEST_BIN)\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n+\t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n \n"},{"id":"487566","messageId":"xmqqeddzfywg.fsf@gitster.g","threadId":"60805","inReplyTo":"CA+kUOanDydgCEax9RFu_xVXkx_LeiSPOoWiUpwAg=EVQxSDJRw@mail.gmail.com","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-30T00:27:59Z","receivedAt":"2024-01-30T00:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie <adam@dinwoodie.org> writes:\n\n>> Hmm, good point. It seems like the answer should obviously be \"yes\", but\n>> Windows CI seemed to pass all the same (and I checked that it indeed ran\n>> the unit tests). Do we only get the $X suffix for MSVC builds or\n>> something? Looks like maybe cygwin, as well.\n>\n> Cygwin will automatically append \".exe\" when doing directory listings;\n> a check if the file \"a\" exists will return true on Cygwin if \"a\" or\n> \"a.exe\" exists; a glob for \"a*\" in a directory containing files \"a1\"\n> and \"a2.exe\" will return \"a1\" and \"a2\". This causes problems in some\n> edge cases, but it means *nix scripts and applications are much more\n> likely to work without any Cygwin-specific handling. I *think* this\n> logic is carried downstream to MSYS2 and thence to Git for Windows.\n\nInteresting, especially that \"a*\" is globbed to \"a2\" and not\n\"a2.exe\".\n\n> As a result, I'm not surprised this worked without handling $X, but I\n> don't think there's any harm in adding it either.\n\nOK.\n\nI wonder if something like this is sufficient?  I am not sure if we\nshould lift the building of t/unit-tests/* up to the primary Makefile\nto mimic the way stuff related to test-tool are built and linked.\nThat way, we do not have to contaminate t/Makefile with compilation\nrelated stuff that we didn't need originally.\n\n t/Makefile | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git c/t/Makefile w/t/Makefile\nindex b7a6fefe28..010ce083b1 100644\n--- c/t/Makefile\n+++ w/t/Makefile\n@@ -6,6 +6,7 @@ include ../shared.mak\n # Copyright (c) 2005 Junio C Hamano\n #\n \n+include ../config.mak.uname\n -include ../config.mak.autogen\n -include ../config.mak\n \n@@ -42,7 +43,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$X,$(UNIT_TEST_SOURCES))\n+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n\n\n"},{"id":"487574","messageId":"20240130052104.GA154684@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240129202201.GA9612@szeder.dev","subject":"Re: [PATCH 1/2] Makefile: use order-only prereq for UNIT_TEST_BIN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:21:04Z","receivedAt":"2024-01-30T05:21:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 09:22:01PM +0100, SZEDER Gábor wrote:\n\n> > If it is a problem, there are two alternatives:\n> > \n> >   - we can just \"mkdir -p\" in the recipe to build the individual\n> >     binaries. This will mean some redundant \"mkdir\" calls, but only when\n> >     actually invoking the compiler.\n> > \n> >   - we could stop making the directory on the fly, and just add it with\n> >     a .gitignore of \"*\". This would work fine, but might be awkward when\n> >     moving back and forth in history.\n> \n> A third alternative is to use $(call mkdir_p_parent_template) in the\n> recipe and get rid of the thus unnecessary UNIT_TEST_BIN dependency\n> and target.  It will only run mkdir when needed, and it's a well\n> established pattern in our Makefile, so you won't have to spend a\n> paragraph or two arguing about potential problems with GNU-isms :)\n\nThanks, I somehow didn't know about that (and didn't find it when\ngrepping around for similar cases, probably because \"mkdir -p\" no longer\nappears in those cases ;) ).\n\nI agree it's a better solution here. I'll send a v2 in a moment with\nthat.\n\n(Ironically, that template requires \"call\" which is in make 3.81, but\nthe commit adding it didn't discuss that at all).\n\n> On a related note, 'make clean' doesn't remove this 't/unit-tests/bin'\n> directory.\n\nNot the end of the world, as we do clean out the contents, so \"ls-files\n-o\" would not mention any leftover cruft. But I agree that we should\nstrive for \"make clean\" to be the opposite of \"make\" as much as\npossible.\n\nI'll add in a patch to v2.\n\n-Peff\n"},{"id":"487576","messageId":"20240130052331.GB154684@coredump.intra.peff.net","threadId":"60805","inReplyTo":"CA+kUOanDydgCEax9RFu_xVXkx_LeiSPOoWiUpwAg=EVQxSDJRw@mail.gmail.com","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:23:31Z","receivedAt":"2024-01-30T05:23:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 09:31:11PM +0000, Adam Dinwoodie wrote:\n\n> > Hmm, good point. It seems like the answer should obviously be \"yes\", but\n> > Windows CI seemed to pass all the same (and I checked that it indeed ran\n> > the unit tests). Do we only get the $X suffix for MSVC builds or\n> > something? Looks like maybe cygwin, as well.\n> \n> Cygwin will automatically append \".exe\" when doing directory listings;\n> a check if the file \"a\" exists will return true on Cygwin if \"a\" or\n> \"a.exe\" exists; a glob for \"a*\" in a directory containing files \"a1\"\n> and \"a2.exe\" will return \"a1\" and \"a2\". This causes problems in some\n> edge cases, but it means *nix scripts and applications are much more\n> likely to work without any Cygwin-specific handling. I *think* this\n> logic is carried downstream to MSYS2 and thence to Git for Windows.\n> \n> As a result, I'm not surprised this worked without handling $X, but I\n> don't think there's any harm in adding it either.\n\nAh, that makes sense. I do think it will work under the conditions you\nlaid out, but I don't know if there are platforms where that's not the\ncase. I'll add the $(X) just to be sure (as that really feels more\nconsistent with what the top-level Makefile is doing anyway).\n\nThanks.\n\n-Peff\n"},{"id":"487577","messageId":"20240130052511.GC154684@coredump.intra.peff.net","threadId":"60805","inReplyTo":"xmqqeddzfywg.fsf@gitster.g","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:25:11Z","receivedAt":"2024-01-30T05:25:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 29, 2024 at 04:27:59PM -0800, Junio C Hamano wrote:\n\n> > As a result, I'm not surprised this worked without handling $X, but I\n> > don't think there's any harm in adding it either.\n> \n> OK.\n> \n> I wonder if something like this is sufficient?\n> [...]\n\nYeah, that matches the patch I came up with locally. I'll roll that into\nv2.\n\n> I am not sure if we\n> should lift the building of t/unit-tests/* up to the primary Makefile\n> to mimic the way stuff related to test-tool are built and linked.\n> That way, we do not have to contaminate t/Makefile with compilation\n> related stuff that we didn't need originally.\n\nI didn't quite understand this comment, though. The building of\nt/unit-tests _is_ in the top-level Makefile. It's just here in the\nrecursive Makefile that we need to have the list of built programs.\n\n-Peff\n"},{"id":"487579","messageId":"20240130053714.GA165967@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240129031540.GA2433764@coredump.intra.peff.net","subject":"[PATCH v2 0/3] some unit-test Makefile polishing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:37:14Z","receivedAt":"2024-01-30T05:37:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 28, 2024 at 10:15:40PM -0500, Jeff King wrote:\n\n> The patches fixes two small hiccups I found with the unit-tests. Neither\n> is a show-stopper, but mostly just small quality-of-life fixes.\n\nAnd here's another iteration based on the feedback from v1. It uses the\nmkdir_p template mentioned by Gábor, fixes the $(X) issue mentioned by\nPatrick, and adds a new patch to handle the directory in \"make clean\".\n\nNo range diff, as range-diff refuses to admit that the patches are\nrelated (presumably because even though the changes are small, the\noriginal patches were also tiny).\n\n  [1/3]: Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN\n  [2/3]: Makefile: remove UNIT_TEST_BIN directory with \"make clean\"\n  [3/3]: t/Makefile: get UNIT_TESTS list from C sources\n\n Makefile   | 10 ++++------\n t/Makefile |  5 ++++-\n 2 files changed, 8 insertions(+), 7 deletions(-)\n\n-Peff\n"},{"id":"487580","messageId":"20240130053740.GA166699@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240130053714.GA165967@coredump.intra.peff.net","subject":"[PATCH v2 1/3] Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:37:40Z","receivedAt":"2024-01-30T05:37:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We build the UNIT_TEST_BIN directory (t/unit-tests/bin) on the fly with\n\"mkdir -p\". And so the recipe for UNIT_TEST_PROGS, which put their\noutput in that directory, depend on UNIT_TEST_BIN to make sure it's\nthere.\n\nBut using a normal dependency leads to weird outcomes, because the\ntimestamp of the directory is important. For example, try this:\n\n  $ make\n  [...builds everything...]\n\n  [now re-build one unit test]\n  $ touch t/unit-tests/t-ctype.c\n  $ make\n      SUBDIR templates\n      CC t/unit-tests/t-ctype.o\n      LINK t/unit-tests/bin/t-ctype\n\nSo far so good. Now running make again should build nothing. But it\ndoesn't!\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-basic\n      LINK t/unit-tests/bin/t-mem-pool\n      LINK t/unit-tests/bin/t-strbuf\n\nEr, what? Let's rebuild again:\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-ctype\n\nWeird. And now we ping-pong back and forth forever:\n\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-basic\n      LINK t/unit-tests/bin/t-mem-pool\n      LINK t/unit-tests/bin/t-strbuf\n  $ make\n      SUBDIR templates\n      LINK t/unit-tests/bin/t-ctype\n\nWhat happens is that writing t/unit-tests/bin/t-ctype updates the mtime\nof the directory t/unit-tests/bin. And then on the next invocation of\nmake, all of those other tests are now older and so get rebuilt. And\nback and forth forever.\n\nWe can fix this by making the directory as part of the build recipe for\nthe programs, using the template from 0b6d0bc924 (Makefiles: add and use\nwildcard \"mkdir -p\" template, 2022-03-03).\n\nHelped-by: SZEDER Gábor <szeder.dev@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 0f748a52e6..228f34a3fe 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3868,10 +3868,8 @@ $(FUZZ_PROGRAMS): %: %.o oss-fuzz/dummy-cmd-main.o $(GITLIBS) GIT-LDFLAGS\n \n fuzz-all: $(FUZZ_PROGRAMS)\n \n-$(UNIT_TEST_BIN):\n-\t@mkdir -p $(UNIT_TEST_BIN)\n-\n-$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)\n+$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS\n+\t$(call mkdir_p_parent_template)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(filter %.a,$^) $(LIBS)\n \n-- \n2.43.0.797.g29b680fc68\n\n"},{"id":"487581","messageId":"20240130053856.GB166699@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240130053714.GA165967@coredump.intra.peff.net","subject":"[PATCH v2 2/3] Makefile: remove UNIT_TEST_BIN directory with \"make clean\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:38:56Z","receivedAt":"2024-01-30T05:38:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We remove $(UNIT_TEST_PROGS), but that leaves the automatically\ngenerated \"bin\" dir they reside in. And once we start cleaning that,\nthere is no point in removing the individual programs, as they'll by\nwiped out by the recurse \"rm\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe layout of the clean rule is kind of weird, in that we put only\nsort-of related things together on their own $(RM) line. I suspect a lot\nmore of this could be lumped together, reducing the total number of\nshells and rm invocations needed, but I guess nobody really cares that\nmuch about the runtime cost of \"make clean\".\n\n Makefile | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 228f34a3fe..b5ae34ea7b 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3680,14 +3680,14 @@ cocciclean:\n \t$(RM) contrib/coccinelle/*.cocci.patch\n \n clean: profile-clean coverage-clean cocciclean\n-\t$(RM) -r .build\n+\t$(RM) -r .build $(UNIT_TEST_BIN)\n \t$(RM) po/git.pot po/git-core.pot\n \t$(RM) git.res\n \t$(RM) $(OBJECTS)\n \t$(RM) headless-git.o\n \t$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)\n \t$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)\n-\t$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)\n+\t$(RM) $(TEST_PROGRAMS)\n \t$(RM) $(FUZZ_PROGRAMS)\n \t$(RM) $(SP_OBJ)\n \t$(RM) $(HCC)\n-- \n2.43.0.797.g29b680fc68\n\n"},{"id":"487582","messageId":"20240130054037.GC166699@coredump.intra.peff.net","threadId":"60805","inReplyTo":"20240130053714.GA165967@coredump.intra.peff.net","subject":"[PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-30T05:40:37Z","receivedAt":"2024-01-30T05:40:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We decide on the set of unit tests to run by asking make to expand the\nwildcard \"t/unit-tests/bin/*\". One unfortunate outcome of this is that\nwe'll run anything in that directory, even if it is leftover cruft from\na previous build. This isn't _quite_ as bad as it sounds, since in\ntheory the unit tests executables are self-contained (so if they passed\nbefore, they'll pass even though they now have nothing to do with the\nchecked out version of Git). But at the very least it's wasteful, and if\nthey _do_ fail it can be quite confusing to understand why they are\nbeing run at all.\n\nThis wildcarding presumably came from our handling of the regular\nshell-script tests, which use $(wildcard t[0-9][0-9][0-9][0-9]-*.sh).\nBut the difference there is that those are actual tracked files. So if\nyou checkout a different commit, they'll go away. Whereas the contents\nof unit-tests/bin are ignored (so not only do they stick around, but you\nare not even warned of the stale files via \"git status\").\n\nThis patch fixes the situation by looking for the actual unit-test\nsource files and then massaging those names into the final executable\nnames. This has two additional benefits:\n\n  1. It will notice if we failed to build one or more unit-tests for\n     some reason (wheras the current code just runs whatever made it to\n     the bin/ directory).\n\n  2. The wildcard should avoid other build cruft, like the pdb files we\n     worked around in 0df903d402 (unit-tests: do not mistake `.pdb`\n     files for being executable, 2023-09-25).\n\nOur new wildcard does make an assumption that unit tests are built from\nC sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS\nfrom the top-level Makefile. But doing so is tricky unless we reorganize\nthat Makefile to split the source file lists into include-able subfiles.\nThat might be worth doing in general, but in the meantime, the\nassumptions made by the wildcard here seems reasonable.\n\nNote that we do need to include config.mak.uname either way, though, as\nwe need the value of $(X) to compute the correct executable names (which\nwould be true even if we had acess to the top-level's UNIT_TEST_PROGRAMS\nvariable).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is actually pretty easy to test on Linux if you just set \"X\"\nyourself.  I confirmed that it is needed for \"make X=.exe unit-tests\" to\nwork, and that fudging an \"X = .exe\" line into config.mak.uname, as\nwould happen on real platforms works with the extra include.\n\n t/Makefile | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex b7a6fefe28..281f4c3534 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -6,6 +6,7 @@ include ../shared.mak\n # Copyright (c) 2005 Junio C Hamano\n #\n \n+-include ../config.mak.uname\n -include ../config.mak.autogen\n -include ../config.mak\n \n@@ -42,7 +43,9 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))\n TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))\n CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))\n CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl\n-UNIT_TESTS = $(sort $(filter-out %.pdb unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))\n+UNIT_TEST_SOURCES = $(wildcard unit-tests/t-*.c)\n+UNIT_TEST_PROGRAMS = $(patsubst unit-tests/%.c,unit-tests/bin/%$(X),$(UNIT_TEST_SOURCES))\n+UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n \n # `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)\n # checks all tests in all scripts via a single invocation, so tell individual\n-- \n2.43.0.797.g29b680fc68\n"},{"id":"487660","messageId":"CA+kUOakoMi4wb0=pAbWiRLmSumiQCXewW3S67cM5WJ4tttYFhg@mail.gmail.com","threadId":"60805","inReplyTo":"xmqqeddzfywg.fsf@gitster.g","subject":"Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2024-01-31T19:13:05Z","receivedAt":"2024-01-31T19:13:41Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Tue, 30 Jan 2024 at 00:28, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Adam Dinwoodie <adam@dinwoodie.org> writes:\n>\n> >> Hmm, good point. It seems like the answer should obviously be \"yes\", but\n> >> Windows CI seemed to pass all the same (and I checked that it indeed ran\n> >> the unit tests). Do we only get the $X suffix for MSVC builds or\n> >> something? Looks like maybe cygwin, as well.\n> >\n> > Cygwin will automatically append \".exe\" when doing directory listings;\n> > a check if the file \"a\" exists will return true on Cygwin if \"a\" or\n> > \"a.exe\" exists; a glob for \"a*\" in a directory containing files \"a1\"\n> > and \"a2.exe\" will return \"a1\" and \"a2\". This causes problems in some\n> > edge cases, but it means *nix scripts and applications are much more\n> > likely to work without any Cygwin-specific handling. I *think* this\n> > logic is carried downstream to MSYS2 and thence to Git for Windows.\n>\n> Interesting, especially that \"a*\" is globbed to \"a2\" and not\n> \"a2.exe\".\n\nMy error, sorry! I've just double-checked and Cygwin's globbing will\nreport the file with the .exe extension. I clearly misremembered how\nthis works.\n\nHaving looked up a bit more of the implementation is simply that, if\nCygwin tries to open a file named \"x\" and doesn't find it, it will\nattempt to open \"x.exe\" before it returns the failure. This means that\nscripts that call (say) `/usr/bin/env bash` or `cat` or `[ \"$x\" = \"$y\"\n]`  or whatever will broadly Just Work(TM) rather than needing to be\nrewritten with the extension added. But the behaviour only applies\nwhen Cygwin is looking for a specific filename.\n"},{"id":"487687","messageId":"xmqqv8792jq2.fsf@gitster.g","threadId":"60805","inReplyTo":"20240130054037.GC166699@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-31T22:58:45Z","receivedAt":"2024-01-31T22:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>   1. It will notice if we failed to build one or more unit-tests for\n>      some reason (wheras the current code just runs whatever made it to\n>      the bin/ directory).\n\n\"whereas\"\n\n> would be true even if we had acess to the top-level's UNIT_TEST_PROGRAMS\n\n\"access\"\n\nI've typofixed them while queuing; no need to resend the patch only\nto fix it.\n\nThanks.\n"},{"id":"487732","messageId":"08303f49-63dd-4141-be34-0d6b5fa28a0b@gmail.com","threadId":"60805","inReplyTo":"20240130054037.GC166699@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] t/Makefile: get UNIT_TESTS list from C sources","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-02-01T10:50:43Z","receivedAt":"2024-02-01T10:50:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 30/01/2024 05:40, Jeff King wrote:\n> We decide on the set of unit tests to run by asking make to expand the\n> wildcard \"t/unit-tests/bin/*\". One unfortunate outcome of this is that\n> we'll run anything in that directory, even if it is leftover cruft from\n> a previous build. This isn't _quite_ as bad as it sounds, since in\n> theory the unit tests executables are self-contained (so if they passed\n> before, they'll pass even though they now have nothing to do with the\n> checked out version of Git). But at the very least it's wasteful, and if\n> they _do_ fail it can be quite confusing to understand why they are\n> being run at all.\n> \n> This wildcarding presumably came from our handling of the regular\n> shell-script tests, which use $(wildcard t[0-9][0-9][0-9][0-9]-*.sh).\n> But the difference there is that those are actual tracked files. So if\n> you checkout a different commit, they'll go away. Whereas the contents\n> of unit-tests/bin are ignored (so not only do they stick around, but you\n> are not even warned of the stale files via \"git status\").\n> \n> This patch fixes the situation by looking for the actual unit-test\n> source files and then massaging those names into the final executable\n> names. This has two additional benefits:\n> \n>    1. It will notice if we failed to build one or more unit-tests for\n>       some reason (wheras the current code just runs whatever made it to\n>       the bin/ directory).\n\nThe downside to this is that if there are any cruft C files lying about \nt/unit-tests we'll fail to run the unit tests. In the past we've avoided \nusing wildcard rules on C sources to avoid problems like this[1]. This \nchange may well be the lesser of two evils but a test run that fails due \nto a cruft C file cannot be fixed by \"make clean && make\" whereas that \nwill fix the problem of a stale test executable.\n\n>    2. The wildcard should avoid other build cruft, like the pdb files we\n>       worked around in 0df903d402 (unit-tests: do not mistake `.pdb`\n>       files for being executable, 2023-09-25).\n> \n> Our new wildcard does make an assumption that unit tests are built from\n> C sources. It would be a bit cleaner if we consulted UNIT_TEST_PROGRAMS\n> from the top-level Makefile. But doing so is tricky unless we reorganize\n> that Makefile to split the source file lists into include-able subfiles.\n> That might be worth doing in general, but in the meantime, the\n> assumptions made by the wildcard here seems reasonable.\n\nUsing UNIT_TEST_PROGRAMS would definitely be a nicer approach long term.\n\nBest Wishes\n\nPhillip\n\n[1] https://lore.kernel.org/git/xmqqtugl102l.fsf@gitster.g/\n\n"},{"id":"487785","messageId":"xmqqh6irwtkd.fsf@gitster.g","threadId":"60805","inReplyTo":"20240130053714.GA165967@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/3] some unit-test Makefile polishing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-02T01:20:18Z","receivedAt":"2024-02-02T01:20:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Jan 28, 2024 at 10:15:40PM -0500, Jeff King wrote:\n>\n>> The patches fixes two small hiccups I found with the unit-tests. Neither\n>> is a show-stopper, but mostly just small quality-of-life fixes.\n>\n> And here's another iteration based on the feedback from v1. It uses the\n> mkdir_p template mentioned by Gábor, fixes the $(X) issue mentioned by\n> Patrick, and adds a new patch to handle the directory in \"make clean\".\n>\n> No range diff, as range-diff refuses to admit that the patches are\n> related (presumably because even though the changes are small, the\n> original patches were also tiny).\n>\n>   [1/3]: Makefile: use mkdir_p_parent_template for UNIT_TEST_BIN\n>   [2/3]: Makefile: remove UNIT_TEST_BIN directory with \"make clean\"\n>   [3/3]: t/Makefile: get UNIT_TESTS list from C sources\n\nMerging this topic seems to stop all \"win test (n)\" jobs at GitHub\nCI, which is puzzling (I would have understood a broken build, but\nthat is not what we are seeing).\n\nhttps://github.com/git/git/actions/runs/7748054008 is a run of 'next'\nthat is broken.\n\nhttps://github.com/git/git/actions/runs/7748547579 is a run of 'seen~1'\nwith this topic reverted (the ps/reftable-backend topic is excluded),\nwhich seems to pass.\n\nDoes it ring a bell, anybody?\n\nThanks.\n"},{"id":"487849","messageId":"b7b92f1a-9231-2f53-299e-ad58fc699284@gmx.de","threadId":"60805","inReplyTo":"xmqqh6irwtkd.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] some unit-test Makefile polishing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-02-02T23:52:16Z","receivedAt":"2024-02-02T23:52:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 1 Feb 2024, Junio C Hamano wrote:\n\n> https://github.com/git/git/actions/runs/7748054008 is a run of 'next'\n> that is broken.\n>\n> https://github.com/git/git/actions/runs/7748547579 is a run of 'seen~1'\n> with this topic reverted (the ps/reftable-backend topic is excluded),\n> which seems to pass.\n>\n> Does it ring a bell, anybody?\n\nYes, it does ring a clear bell over here.\nhttps://github.com/git/git/actions/runs/7748054008/job/21130098985#step:5:81\npoints to the culprit:\n\n    fatal: not a git repository (or any of the parent directories): .git\n    make: *** [../config.mak.uname:753: vcxproj] Error 128\n\nThe line 753 of that file (as can be seen at\nhttps://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)\nis the first statement of the `vcxproj` target, executing `update-refresh`:\n\n    vcxproj:\n        # Require clean work tree\n        git update-index -q --refresh && \\\n        git diff-files --quiet && \\\n        git diff-index --cached --quiet HEAD --\n        [...]\n\nThis means that `vcxproj` is executed. And the explanation is in\nhttps://github.com/git/git/actions/runs/7748054008/job/21130098985#step:5:78,\nwhich runs `make --quiet -C t 'T=<long-list-of-files>'`, crucially\n_without_ specifying any Makefile rule, and the `vcxproj` rule happens to\nbe the first one that is defined on Windows, so it's used by default.\n\nOne workaround would be to remove the `vcxproj` rule (which by now has\nbeen safely superseded by the CMake support we have in\n`contrib/buildsystems/`).\n\nBut the safer way would be to insert these two lines at the beginning of\n`t/Makefile` (cargo-culted from the top-level `Makefile`):\n\n    # The default target of this Makefile is...\n    all::\n\nCiao,\nJohannes\n"},{"id":"487860","messageId":"xmqqjznmtjr9.fsf@gitster.g","threadId":"60805","inReplyTo":"b7b92f1a-9231-2f53-299e-ad58fc699284@gmx.de","subject":"Re: [PATCH v2 0/3] some unit-test Makefile polishing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-03T01:32:42Z","receivedAt":"2024-02-03T01:32:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> The line 753 of that file (as can be seen at\n> https://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)\n\nOuch.  When it is laid out like this it is very obvious why this is\nbroken, and what its workaround should be.\n\nThanks.  Let's queue this on top.\n\n------- >8 ------------- >8 ------------- >8 ------------- >8 -------\nSubject: [PATCH] t/Makefile: say the default target upfront\n\nSimilar to how 2731d048 (Makefile: say the default target upfront.,\n2005-12-01) added the default target to the very beginning of the\nmain Makefile to prevent a random rule that happens to be defined\nfirst in an included makefile fragments from becoming the default\ntarget, protect this Makefile the same way.\n\nThis started to matter as we started to include config.mak.uname\nand that included makefile fragment does more than defining Make\nmacros, unfortunately.\n\nHelped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/Makefile | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 281f4c3534..2d95046f26 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -1,3 +1,6 @@\n+# The default target of this Makefile is...\n+all::\n+\n # Import tree-wide shared Makefile behavior and libraries\n include ../shared.mak\n \n@@ -52,7 +55,7 @@ UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(UNIT_TEST_PROGRAMS)))\n # scripts not to run the external \"chainlint.pl\" script themselves\n CHAINLINTSUPPRESS = GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT &&\n \n-all: $(DEFAULT_TEST_TARGET)\n+all:: $(DEFAULT_TEST_TARGET)\n \n test: pre-clean check-chainlint $(TEST_LINT)\n \t$(CHAINLINTSUPPRESS) $(MAKE) aggregate-results-and-cleanup\n-- \n2.43.0-522-g2a540e432f\n\n"},{"id":"487882","messageId":"20240204044104.GA295803@coredump.intra.peff.net","threadId":"60805","inReplyTo":"xmqqjznmtjr9.fsf@gitster.g","subject":"Re: [PATCH v2 0/3] some unit-test Makefile polishing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-02-04T04:41:04Z","receivedAt":"2024-02-04T04:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 02, 2024 at 05:32:42PM -0800, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > The line 753 of that file (as can be seen at\n> > https://github.com/git/git/blob/38aa6559b0c513d755d6d5ccf32414ed63754726/config.mak.uname#L753)\n> \n> Ouch.  When it is laid out like this it is very obvious why this is\n> broken, and what its workaround should be.\n> \n> Thanks.  Let's queue this on top.\n> \n> ------- >8 ------------- >8 ------------- >8 ------------- >8 -------\n> Subject: [PATCH] t/Makefile: say the default target upfront\n> \n> Similar to how 2731d048 (Makefile: say the default target upfront.,\n> 2005-12-01) added the default target to the very beginning of the\n> main Makefile to prevent a random rule that happens to be defined\n> first in an included makefile fragments from becoming the default\n> target, protect this Makefile the same way.\n> \n> This started to matter as we started to include config.mak.uname\n> and that included makefile fragment does more than defining Make\n> macros, unfortunately.\n> \n> Helped-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nThanks both of you for identifying and fixing this. I should have been\nable to catch this, as it triggers with a simple \"make\" in the t/\ndirectory (rather than \"make prove\" or \"make unit-test\", which is of\ncourse what I checked).\n\nSorry I'm slow to chime in; I've been offline all week (and probably\nwill be for another few days) due to a family emergency. But hopefully\nwith this fix the topic is OK now.\n\n-Peff\n"}]}