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

[Junio C Hamano <gitster@pobox.com>]

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

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