| Message ID | 20240912094730.GC589828@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ce2e99c7d5518b622c3017cd12aa254c853df4f |
| Headers | show |
| Series | make linux32 ci job work with recent actions | expand |
On Thu, Sep 12, 2024 at 05:47:30AM -0400, Jeff King wrote: > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > index 97f9b06310..db8e8f75a4 100644 > --- a/.github/workflows/main.yml > +++ b/.github/workflows/main.yml > @@ -339,8 +339,8 @@ jobs: > image: alpine > distro: alpine-latest > - jobname: linux32 > - image: daald/ubuntu32:xenial > - distro: ubuntu32-16.04 > + image: i386/ubuntu:focal > + distro: ubuntu32-20.04 > - jobname: pedantic > image: fedora > distro: fedora-latest We could counteract the loss of testing against Ubuntu 16.04 by adding it to GitLab CI instead: diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2589098eff7..80b1668ebeb 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -25,6 +25,9 @@ test:linux: fi parallel: matrix: + - jobname: linux-old + image: ubuntu:16.04 + CC: gcc - jobname: linux-sha256 image: ubuntu:latest CC: clang I didn't test it, but it should work alright. GitLab doesn't put any additional executables into the container, so it is entirely self contained. Let me know in case you think this is a good idea and I'll run a CI pipeline against this change. It's not 32 bit, but at least we continue to verify that Git builds against old distros. Patrick
On Thu, Sep 12, 2024 at 12:41:03PM +0200, Patrick Steinhardt wrote: > On Thu, Sep 12, 2024 at 05:47:30AM -0400, Jeff King wrote: > > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > > index 97f9b06310..db8e8f75a4 100644 > > --- a/.github/workflows/main.yml > > +++ b/.github/workflows/main.yml > > @@ -339,8 +339,8 @@ jobs: > > image: alpine > > distro: alpine-latest > > - jobname: linux32 > > - image: daald/ubuntu32:xenial > > - distro: ubuntu32-16.04 > > + image: i386/ubuntu:focal > > + distro: ubuntu32-20.04 > > - jobname: pedantic > > image: fedora > > distro: fedora-latest > > We could counteract the loss of testing against Ubuntu 16.04 by adding > it to GitLab CI instead: > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 2589098eff7..80b1668ebeb 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -25,6 +25,9 @@ test:linux: > fi > parallel: > matrix: > + - jobname: linux-old > + image: ubuntu:16.04 > + CC: gcc > - jobname: linux-sha256 > image: ubuntu:latest > CC: clang > > I didn't test it, but it should work alright. GitLab doesn't put any > additional executables into the container, so it is entirely self > contained. Let me know in case you think this is a good idea and I'll > run a CI pipeline against this change. That seems like a good thing to do to mitigate the loss. In a perfect world we'd have all platforms running all the tests, just because it helps align the work between finding and fixing (i.e., I might introduce a bug and not even know it is failing, and you have to spend time reporting it to me). But the world isn't perfect, so finding out about my bug _eventually_ is OK. :) > It's not 32 bit, but at least we continue to verify that Git builds > against old distros. I think that's OK. AFAICT this was just another case of overloading CI jobs with multiple independent variables (and leaving people to wonder if any failure was because of the 32-bit nature, or because it was old). Having a dedicated "old" job makes that more obvious. -Peff
On Thu, Sep 12, 2024 at 07:22:42AM -0400, Jeff King wrote: > On Thu, Sep 12, 2024 at 12:41:03PM +0200, Patrick Steinhardt wrote: > > > On Thu, Sep 12, 2024 at 05:47:30AM -0400, Jeff King wrote: > > > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > > > index 97f9b06310..db8e8f75a4 100644 > > > --- a/.github/workflows/main.yml > > > +++ b/.github/workflows/main.yml > > > @@ -339,8 +339,8 @@ jobs: > > > image: alpine > > > distro: alpine-latest > > > - jobname: linux32 > > > - image: daald/ubuntu32:xenial > > > - distro: ubuntu32-16.04 > > > + image: i386/ubuntu:focal > > > + distro: ubuntu32-20.04 > > > - jobname: pedantic > > > image: fedora > > > distro: fedora-latest > > > > We could counteract the loss of testing against Ubuntu 16.04 by adding > > it to GitLab CI instead: > > > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > > index 2589098eff7..80b1668ebeb 100644 > > --- a/.gitlab-ci.yml > > +++ b/.gitlab-ci.yml > > @@ -25,6 +25,9 @@ test:linux: > > fi > > parallel: > > matrix: > > + - jobname: linux-old > > + image: ubuntu:16.04 > > + CC: gcc > > - jobname: linux-sha256 > > image: ubuntu:latest > > CC: clang > > > > I didn't test it, but it should work alright. GitLab doesn't put any > > additional executables into the container, so it is entirely self > > contained. Let me know in case you think this is a good idea and I'll > > run a CI pipeline against this change. > > That seems like a good thing to do to mitigate the loss. In a perfect > world we'd have all platforms running all the tests, just because it > helps align the work between finding and fixing (i.e., I might introduce > a bug and not even know it is failing, and you have to spend time > reporting it to me). But the world isn't perfect, so finding out about > my bug _eventually_ is OK. :) For reference, [here] is the merge request. The [first] pipeline failed because Java is too old, as you also mention in one of the preceding commits: JGit Version Exception in thread "main" java.lang.UnsupportedClassVersionError: org/eclipse/jgit/pgm/Main has been compiled by a more recent version of the Java Runtime (class file version 55.0), this version of the Java Runtime only recognizes class file versions up to 52.0 at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:756) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) at java.net.URLClassLoader.defineClass(URLClassLoader.java:468) at java.net.URLClassLoader.access$100(URLClassLoader.java:74) at java.net.URLClassLoader$1.run(URLClassLoader.java:369) at java.net.URLClassLoader$1.run(URLClassLoader.java:363) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:362) at java.lang.ClassLoader.loadClass(ClassLoader.java:418) at org.springframework.boot.loader.LaunchedURLClassLoader.loadClass(LaunchedURLClassLoader.java:151) at java.lang.ClassLoader.loadClass(ClassLoader.java:351) at java.lang.Class.forName0(Native Method) at java.lang.Class.forName(Class.java:348) at org.springframework.boot.loader.MainMethodRunner.run(MainMethodRunner.java:46) at org.springframework.boot.loader.Launcher.launch(Launcher.java:108) at org.springframework.boot.loader.Launcher.launch(Launcher.java:58) at org.springframework.boot.loader.JarLauncher.main(JarLauncher.java:65) I've thus added the following hunk: diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 735ee6f4639..c85b1f55700 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -55,6 +55,11 @@ ubuntu-*|ubuntu32-*) ${CC_PACKAGE:-${CC:-gcc}} $PYTHON_PACKAGE case "$distro" in + ubuntu-16.04) + # Does not support JGit, but we also don't really care about + # the others. We rather care whether Git still compiles and + # runs fine overall. + ;; ubuntu-*) mkdir --parents "$CUSTOM_PATH" And with that the [fixed] pipeline builds and executes our tests just fine. I didn't wait for tests to finish though. Patrick [here]: https://gitlab.com/gitlab-org/git/-/merge_requests/210 [first]: https://gitlab.com/gitlab-org/git/-/jobs/7808775485 [fixed]: https://gitlab.com/gitlab-org/git/-/jobs/7808836999
On Thu, Sep 12, 2024 at 01:53:00PM +0200, Patrick Steinhardt wrote: > On Thu, Sep 12, 2024 at 07:22:42AM -0400, Jeff King wrote: > > On Thu, Sep 12, 2024 at 12:41:03PM +0200, Patrick Steinhardt wrote: > And with that the [fixed] pipeline builds and executes our tests just > fine. I didn't wait for tests to finish though. > > Patrick > > [here]: https://gitlab.com/gitlab-org/git/-/merge_requests/210 > [first]: https://gitlab.com/gitlab-org/git/-/jobs/7808775485 > [fixed]: https://gitlab.com/gitlab-org/git/-/jobs/7808836999 Most of the tests pass, except for t5559. Seems like it doesn't have mod_http2. Maybe its Apache version is too old, or it needs to be installed separately. Patrick
On Thu, Sep 12, 2024 at 02:47:38PM +0200, Patrick Steinhardt wrote: > On Thu, Sep 12, 2024 at 01:53:00PM +0200, Patrick Steinhardt wrote: > > On Thu, Sep 12, 2024 at 07:22:42AM -0400, Jeff King wrote: > > > On Thu, Sep 12, 2024 at 12:41:03PM +0200, Patrick Steinhardt wrote: > > And with that the [fixed] pipeline builds and executes our tests just > > fine. I didn't wait for tests to finish though. > > > > Patrick > > > > [here]: https://gitlab.com/gitlab-org/git/-/merge_requests/210 > > [first]: https://gitlab.com/gitlab-org/git/-/jobs/7808775485 > > [fixed]: https://gitlab.com/gitlab-org/git/-/jobs/7808836999 > > Most of the tests pass, except for t5559. Seems like it doesn't have > mod_http2. Maybe its Apache version is too old, or it needs to be > installed separately. Yeah, according to "apt-file", there's no package which contains mod_http2.so. t5559 is supposed to notice that during webserver setup and just skip the script. Poking at it myself in a xenial container, I think it does do so correctly. So that's good. But the CI environment switches GIT_TEST_HTTPD from "auto" to "true", turning a setup failure into an error. This is overall a good thing (since we'd notice if our apache setup breaks), but obviously is wrong here. Unfortunately we don't have a knob just for http2. So the best we can do is something like (might be whitespace-damaged, I just pasted it out of a container session): diff --git a/ci/lib.sh b/ci/lib.sh index 51f8f59..0514f6a 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -336,7 +336,15 @@ ubuntu-*) fi MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE" - export GIT_TEST_HTTPD=true + case "$distro" in + ubuntu-16.04) + # too old for http/2 + export GIT_TEST_HTTPD=auto + ;; + *) + export GIT_TEST_HTTPD=yes + ;; + esac # The Linux build installs the defined dependency versions below. # The OS X build installs much more recent versions, whichever That would still run the regular tests, and just turn the http2 failure into a "skip". But it does make me nervous that we'd break something for the non-http2 tests on that old platform and never realize it. So maybe we need a GIT_TEST_HTTP2 knob that defaults to the value of GIT_TEST_HTTPD. And then we can turn it off for 16.04, leave the regular one as "yes". I assume you're collecting a few patches to make your new xenial job work. I think what I suggested above should be pretty easy to implement, but let me know if you'd like me to come up with something concrete. -Peff
On Fri, Sep 13, 2024 at 12:55:10AM -0400, Jeff King wrote: > On Thu, Sep 12, 2024 at 02:47:38PM +0200, Patrick Steinhardt wrote: > > > On Thu, Sep 12, 2024 at 01:53:00PM +0200, Patrick Steinhardt wrote: > > > On Thu, Sep 12, 2024 at 07:22:42AM -0400, Jeff King wrote: > > > > On Thu, Sep 12, 2024 at 12:41:03PM +0200, Patrick Steinhardt wrote: > > > And with that the [fixed] pipeline builds and executes our tests just > > > fine. I didn't wait for tests to finish though. > > > > > > Patrick > > > > > > [here]: https://gitlab.com/gitlab-org/git/-/merge_requests/210 > > > [first]: https://gitlab.com/gitlab-org/git/-/jobs/7808775485 > > > [fixed]: https://gitlab.com/gitlab-org/git/-/jobs/7808836999 > > > > Most of the tests pass, except for t5559. Seems like it doesn't have > > mod_http2. Maybe its Apache version is too old, or it needs to be > > installed separately. > > Yeah, according to "apt-file", there's no package which contains > mod_http2.so. t5559 is supposed to notice that during webserver setup > and just skip the script. Poking at it myself in a xenial container, I > think it does do so correctly. So that's good. > > But the CI environment switches GIT_TEST_HTTPD from "auto" to "true", > turning a setup failure into an error. This is overall a good thing > (since we'd notice if our apache setup breaks), but obviously is wrong > here. Unfortunately we don't have a knob just for http2. So the best we > can do is something like (might be whitespace-damaged, I just pasted it > out of a container session): > > diff --git a/ci/lib.sh b/ci/lib.sh > index 51f8f59..0514f6a 100755 > --- a/ci/lib.sh > +++ b/ci/lib.sh > @@ -336,7 +336,15 @@ ubuntu-*) > fi > MAKEFLAGS="$MAKEFLAGS PYTHON_PATH=/usr/bin/$PYTHON_PACKAGE" > > - export GIT_TEST_HTTPD=true > + case "$distro" in > + ubuntu-16.04) > + # too old for http/2 > + export GIT_TEST_HTTPD=auto > + ;; > + *) > + export GIT_TEST_HTTPD=yes > + ;; > + esac > > # The Linux build installs the defined dependency versions below. > # The OS X build installs much more recent versions, whichever > > > That would still run the regular tests, and just turn the http2 failure > into a "skip". But it does make me nervous that we'd break something for > the non-http2 tests on that old platform and never realize it. So maybe > we need a GIT_TEST_HTTP2 knob that defaults to the value of > GIT_TEST_HTTPD. And then we can turn it off for 16.04, leave the regular > one as "yes". > > I assume you're collecting a few patches to make your new xenial job > work. I think what I suggested above should be pretty easy to implement, > but let me know if you'd like me to come up with something concrete. Yeah, that does the job, thanks. Let me tie all of this into a neat package and post it as 5/4 on top of this patch series. Patrick
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 97f9b06310..db8e8f75a4 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -339,8 +339,8 @@ jobs: image: alpine distro: alpine-latest - jobname: linux32 - image: daald/ubuntu32:xenial - distro: ubuntu32-16.04 + image: i386/ubuntu:focal + distro: ubuntu32-20.04 - jobname: pedantic image: fedora distro: fedora-latest diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh index 00cb7df67a..735ee6f463 100755 --- a/ci/install-dependencies.sh +++ b/ci/install-dependencies.sh @@ -37,9 +37,18 @@ ubuntu-*|ubuntu32-*) # Required so that apt doesn't wait for user input on certain packages. export DEBIAN_FRONTEND=noninteractive + case "$distro" in + ubuntu-*) + SVN='libsvn-perl subversion' + ;; + *) + SVN= + ;; + esac + sudo apt-get -q update sudo apt-get -q -y install \ - language-pack-is libsvn-perl apache2 cvs cvsps git gnupg subversion \ + language-pack-is apache2 cvs cvsps git gnupg $SVN \ make libssl-dev libcurl4-openssl-dev libexpat-dev wget sudo default-jre \ tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl \ libemail-valid-perl libio-pty-perl libio-socket-ssl-perl libnet-smtp-ssl-perl libdbd-sqlite3-perl libcgi-pm-perl \
The Xenial image we're using was released more than 8 years ago. This is a problem for using some recent GitHub Actions scripts, as they require Node.js 20, and all of the binaries they ship need glibc 2.28 or later. We're not using them yet, but moving forward prepares us for a future patch which will. Xenial was actually the last official 32-bit Ubuntu release, but you can still find i386 images for more recent releases. This patch uses Focal, which was released in 2020 (and is the oldest one with glibc 2.28). There are two small downsides here: - while Xenial is pretty old, it is still in LTS support until April 2026. So there's probably some value in testing with such an old system, and we're losing that. - there are no i386 subversion packages in the Focal repository. So we won't be able to test that (OTOH, we had never tested it until the previous patch which unified the 32/64-bit dependency code). Signed-off-by: Jeff King <peff@peff.net> --- .github/workflows/main.yml | 4 ++-- ci/install-dependencies.sh | 11 ++++++++++- 2 files changed, 12 insertions(+), 3 deletions(-)