Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci(windows): avoid using external gpg by mistake #1057

Conversation

dscho
Copy link
Member

@dscho dscho commented Oct 13, 2021

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

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Oct 13, 2021

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

Submitted as pull.1057.git.1634129748874.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1057/dscho/work-around-windows-ci-failures-with-gpg-v1

To fetch this version to local tag pr-1057/dscho/work-around-windows-ci-failures-with-gpg-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1057/dscho/work-around-windows-ci-failures-with-gpg-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> 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(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 0f7516c9ef3..2a6d68718ae 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -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

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

This branch is now known as js/windows-ci-path-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

This patch series was integrated into seen via git@d01c730.

@gitgitgadget gitgitgadget bot added the seen label Oct 13, 2021
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Junio,

On Wed, 13 Oct 2021, Junio C Hamano wrote:

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

> 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

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 13, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 14, 2021

This patch series was integrated into seen via git@4cabc44.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 14, 2021

This patch series was integrated into next via git@3047fe5.

@gitgitgadget gitgitgadget bot added the next label Oct 14, 2021
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 15, 2021

There was a status update in the "New Topics" section about the branch js/windows-ci-path-fix on the Git mailing list:

The PATH used in CI job may be too wide and let incompatible dlls
to be grabbed, which can cause the build&test to fail.  Tighten it.

Will merge to 'master'.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2021

This patch series was integrated into seen via git@1801777.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 18, 2021

This patch series was integrated into seen via git@f4c4923.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 19, 2021

This patch series was integrated into seen via git@ada1a17.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 19, 2021

This patch series was integrated into next via git@ada1a17.

@gitgitgadget
Copy link

gitgitgadget bot commented Oct 19, 2021

This patch series was integrated into master via git@ada1a17.

@gitgitgadget gitgitgadget bot added the master label Oct 19, 2021
@gitgitgadget gitgitgadget bot closed this Oct 19, 2021
@gitgitgadget
Copy link

gitgitgadget bot commented Oct 19, 2021

Closed via ada1a17.

@dscho dscho deleted the work-around-windows-ci-failures-with-gpg branch October 19, 2021 05:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant