| Message ID | 20240129031933.GB2433899@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | some unit-test Makefile polishing | expand |
On Sun, Jan 28, 2024 at 10: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
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
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.
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
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
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
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
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.
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
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(-)