Re: [PATCH] fixup??? Merge branch 'ab/ci-updates' into seen

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 23, 2021, 17:58 UTC
Message-ID: <xmqqo86a92jm.fsf@gitster.g>
In-Reply-To: <211123.86ilwjujmd.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 23 quoted lines

> I'm pointing out that your patch to "master" has a logic error where you
> added the scalar tests after that case/esac, but on "master" any new
> "make new-test" needs to be added thusly:
>
>     diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>     index cc62616d806..37df8e2397a 100755
>     --- a/ci/run-build-and-tests.sh
>     +++ b/ci/run-build-and-tests.sh
>     @@ -34,21 +34,28 @@ linux-gcc)
> ...
>      *)
>             make test
>     +       make new-test
>             ;;
>      esac
>     +"and not here, as it would run under pedantic"
>      
>      check_unignored_build_artifacts
>      
> As a result if you look at your own CI run's "pedantic" job at
> https://github.com/gitgitgadget/git/runs/4292915519?check_suite_focus=true
> you'll see that it runs the scalar test, which is not the
> intention. That job should be compile-only job with the -pedantic flag.

I think the above shows that it is a bug in the topic itself, but the presense of the ab/ci-updates topic makes the issue harder to see. It makes the problem manifest in quite a different way. The band-aid we saw from Dscho does "fix" the manifestation after two topics get merged (i.e. scalar build or test must follow the primary build and cannot be done by itself), without correcting the original bug (i.e. scalar test is done in a wrong CI job).

Also, when writing recipes for CI, if you know that scalar build or test must be preceded by primary build, I wonder if it is with more good manners to write

	make test
-	make -C scalar test
+	make && make -C scalar test

to make the dendency clear? In addition, it would be courteous to the fellow developers to make the public entry points like "all" and "test" self contained. The Makefile of scalar knows as well as, or better than, the developers that going up to the top-level of the working tree and running "make" is required before "all" target can be built, so automating that would help everybody from the trouble, and the silly ugliness of "make && make -C there" I suggested above.

I do not, for example, mind at all if something silly like this was done in contrib/scalar/Makefile:

    all: ../../git
    ../../git:
	$(MAKE) -C ../..
    test: all
	...

which is with clearly broken dependencies (e.g. if you edit revision.c, scalar will not be rebuilt or the change would not affect scalar's tests), but for the purpose of "letting CI build and test from scratch to smoke out problems early", it is good enough.

Perhaps Ævar's suggestions were a lot more perfectionist than what pragmatic me would say, and didn't mesh well with the test of Dscho who is even more pragmatic than me? In a separate message, Dscho talks about weeks of delay, but it does not look to me that it is solely Ævar's fault.

We know we hope to be able to make scalar as part of the top-level offering from the project someday, but before that, we should make sure it is as good as it has been advertiesed so far, and by not even being able to easily integrate "correctly" (i.e. in line with the design of the surrounding code) with the CI, we are stumbling at the first step.

Thanks, both.