| Message ID | 20220415123922.30926-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ci: lock "pedantic" job into fedora 35 and other cleanup | expand |
On Fri, Apr 15 2022, Carlo Marcelo Arenas Belón wrote: > Fedora 36 is scheduled to be released in Apr 19th, but it includes > a prerelease of gcc 12 that has known issues[1] with our codebase > and therefore requires extra changes to not break a DEVELOPER=1 > build. > > Lock the CI job to the current release image, and while at it rename > the job to better reflect what it is currently doing, instead of its > original objective. The CI job was added in your cebead1ebfb (ci: run a pedantic build as part of the GitHub workflow, 2021-08-08), and later in 6a8cbc41bac (developer: enable pedantic by default, 2021-09-03) we made "pedantic" the default. Is there any point in having this job at all anymore, or is it just a "does it compile on Fedora?" now? Your cebead1ebfb notes that its gcc is ahead of the curve, if that's what we actually want perhaps s/fedora/linux-newer-gcc/ ? I think this change would be much clearer if we first delete the now-redundant pedantic flag, and then later do whatever else that's needed... > [...] > This merges fine to master, maint and next but will need some work to > get into seen. > > Alternatively, the fixes to fix the build could be merged instead, but > it will still require at least one temporary change to disable a flag > as the underlying bug[3] is yet to be addressed in gcc (or somewhere > else in Fedora). I get the same thing on GCC12 on Debian. Wouldn't a much more useful thing be to upgrade to gcc12 anyway, and just set -Wno-error=stringop-overread on gcc12 for dir.(o|s|sp)? Then we'd find any future issues, and blacklist this one known issue... Or just set -O1 under the same condition. > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh > index 78b7e326da6..660e25d1d26 100755 > --- a/ci/install-docker-dependencies.sh > +++ b/ci/install-docker-dependencies.sh > @@ -15,8 +15,8 @@ linux-musl) > apk add --update build-base curl-dev openssl-dev expat-dev gettext \ > pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null > ;; > -pedantic) > +fedora) > dnf -yq update >/dev/null && > - dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null > + dnf -yq install make gcc git findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null Why do we need to install "git" now, I'd think because we're upgrading, but here we're pinning the old version, what changed? > ;; > esac > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 280dda7d285..de0f8d36d7c 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -37,10 +37,9 @@ linux-clang) > linux-sha256) > export GIT_TEST_DEFAULT_HASH=sha256 > ;; > -pedantic) > +fedora) > # Don't run the tests; we only care about whether Git can be > # built. > - export DEVOPTS=pedantic > export MAKE_TARGETS=all > ;; > esac
Hi Carlo On 15/04/2022 13:39, Carlo Marcelo Arenas Belón wrote: > Fedora 36 is scheduled to be released in Apr 19th, but it includes > a prerelease of gcc 12 that has known issues[1] I was confused as that thread seems to be about errors when compiling with -O3 rather than -O2 but your fedora bug report[1] indicates that there are in fact problems when compiling with -O2 which will affect our ci. > with our codebase > and therefore requires extra changes to not break a DEVELOPER=1 > build. > > Lock the CI job to the current release image, and while at it rename > the job to better reflect what it is currently doing, instead of its > original objective. > > Finally add git which was a known[2] issue for a while. It would be useful to explain what the issue is, that email also mentions running the tests under this job but we're not doing that here. Thanks for fixing this before it breaks everyone's ci runs Phillip [1] https://bugzilla.redhat.com/show_bug.cgi?id=2075786 > [1] https://lore.kernel.org/git/YZQhLh2BU5Hquhpo@coredump.intra.peff.net/ > [2] https://lore.kernel.org/git/xmqqeeb1dumx.fsf@gitster.g/ > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > This merges fine to master, maint and next but will need some work to > get into seen. > > Alternatively, the fixes to fix the build could be merged instead, but > it will still require at least one temporary change to disable a flag > as the underlying bug[3] is yet to be addressed in gcc (or somewhere > else in Fedora). > > [3] https://bugzilla.redhat.com/show_bug.cgi?id=2075786 > > .github/workflows/main.yml | 4 ++-- > ci/install-docker-dependencies.sh | 4 ++-- > ci/run-build-and-tests.sh | 3 +-- > 3 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > index c35200defb9..48e212f4110 100644 > --- a/.github/workflows/main.yml > +++ b/.github/workflows/main.yml > @@ -282,8 +282,8 @@ jobs: > - jobname: linux32 > os: ubuntu32 > image: daald/ubuntu32:xenial > - - jobname: pedantic > - image: fedora > + - jobname: fedora > + image: fedora:35 > env: > jobname: ${{matrix.vector.jobname}} > runs-on: ubuntu-latest > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh > index 78b7e326da6..660e25d1d26 100755 > --- a/ci/install-docker-dependencies.sh > +++ b/ci/install-docker-dependencies.sh > @@ -15,8 +15,8 @@ linux-musl) > apk add --update build-base curl-dev openssl-dev expat-dev gettext \ > pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null > ;; > -pedantic) > +fedora) > dnf -yq update >/dev/null && > - dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null > + dnf -yq install make gcc git findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null > ;; > esac > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 280dda7d285..de0f8d36d7c 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -37,10 +37,9 @@ linux-clang) > linux-sha256) > export GIT_TEST_DEFAULT_HASH=sha256 > ;; > -pedantic) > +fedora) > # Don't run the tests; we only care about whether Git can be > # built. > - export DEVOPTS=pedantic > export MAKE_TARGETS=all > ;; > esac
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > Fedora 36 is scheduled to be released in Apr 19th, but it includes > a prerelease of gcc 12 that has known issues[1] with our codebase > and therefore requires extra changes to not break a DEVELOPER=1 > build. > > Lock the CI job to the current release image, and while at it rename > the job to better reflect what it is currently doing, instead of its > original objective. Please spell out what "original objective" is, why we are changing the purpose of the target, and how such a change is justifiable. I've always thought that the original objective was to try to see if we still compile with the "-pedantic" option on. Is it now "we want to make sure a recent but not latest version of Fedora is OK"? Why do we want to do so? Please write your log message with an explicit focus to help the future developers decide, when they want to bump the version from 35 to say 40 in the future, if it does or does not violate the spirit of this change. > Finally add git which was a known[2] issue for a while. Instead of referring to an external message, spell it out, it is not all that long. Without working "git" installed, "save_good_tree" step in the CI were not running correctly at all. This was originally noticed in [2]; take the fix suggested there. All in all, each of these three independent and unrelated changes may individually be a good thing to do on its own (or may not be), i.e. * Avoid fedora 36 and later at least for now * Do not build with pedantic any more * Install "git" to help "save_good_tree" step in the CI but they are unrelated changes that deserves their own justification. > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > index c35200defb9..48e212f4110 100644 > --- a/.github/workflows/main.yml > +++ b/.github/workflows/main.yml > @@ -282,8 +282,8 @@ jobs: > - jobname: linux32 > os: ubuntu32 > image: daald/ubuntu32:xenial > - - jobname: pedantic > - image: fedora > + - jobname: fedora > + image: fedora:35 > env: > jobname: ${{matrix.vector.jobname}} > runs-on: ubuntu-latest > diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh > index 78b7e326da6..660e25d1d26 100755 > --- a/ci/install-docker-dependencies.sh > +++ b/ci/install-docker-dependencies.sh > @@ -15,8 +15,8 @@ linux-musl) > apk add --update build-base curl-dev openssl-dev expat-dev gettext \ > pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null > ;; > -pedantic) > +fedora) > dnf -yq update >/dev/null && > - dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null > + dnf -yq install make gcc git findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null > ;; > esac > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh > index 280dda7d285..de0f8d36d7c 100755 > --- a/ci/run-build-and-tests.sh > +++ b/ci/run-build-and-tests.sh > @@ -37,10 +37,9 @@ linux-clang) > linux-sha256) > export GIT_TEST_DEFAULT_HASH=sha256 > ;; > -pedantic) > +fedora) > # Don't run the tests; we only care about whether Git can be > # built. > - export DEVOPTS=pedantic > export MAKE_TARGETS=all > ;; > esac
On Fri, Apr 15, 2022 at 11:31 AM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > Fedora 36 is scheduled to be released in Apr 19th, but it includes > > a prerelease of gcc 12 that has known issues[1] with our codebase > > and therefore requires extra changes to not break a DEVELOPER=1 > > build. > > > > Lock the CI job to the current release image, and while at it rename > > the job to better reflect what it is currently doing, instead of its > > original objective. > > Please spell out what "original objective" is, why we are changing > the purpose of the target, and how such a change is justifiable. Will do in a reroll with the objective of "let's make the pedantic job more useful" but that is IMHO less of a priority and orthogonal to the objective of this series which was "let's avoid breaking our CI because of fedora's gcc updates". Original I had that done on top of ab/http-gcc-12-workaround (which fixes a real, albeit likely harmless bug that compiler was surfacing, and that will also break our CI in a few days) but after the feedback from this thread, think might be preferred and also less likely to conflict with current seen. Would that be reasonable, eventhough it is so late in the RC cycle? Carlo
Carlo Arenas <carenas@gmail.com> writes:
> Would that be reasonable, eventhough it is so late in the RC cycle?
I do not think we will take anything like to the release next week;
it is way way too late for that. But if it can preview sooner in
'seen' to help review by others, that would still be good thing to
aim for, I would think.
Thanks.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c35200defb9..48e212f4110 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -282,8 +282,8 @@ jobs: - jobname: linux32 os: ubuntu32 image: daald/ubuntu32:xenial - - jobname: pedantic - image: fedora + - jobname: fedora + image: fedora:35 env: jobname: ${{matrix.vector.jobname}} runs-on: ubuntu-latest diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh index 78b7e326da6..660e25d1d26 100755 --- a/ci/install-docker-dependencies.sh +++ b/ci/install-docker-dependencies.sh @@ -15,8 +15,8 @@ linux-musl) apk add --update build-base curl-dev openssl-dev expat-dev gettext \ pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null ;; -pedantic) +fedora) dnf -yq update >/dev/null && - dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null + dnf -yq install make gcc git findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null ;; esac diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh index 280dda7d285..de0f8d36d7c 100755 --- a/ci/run-build-and-tests.sh +++ b/ci/run-build-and-tests.sh @@ -37,10 +37,9 @@ linux-clang) linux-sha256) export GIT_TEST_DEFAULT_HASH=sha256 ;; -pedantic) +fedora) # Don't run the tests; we only care about whether Git can be # built. - export DEVOPTS=pedantic export MAKE_TARGETS=all ;; esac
Fedora 36 is scheduled to be released in Apr 19th, but it includes a prerelease of gcc 12 that has known issues[1] with our codebase and therefore requires extra changes to not break a DEVELOPER=1 build. Lock the CI job to the current release image, and while at it rename the job to better reflect what it is currently doing, instead of its original objective. Finally add git which was a known[2] issue for a while. [1] https://lore.kernel.org/git/YZQhLh2BU5Hquhpo@coredump.intra.peff.net/ [2] https://lore.kernel.org/git/xmqqeeb1dumx.fsf@gitster.g/ Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- This merges fine to master, maint and next but will need some work to get into seen. Alternatively, the fixes to fix the build could be merged instead, but it will still require at least one temporary change to disable a flag as the underlying bug[3] is yet to be addressed in gcc (or somewhere else in Fedora). [3] https://bugzilla.redhat.com/show_bug.cgi?id=2075786 .github/workflows/main.yml | 4 ++-- ci/install-docker-dependencies.sh | 4 ++-- ci/run-build-and-tests.sh | 3 +-- 3 files changed, 5 insertions(+), 6 deletions(-)