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

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The merge of the `ab/ci-updates` patches needs this fix-up to avoid
breaking the Scalar tests.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    fixup??? Merge branch 'ab/ci-updates' into seen
    
    This fixes the CI breakage introduced with ab/ci-updates. It is based on
    the current version of seen: 89513b853be (Merge branch 'en/keep-cwd'
    into seen, 2021-11-21).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1081%2Fdscho%2Ffix-ci-updates-merge-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1081/dscho/fix-ci-updates-merge-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1081

 ci/run-build-and-tests.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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
 

base-commit: 89513b853be400c439e19e479ed14e9e8de44722
-- 
gitgitgadget

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

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 22 2021, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The merge of the `ab/ci-updates` patches needs this fix-up to avoid
> breaking the Scalar tests.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     fixup??? Merge branch 'ab/ci-updates' into seen
>     
>     This fixes the CI breakage introduced with ab/ci-updates. It is based on
>     the current version of seen: 89513b853be (Merge branch 'en/keep-cwd'
>     into seen, 2021-11-21).
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1081%2Fdscho%2Ffix-ci-updates-merge-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1081/dscho/fix-ci-updates-merge-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1081
>
>  ci/run-build-and-tests.sh | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> 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 is that on js/scalar this won't work:

    make clean
    make -C contrib/scalar test

Since its dependency graph is broken. It tries to run before "make all"
has been run, but fails.

It would be one thing if it just refused to run because the sources
haven't been compiled, but instead it tries in a way that suggests
that's meant to work, but it doesn't work.

I didn't spot this myself because I was testing on my locally patched
version of the scalar topic which doesn't have this breakage, which is a
patch you haven't reviwed/looked at/replied to yet, as noted in [1].

But there's also an existing breakage in js/scalar and this fix-up,
which is that yes, CI passes with this change. But what are meant to be
compile-only CI targets which don't run "make test" are now running no
tests ... except for this one scalar test.

Fixing that issue both in js/scalar & in this fix-up would be rather
easy, but it's all just a work-arond for the discrepancy that "make
test" isn't running the scalar tests by default.

If we fixed that more general problem this fix-up wouldn't be needed.

But at this point I'm just starting to repeat points I've made in
patches[2] I've sent you on top of js/scalar, and which you seem to be
ignoring.

1. https://lore.kernel.org/git/211118.86zgq14jp1.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

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.

Will fix,
Dscho

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

[Ævar Arnfjörð Bjarmason]

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/

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

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.

Ciao,
Johannes

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.

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

[Ævar Arnfjörð Bjarmason]

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)

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

[Junio C Hamano]

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