mbox series

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

Message ID 20240130053714.GA165967@coredump.intra.peff.net (mailing list archive)
Headers show
Series some unit-test Makefile polishing | expand

Message

Jeff King Jan. 30, 2024, 5:37 a.m. UTC 
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

Comments

Junio C Hamano Feb. 2, 2024, 1:20 a.m. UTC | #1 
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.
Johannes Schindelin Feb. 2, 2024, 11:52 p.m. UTC | #2 
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
Junio C Hamano Feb. 3, 2024, 1:32 a.m. UTC | #3 
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
Jeff King Feb. 4, 2024, 4:41 a.m. UTC | #4 
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