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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 22 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> 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?

What I'm pointing out is that this isn't an issue that happened because
of the merger with ab/ci-updates, but merely turned into a CI failure
because of it.

Before that in your initial patch to integrate it into CI[1] the
relevant part of the patch was, with extra context added by me:

    [...]
       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 test
               ;;
       esac
       make test
       ;;
     esac
    +make -C contrib/scalar test
     
     check_unignored_build_artifacts

I.e. it added scalar testing to the linux-gcc-4.8 & "pedantic" jobs,
which are meant to be compile-only.

The other issue is that the "test" Makefile target in
contrib/scalar/Makefile attempts to build the top-level from scratch,
but fails (which is how it turned into a CI failure). The same thing
happens when running it outside fo CI.

I don't think I've been demanding that you do anything. I have been
asking if there's a good reason for why we wouldn't test this code we've
got in-tree. Your commit[1] states:

    Since Scalar depends on `libgit.a`, it makes sense to ensure in the CI
    and the PR builds that it does not get broken in case of industrious
    refactorings of the core Git code.

Which is rationale that I entirely agree with, the only extent to which
I don't is that I don't think it goes far enough, i.e. shouldn't we also
add this to "make test" and not just CI? Why shouldn't I see failures
locally, and only when I push to CI (unless I go out of my way to run
the tests-like-CI-would)?

In any case, both of these breakages are present in your version of the
version of the patches, but not the change I've been proposing on-top to
add it to CI and "make test". You've also refused to talk about why you
insist on that particular approach, which is shown to be more fragile
here. So it seems rather odd to blame my suggestions (or "demands") for
them.

1. https://lore.kernel.org/git/1b0328fa236a35c2427b82f53c32944e513580d3.1637158762.git.gitgitgadget@gmail.com/