On Tue, Nov 23 2021, Johannes Schindelin wrote:
> Hi Ævar,
>
> On Tue, 23 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 22 2021, Johannes Schindelin wrote:
>>
>> > On Mon, 22 Nov 2021, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Mon, Nov 22 2021, Johannes Schindelin via GitGitGadget wrote:
>> >>
>> >> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> >> > index c0bae709b3b..c508c18ad44 100755
>> >> > --- a/ci/run-build-and-tests.sh
>> >> > +++ b/ci/run-build-and-tests.sh
>> >> > @@ -45,9 +45,8 @@ linux-gcc-4.8)
>> >> > export MAKE_TARGETS=all
>> >> > ;;
>> >> > esac
>> >> > -make -C contrib/scalar test
>> >> > -
>> >> > make $MAKE_TARGETS
>> >> > +make -C contrib/scalar test
>> >> >
>> >> > check_unignored_build_artifacts
>> >>
>> >> The CI breakage was introduced with the merger with ab/ci-updates, but
>> >> the combination of the two just reveals an existing breakage in
>> >> js/scalar.
>> >
>> > Which shows that I was wrong to pander to your repeated demand to include
>> > Scalar in the CI builds already at this early stage.
>>
>> Us finding an a bug in a topic that's happening outside of CI means we
>> shouldn't have added it to CI in the first place? Isn't spotting these
>> issues a good thing?
>
> Let's analyze "these issues".
>
> Before your patch series, Scalar's `make -C contrib/scalar test` came
> after the `make test` which ensured that Git was built. As designed.
>
> After merging your patch series, the `make test` was magically moved
> _after_ `make -C contrib/scalar test` (which is wrong for more reasons
> than just that Git was not built yet).
>
> So the "issue" is a simple mis-merge, and I provided a fix.
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)
export GIT_TEST_WRITE_REV_INDEX=1
export GIT_TEST_CHECKOUT_WORKERS=2
make test
+ make new-test
;;
linux-clang)
export GIT_TEST_DEFAULT_HASH=sha1
make test
+ make new-test
export GIT_TEST_DEFAULT_HASH=sha256
make test
+ make new-test
;;
linux-gcc-4.8|pedantic)
# Don't run the tests; we only care about whether Git can be
# built with GCC 4.8 or with pedantic
+ "make new test does not go here"
;;
*)
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 also notice now that it doesn't behave the same way with the
sha1/sha256 test. We do end up testing both, but it's not tested like
the rest (but in any case that job is split up in my CI topic).
The other issue is that even if you put a "make test" before a "make"
etc. it should still work, as "test" knows to depend on the compilation
it needs for "test", but that's not the case with "make -C
contrib/scalar test".
Shouldn't it either know how to build git from scratch, or better yet
behave like "make -C t" which doesn't, and just punts out entirely? The
in-between seems strange.
In any case, I see js/scalar got ejected out of "seen" with Junio's last
push-out. I think that any re-roll based on "master" should be adding
the "make new-test" like the above diff. At that point we'll only have a
textual conflict, not a semantic one.
> P.S.: Of course, this could have been easily avoided by holding off
> patches that intentionally touch the very same code as other patch series
> that are already in flight. This kind of conflict seems to happen more
> often than usual as of late. It happened with the FSMonitor patches and
> repo-settings, with the hooks patches, the pager patch yesterday, etc.
Sure, without going into the details of some of those (which I think
you've got the wrong impression of) it's also the responsibility of us
to try to avoid these conflicts when possible.
You've picked an approach with js/scalar that'll require changing every
place where we invoke "make test" in CI to also invoke a "make -C
contrib/scalar test", instead of just adding it to "make test"
itself. The latter is clearly possible, I've sent you a patch to do it
that way, and it works.
So why can't we discuss why that more invasive and textually conflicting
approach is necessary? The scalar topic is clearly a much bigger bite
for git.git than any relatively recent and tiny ci/ topic of mine, and
will be longer in-flight. So anything that reduces such conflicts would
be a good thing.
My current diffstat on top of your "vanilla" topic showing no
special-casing in .github/*, ci/* etc. is needed with that approach, and
thus any potential for conflicts there doesn't exist anymore[1]:
.github/workflows/main.yml | 15 -----
.gitignore | 1 +
Documentation/Makefile | 4 ++
Documentation/cmd-list.perl | 2 +-
Documentation/git.txt | 12 ++++
{contrib/scalar => Documentation}/scalar.txt | 4 +-
Makefile | 96 +++++++++++++++++++---------
ci/run-build-and-tests.sh | 1 -
ci/run-test-slice.sh | 5 --
command-list.txt | 2 +
contrib/scalar/.gitignore | 2 -
contrib/scalar/Makefile | 45 -------------
contrib/scalar/t/Makefile | 78 ----------------------
contrib/scalar/scalar.c => scalar.c | 0
{contrib/scalar/t => t}/t9099-scalar.sh | 42 +++++++-----
15 files changed, 115 insertions(+), 194 deletions(-)
1. http://github.com/avar/git/commit/6df9fc2812d (commit message not
entirely up-to-date)
