Skip to contentSkip to conversation

ci(windows): ensure that we do not pick up random executables

PATCH In review Johannes Schindelin via GitGitGadget 3 participants 4 messages Oct 13, 2021

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ci(windows): ensure that we do not pick up random executables

From: Johannes Schindelin <[email protected]>

On the Windows build agents, a lot of programs are installed, and added to the PATH automatically.

One such program is Git for Windows, and due to the way it is set up, unfortunately its copy of gpg.exe is also reachable via the PATH.

This usually does not pose any problems. To the contrary, it even allows us to test the GPG parts of Git's test suite even if gpg.exe is not delivered as part of git-sdk-64-minimal, the minimal subset of Git for Windows' SDK that we use in the CI builds to compile Git.

However, every once in a while we build a new MSYS2 runtime, which means that there is a mismatch between the copy in git-sdk-64-minimal and the copy in C:\Program Files\Git\usr\bin. When that happens we hit the dreaded problem where only one msys-2.0.dll is expected to be in the PATH, and things start to fail.

Let's avoid all of this by restricting the PATH to the minimal set. This is actually done by git-sdk-64-minimal's /etc/profile, and we just have to source this file manually (one would expect that it is sourced automatically, but the Bash steps in Azure Pipelines/GitHub workflows are explicitly run using --noprofile, hence the need for doing this explicitly).

---
ci(windows): avoid using external gpg by mistake

I stumbled over a CI failure this morning when I scrambled to tie up all the loose ends for an unexpected v2.33.1 release. Turns out that the CI picked up the installed Git for Windows' gpg.exe, and due to a (new, as of this morning) mismatch in the MSYS2 runtime between the installed Git for Windows and the subset of the Git for Windows SDK used for compiling in the CI runs, it worked just enough to pass the prereq, but then failed the tests.

Seeing as essentially every single CI build will fail without this patch right up until the point in time when Git for Windows is upgraded in the build agents (to a version that does yet exist), it would be good to fast-track this to maint.

Note: I based this on the earliest topic where it would apply without merge conflicts, js/ci-windows-update (which is unfortunately quite recent, it is not reachable from any version older than v2.33.0).

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1057%2Fdscho%2Fwork-around-windows-ci-failures-with-gpg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1057/dscho/work-around-windows-ci-failures-with-gpg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1057

 .github/workflows/main.yml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
.github/workflows/main.yml +3 -3
index 0f7516c9ef3..2a6d68718ae 100644
@@ -88,7 +88,7 @@ jobs:
       env:
         HOME: ${{runner.workspace}}
         NO_PERL: 1
-      run: ci/make-test-artifacts.sh artifacts
+      run: . /etc/profile && ci/make-test-artifacts.sh artifacts
     - name: zip up tracked files
       run: git archive -o artifacts/tracked.tar.gz HEAD
     - name: upload tracked files and build artifacts
@@ -115,7 +115,7 @@ jobs:
     - uses: git-for-windows/setup-git-for-windows-sdk@v1
     - name: test
       shell: bash
-      run: ci/run-test-slice.sh ${{matrix.nr}} 10
+      run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
     - name: ci/print-test-failures.sh
       if: failure()
       shell: bash
@@ -199,7 +199,7 @@ jobs:
       env:
         NO_SVN_TESTS: 1
         GIT_TEST_SKIP_REBASE_P: 1
-      run: ci/run-test-slice.sh ${{matrix.nr}} 10
+      run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
     - name: ci/print-test-failures.sh
       if: failure()
       shell: bash

base-commit: d681d0dc3a77016caa7e26abfe734afbdab44de5

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 3 lines

Note: I based this on the earliest topic where it would apply without merge conflicts, js/ci-windows-update (which is unfortunately quite recent, it is not reachable from any version older than v2.33.0).

So do you want this forked from v2.33.0 (and merged to 'maint' so that v2.33.2, if we need to issue it, would have it)?

https://github.com/git/git/actions/runs/1334961399 is the CI/PR run on v2.33.1 from yesterday. Our PATH wasn't contaminated and we didn't see the problem you fixed here by mere luck, and you were unlucky when you ran the same for generating your release material?

Thanks.

Quoted 39 lines

Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1057/dscho/work-around-windows-ci-failures-with-gpg-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1057

 .github/workflows/main.yml | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
.github/workflows/main.yml +3 -3
index 0f7516c9ef3..2a6d68718ae 100644
@@ -88,7 +88,7 @@ jobs:
       env:
         HOME: ${{runner.workspace}}
         NO_PERL: 1
-      run: ci/make-test-artifacts.sh artifacts
+      run: . /etc/profile && ci/make-test-artifacts.sh artifacts
     - name: zip up tracked files
       run: git archive -o artifacts/tracked.tar.gz HEAD
     - name: upload tracked files and build artifacts
@@ -115,7 +115,7 @@ jobs:
     - uses: git-for-windows/setup-git-for-windows-sdk@v1
     - name: test
       shell: bash
-      run: ci/run-test-slice.sh ${{matrix.nr}} 10
+      run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
     - name: ci/print-test-failures.sh
       if: failure()
       shell: bash
@@ -199,7 +199,7 @@ jobs:
       env:
         NO_SVN_TESTS: 1
         GIT_TEST_SKIP_REBASE_P: 1
-      run: ci/run-test-slice.sh ${{matrix.nr}} 10
+      run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
     - name: ci/print-test-failures.sh
       if: failure()
       shell: bash

base-commit: d681d0dc3a77016caa7e26abfe734afbdab44de5

#2 lore raw

Hi Junio,

Quoted 6 lines · Junio C Hamano
Quoted 3 lines

Note: I based this on the earliest topic where it would apply without merge conflicts, js/ci-windows-update (which is unfortunately quite recent, it is not reachable from any version older than v2.33.0).

So do you want this forked from v2.33.0 (and merged to 'maint' so that v2.33.2, if we need to issue it, would have it)?

Well, I followed your custom and based it on the oldest applicable topic, js/ci-windows-update. My preference would be for this to be merged into maint rather sooner than later, so that GitGitGadget PRs could target maint and get a meaningful test coverage.

Quoted 4 lines

https://github.com/git/git/actions/runs/1334961399 is the CI/PR run on v2.33.1 from yesterday. Our PATH wasn't contaminated and we didn't see the problem you fixed here by mere luck, and you were unlucky when you ran the same for generating your release material?

At the time this ran, the msys2-runtime package was already upgraded to a slightly newer version, and even deployed to Git for Windows' Pacman repository. However, that change was not yet reflected in the git-sdk-64 repository (which is a Git mirror, tracking all the Pacman packages that make up Git for Windows' SDK), as this repository is updated by a nightly automated build.

As a consequence, the git-sdk-64-minimal build had not run, therefore your build picked up the previous one, with the earlier msys2-runtime version.

That means that the msys-2.0.dll file contained in git-sdk-64-minimal was identical to the one in Git for Windows as installed on GitHub's build agents.

So yes, the PATH was "contaminated". Meaning: the test suite run did pick up the gpg.exe from Git for Windows. And since there was no msys-2.0.dll mismatch, it did its job as intended.

The problem arose when I scrambled to get all the things I wanted to wrap up before the next Git for Windows release, this past morning, at which stage the git-sdk-64-minimal had already picked up that new msys2-runtime version. As a consequence, the MSYS2 Bash we use to run Git's test suite in the CI builds had a msys-2.0.dll that was different from the one in C:\Program Files\Git\usr\bin (which lives next to the gpg.exe and is therefore used when running that program). That, in turn, prevented certain functionality from working. Which made the GPG-related tests fail (maybe not even all of them).

Ciao,
Dscho

#3 lore raw
Quoted 7 lines · Johannes Schindelin
Quoted 2 lines

So do you want this forked from v2.33.0 (and merged to 'maint' so that v2.33.2, if we need to issue it, would have it)?

Well, I followed your custom and based it on the oldest applicable topic, js/ci-windows-update. My preference would be for this to be merged into maint rather sooner than later, so that GitGitGadget PRs could target maint and get a meaningful test coverage.

Yes, will do. Tonight's pushout did not have it in 'next', but I do not think of a reason why we shouldn't fast-track the topic.

Thanks.