patchci: remove 'Upload failed tests' directories' step from linux32 jobs

Linux32 jobs seem to be getting:

    Error: This request has been automatically failed because it uses a
    deprecated version of `actions/upload-artifact: v1`. Learn more:
    https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/

before doing anything useful.  For now, disable the step.

Ever since actions/upload-artifact@v1 got disabled, mentioning the offending version of it seems to stop anything from happening. At least this should run the same build and test.

See

    https://github.com/git/git/actions/runs/10780030750/job/29894867249

for example.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 .github/workflows/main.yml | 6 ------
 1 file changed, 6 deletions(-)

Junio C Hamano <gitster@pobox.com> writes:

I refrain from merging my own patches until somebody else at least comments on them, but I'll make an exception and merge this to 'next', as a few "container" jobs _always_ fail to start otherwise. At least with the step removed, a run without any test failures would tell us that these container tasks are OK, which is much better.

If somebody finds a better solution (i.e., a way to extract trash directories of failed tests', with actions/upload-artifact@v1), that can be added later on top.

On Wed, Sep 11, 2024 at 03:32:46PM -0700, Junio C Hamano wrote:

I looked at this a bit last night, but as I didn't come up with any useful solution, I refrained from replying. I was going to do a brief write-up of all of my dead ends, but after banging my head against the wall a bit more, I think I might actually have something. ;)

I think a better path forward is to figure out how to keep up to date with the upload-artifact action for all jobs. The root of the issue is that all of the runner images are 64-bit, and the actions aren't prepared to run inside a 32-bit container. They're written in javascript and ship with their own node.js, but it doesn't work inside the container due to dynamic linking issues.

So you probably saw a message like this:

  exec /__e/node20/bin/node: no such file or directory

in the job log. That funky path is the node binary that ships with the action, and the ENOENT is because it can't find the runtime linker.

  Aside: I think the world would be a better place if they shipped a
  totally static build of node, but there may be some complications
  there. I found some discussion here:

    https://github.com/actions/setup-node/issues/922
    https://github.com/actions/runner/issues/2906

There are two options here, I think:

  1. Instead of running everything inside the container, we can run the
     actions outside (on the normal 64-bit runner image), and just do
     the build/test steps inside the container by manually running
     docker with the appropriate mounts.

     This was the suggested here:

       https://github.com/actions/runner/issues/1491#issuecomment-970418408

     and apparently is how libgit2 works (I don't think it's too hard to
     do so, but their infrastructure wasn't totally trivial).

  2. After some tricky debugging via tmate[1], I found that we can
     install the necessary libc bits within the container. But there's
     another catch! They've moved to node20, which requires glibc2.28,
     and the eight-year-old xenial release we're using is too old for
     that. We have to move to Focal, which was released in 2020.

It feels like (1) is probably the more robust and flexible solution overall. But we can get to (2) a lot more easily from where we are now.

Doing this:

On Thu, Sep 12, 2024 at 03:56:32AM -0400, Jeff King wrote:

So obviously that other email ended up long and full of details. What I was originally going to say was: "OK, losing artifact uploads for linux32 is not the end of the world, and it is important to unblock CI now."

But I _think_ all of those details I sent add up to a possible patch series. But it probably still makes sense to take your patch here, as it unblocks CI in the short term. And then I can build the bigger fix on top of that (it touches the same spot, but in the end both end up removing that special case).

-Peff

On Thu, Sep 12, 2024 at 04:00:11AM -0400, Jeff King wrote:

OK, here's what I came up with. I built it on top of what you have queued in jc/ci-upload-artifact-and-linux32, but it could be applied separately (and it should merge OK).

The state after each patch should still pass, but of course each of them depends on the ones before.

The big downside is discussed in patch 3: we are losing what is probably the oldest platform we test against. I don't think it's fixable in this approach, though; the Actions node just won't run on such an old platform. So the options are:

  - hope that they eventually move to a true static node binary

  - switch the design entirely; handle docker ourselves, run the actions
    in the regular runner-image environment, and then just do the "make
    test" bits inside the container

IMHO what's in this series is an OK tradeoff (we are going from an 8-year-old image to a 4-year-old one, but it gets older with each passing year). If somebody wants to pursue the alternate design above, be my guest. I'm just trying not to fall into that rabbit hole myself. ;)

  [1/4]: ci: drop run-docker scripts
  [2/4]: ci: unify ubuntu and ubuntu32 dependencies
  [3/4]: ci: use more recent linux32 image
  [4/4]: ci: use regular action versions for linux32 job

 .github/workflows/main.yml | 12 +++----
 ci/install-dependencies.sh | 45 +++++++++++++++-----------
 ci/run-docker-build.sh     | 66 --------------------------------------
 ci/run-docker.sh           | 47 ---------------------------
 4 files changed, 32 insertions(+), 138 deletions(-)
 delete mode 100755 ci/run-docker-build.sh
 delete mode 100755 ci/run-docker.sh

-Peff

We haven't used these scripts since 4a6e4b9602 (CI: remove Travis CI support, 2021-11-23), as the GitHub Actions config has support for directly running jobs within docker containers.

It's possible we might want to resurrect something like this in order to be more agnostic to the CI platform. But it's not clear exactly what it would look like. And in the meantime, it's just a maintenance burden as we make changes to CI config, and is subject to bitrot. In fact it's already broken; it references ci/install-docker-dependencies.sh, which went away in 9cdeb34b96 (ci: merge scripts which install dependencies, 2024-04-12).

Signed-off-by: Jeff King <peff@peff.net>
---
I think this is worth doing even if we don't take the rest of the
patches. But obviously it gets even more bit-rotted as the other patches
change the CI config file.

 ci/run-docker-build.sh | 66 ------------------------------------------
 ci/run-docker.sh       | 47 ------------------------------
 2 files changed, 113 deletions(-)
 delete mode 100755 ci/run-docker-build.sh
 delete mode 100755 ci/run-docker.sh

On Thu, Sep 12, 2024 at 05:43:36AM -0400, Jeff King wrote:

I initially wanted to use them for GitLab CI, but eventually I figured that it makes way more sense to just merge the setup instructions we have into a single, unified script. Docker or not doesn't really matter all that much when you want to install dependencies. What matters more is the actual platform you're on, but that is independent of Docker anyway.

The only remaining usecase I could see for these is to run CI-like builds on a developer's machine. But the fact that these scripts don't work at all anymore and have started to bitrot already demonstrates that nobody does seem to do that in the first place.

So I think removing them is the right thing to do.

Patrick

The script to install dependencies has two separate entries for 32-bit and 64-bit Ubuntu systems. This increases the maintenance burden since both should need roughly the same packages.

That hasn't been too bad so far because we've stayed on the same 32-bit image since 2017. Trying to move to a newer image revealed several problems with the linux32 job:

  - newer images complain about using "linux32 --32bit i386", due to
    seccomp restrictions. We can loosen these with a docker option, but
    I don't think running it is even doing anything. We use it only for
    pretending to "apt" that we're on a 32-bit machine, but inside the
    container image apt is already configured as a 32-bit system (even
    though the kernel outside the container is obviously 64-bit).  Using
    the same apt invocation for both architectures just gets rid of this
    call entirely.

  - we set DEBIAN_FRONTEND to avoid hanging on packages that ask the
    user questions. This wasn't a problem on the old image, but it is on
    newer ones. The 64-bit stanza handles this already.

    As a bonus, the 64-bit stanza uses "apt -q" instead of redirecting
    output to /dev/null. This would have saved me a lot of debugging
    time trying to figure out why it was hanging. :)

  - the old image seems to have zlib-dev installed by default, but newer
    ones do not.

In addition, there were probably many tests being skipped on the 32-bit build because we didn't have support packages installed (e.g., gpg). Now we'll run them.

We do need to keep some parts split off just for 64-bit systems: our p4 and lfs installs reference x86_64/amd64 binaries. The downloaded jgit should work in theory, since it's just a jar file embedded in a shell script that relies on the system java. But the system java in our image is too old, so I've left it as 64-bit only for now.

Signed-off-by: Jeff King <peff@peff.net>
---
Patch is easier to read with "-w".

 ci/install-dependencies.sh | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

On Thu, Sep 12, 2024 at 05:45:37AM -0400, Jeff King wrote:

Okay. All of the build dependencies retrieved via apt are the same across two platforms now, but the dependencies we fetch and install manually are 64-bit specific. So overall we get more coverage with less duplication. Nice.

Patrick

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(-)

On Thu, Sep 12, 2024 at 05:47:30AM -0400, Jeff King wrote:

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:

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. :)

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:

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:

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:

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):

On Fri, Sep 13, 2024 at 12:55:10AM -0400, Jeff King wrote:

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

The linux32 job runs inside a docker container with a 32-bit libc, etc. This breaks any GitHub Actions scripts that are implemented in javascript, because they ship with their own 64-bit version of Node.js that's dynamically linked. They'll fail with a message like:

    exec /__e/node20/bin/node: no such file or directory

because they can't find the runtime linker.

This hasn't been a problem until recently because we special-case older, non-javascript versions of these actions for the linux32 job. But it recently became an issue when our old version of actions/upload-artifact was deprecated, causing the job to fail. We worked around that in 90f2c7240c (ci: remove 'Upload failed tests' directories' step from linux32 jobs, 2024-09-09), but it meant a loss of functionality for that job. And we may eventually run into the same deprecation problem with actions/checkout, which can't just be removed.

We can solve the linking issue by installing the 64-bit libc and stdc++ packages before doing anything else. Coupled with the switch to a more recent image in the previous patch, that lets us remove the special-casing of the action scripts entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
 .github/workflows/main.yml | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

Will queue.

Let's target to merge this down to 'maint'; sooner the better.

Thanks.

In the preceding commits we had to convert the linux32 job to be based on Ubuntu 20.04 instead of Ubuntu 16.04 due to a limitation in GitHub Workflows. This was the only job left that still tested against this old but supported Ubuntu version, and we have no other jobs that test with a comparatively old Linux distribution.

Add a new job to GitLab CI that tests with Ubuntu 16.04 to cover the resulting test gap. GitLab doesn't modify Docker images in the same way GitHub does and thus doesn't fall prey to the same issue. There are two compatibility issues uncovered by this:

  - Ubuntu 16.04 does not support HTTP/2 in Apache. We thus cannot set
    `GIT_TEST_HTTPD=true`, which would otherwise cause us to fail when
    Apache fails to start.

  - Ubuntu 16.04 cannot use recent JGit versions as they depend on a
    more recent Java runtime than we have available. We thus disable
    installing any kind of optional dependencies that do not come from
    the package manager.

These two restrictions are fine though, as we only really care about whether Git compiles and runs on such old distributions in the first place.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Test runs of this can be found at [1]. Note that the Alpine Linux jobs fail, due to an upstream regression in libcurl [2].

[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/210 [2]: <ZuPKvYP9ZZ2mhb4m@pks.im>

 .gitlab-ci.yml             | 3 +++
 ci/install-dependencies.sh | 5 +++++
 ci/lib.sh                  | 9 ++++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

On Fri, Sep 13, 2024 at 07:52:51AM +0200, Patrick Steinhardt wrote:

OK, this looks reasonable to me. I do think we could have our cake and eat it too on the Apache support if we added a GIT_TEST_HTTP2 knob. But it's probably not all that big a deal in practice, and after another 1.5 years I think we'd drop this 16.04 job anyway (since it will be out of LTS then).

Thanks for putting this together.

-Peff

On Fri, Sep 13, 2024 at 02:21:13AM -0400, Jeff King wrote:

Note that we _do_ run the Apache tests, but only for HTTP/1. That's what the "auto" setting does automatically: Apache starts up just fine without the HTTP/2 module and thus we run all tests that don't rely on HTTP/2. On the other hand it fails to boot with HTTP/2, and thus we skip over these tests automatically.

Patrick

On Fri, Sep 13, 2024 at 08:39:34AM +0200, Patrick Steinhardt wrote:

Right, what I mean is that we would not notice if that job started skipping the HTTP/1 tests (e.g., because we changed something in apache.conf that didn't work on that old distro). We know it works now, but our ideal config going forward is "skip the HTTP/2 tests if needed, but fail if the HTTP/1 tests do not run".

-Peff

On Fri, Sep 13, 2024 at 02:43:52AM -0400, Jeff King wrote:

Ah, that's true indeed. As you mention it probably doesn't matter all that much. I think it's nice to verify that things work to the best extent possible for such old platforms. But wiring up a new prereq just for that doesn't feel all that important to me.

Patrick

Jeff King <peff@peff.net> writes:

Yes, thanks, both.  Queued.