| Message ID | pull.1081.git.1637578930464.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixup??? Merge branch 'ab/ci-updates' into seen | expand |
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/
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
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/
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.
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)
Æ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.
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