From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-11-22 11:02:14
From: Johannes Schindelin <redacted>
The merge of the `ab/ci-updates` patches needs this fix-up to avoid
breaking the Scalar tests.
Signed-off-by: Johannes Schindelin <redacted>
---
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(-)
On Mon, Nov 22 2021, Johannes Schindelin via GitGitGadget wrote:
quoted hunk
From: Johannes Schindelin <redacted>
The merge of the `ab/ci-updates` patches needs this fix-up to avoid
breaking the Scalar tests.
Signed-off-by: Johannes Schindelin <redacted>
---
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(-)
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/
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/
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.
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)