ci: speed-up the Windows parts of our GitHub workflow

[dscho]

This patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action (Git for Windows is running with this Action for a while now, getting all the kinks out).

This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.

Changes since v2:

• Made the handwaving make [...] NO_GETTEXT comment in the commit message of the patch "artifacts-tar: respect NO_GETTEXT" more explicit, by setting NO_GETTEXT to a bogus value as required by make.
• Added an explicit NO_GETTEXT=YesPlease to the make artifacts-tar invocation in the vs-build job, as well as an explanation in the corresponding commit message why this explicit mention is technically not required.

Changes since v1:

• Added a patch to fix make NO_GETTEXT=Yep artifacts-tar (not to include .mo files), as suggested by Matthias Aßauer in the GitGitGadget PR, which should fix the CI failure in seen that Junio pointed out. The bug was unhidden by mr/cmake fixing the CMake build (which ignored NO_GETTEXT before).

cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

[dennisameling]

@dscho you probably need to set an explicit output directory like I just did in git-for-windows@28c9dbc (path: ./git-sdk-64-minimal), otherwise it will default to the C:\ root according to your GH Action: https://github.com/git-for-windows/setup-git-for-windows-sdk/blob/main/main.ts#L19

[dscho]

@dscho you probably need to set an explicit output directory like I just did in git-for-windows@28c9dbc (path: ./git-sdk-64-minimal), otherwise it will default to the C:\ root according to your GH Action: https://github.com/git-for-windows/setup-git-for-windows-sdk/blob/main/main.ts#L19

Oh, but it's a good thing that git-sdk-64-minimal is installed to C:\... that way, it won't show up as "untracked" (which would make the run fail).

[dscho]

I backed out the "cache vcpkg artifacts" part into #879: There is an external issue that requires a workaround, and I hope that it'll be resolved soon and won't need that workaround any longer.

[dscho]

I guess I want to wait just a couple more days, to see setup-git-for-windows-sdk stabilize, and then tag a v1.0.0 of that Action. After that, I'll edit this PR and send it to the list.

[dscho]

I guess I want to wait just a couple more days, to see setup-git-for-windows-sdk stabilize, and then tag a v1.0.0 of that Action. After that, I'll edit this PR and send it to the list.

I did tag v1.0.0...

[dscho]

/submit

[gitgitgadget]

Submitted as pull.878.git.1624461857.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-878/dscho/use-setup-git-for-windows-sdk-action-v1

To fetch this version to local tag pr-878/dscho/use-setup-git-for-windows-sdk-action-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-878/dscho/use-setup-git-for-windows-sdk-action-v1

[gitgitgadget]

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

[dscho]

/submit

[gitgitgadget]

Submitted as pull.878.v3.git.1625439315.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-878/dscho/use-setup-git-for-windows-sdk-action-v3

To fetch this version to local tag pr-878/dscho/use-setup-git-for-windows-sdk-action-v3:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-878/dscho/use-setup-git-for-windows-sdk-action-v3

[gitgitgadget]

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Sun, Jul 04 2021, Dennis Ameling via GitGitGadget wrote:

> From: Dennis Ameling <dennis@dennisameling.com>

Re the v3 cover letter & my v2 comment:

> We already build Git for Windows with `NO_GETTEXT` when compiling with
> GCC. Let's do the same with Visual C, too.
>
> Note that we do not technically _need_ to pass `NO_GETTEXT` explicitly
> in that `make artifacts-tar` invocation because we do this while `MSVC`
> is set (which will set `uname_S := Windows`, which in turn will set
> `NO_GETTEXT = YesPlease`). But it is definitely nicer to be explicit
> here.

Okey, so we never used it in the first place. That makes the subject and
first paragraph of the commit message seem really out of place. So we
really mean something like this instead?:

    ci(vs-build): be explicit about NO_GETTEXT

    We already supply `NO_GETTEXT` implicitly due to config.mak.uname,
    but let's do so explicitly to ???

But if we're being explicit we also have SKIP_DASHED_BUILT_INS=YesPlease
since ef60e9f74b2 (ci: stop linking built-ins to the dashed versions,
2020-09-21) for CI, which has an even bigger effect on what's included
in the tarball, so it seems odd to single out NO_GETTEXT=YesPlease,
unless there's some other reason to do this that I'm missing.

Hrm, isn't the real reason here that before 5/7 this would error out,
because while NO_GETTEXT=Y was implicit and we picked it up from the
config.mak.uname, we just had the $(MOFILES) in the archive-tar list
unconditionally.

So after 5/7 that's not the case, so we don't need this change anymore,
but we're making this change anyway? Seems like the result of this being
the first try at a fix, and then re-sequencing the two & keeping the
now-redundant hotfix.

> Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
> Helped-by: Matthias Aßhauer <mha1993@live.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .github/workflows/main.yml | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 0f7516c9ef3..c99628681ef 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -159,7 +159,7 @@ jobs:
>        shell: bash
>        run: |
>          cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows \
> -        -DMSGFMT_EXE=C:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
> +        -DNO_GETTEXT=YesPlease -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
>      - name: MSBuild
>        run: msbuild git.sln -property:Configuration=Release -property:Platform=x64 -maxCpuCount:4 -property:PlatformToolset=v142
>      - name: bundle artifact tar
> @@ -169,7 +169,7 @@ jobs:
>          VCPKG_ROOT: ${{github.workspace}}\compat\vcbuild\vcpkg
>        run: |
>          mkdir -p artifacts &&
> -        eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)"
> +        eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts NO_GETTEXT=YesPlease 2>&1 | grep ^tar)"
>      - name: zip up tracked files
>        run: git archive -o artifacts/tracked.tar.gz HEAD
>      - name: upload tracked files and build artifacts

[gitgitgadget]

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

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-773851311-1625489060=:8230
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi =C3=86var,

On Mon, 5 Jul 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:

> On Sun, Jul 04 2021, Dennis Ameling via GitGitGadget wrote:
>
> > From: Dennis Ameling <dennis@dennisameling.com>
>
> Re the v3 cover letter & my v2 comment:
>
> > We already build Git for Windows with `NO_GETTEXT` when compiling with
> > GCC. Let's do the same with Visual C, too.
> >
> > Note that we do not technically _need_ to pass `NO_GETTEXT` explicitly
> > in that `make artifacts-tar` invocation because we do this while `MSVC=
`
> > is set (which will set `uname_S :=3D Windows`, which in turn will set
> > `NO_GETTEXT =3D YesPlease`). But it is definitely nicer to be explicit
> > here.
>
> Okey, so we never used it in the first place.

No, you misunderstood.

While it _is_ true that we set `NO_GETTEXT` implicitly (via `MSVC`) when
running `artifacts-tar`, that flag was ignored before this here patch
series.

> That makes the subject and first paragraph of the commit message seem
> really out of place. So we really mean something like this instead?:
>
>     ci(vs-build): be explicit about NO_GETTEXT
>
>     We already supply `NO_GETTEXT` implicitly due to config.mak.uname,
>     but let's do so explicitly to ???
>
> But if we're being explicit we also have SKIP_DASHED_BUILT_INS=3DYesPlea=
se
> since ef60e9f74b2 (ci: stop linking built-ins to the dashed versions,
> 2020-09-21) for CI, which has an even bigger effect on what's included
> in the tarball, so it seems odd to single out NO_GETTEXT=3DYesPlease,
> unless there's some other reason to do this that I'm missing.

Yes, you are missing the fact that the `SKIP_DASHED_BUILT_INS` flag is set
explicitly.

The `NO_GETTEXT` flag was _not_ set explicitly. It is set by the section
of `config.mak.uname` that is in effect if `uname_S` is set to `Windows`,
which it is if we set the `MSVC` flag, which we still set in `vs-build`,
for tradition, even if we no longer build with `make MSVC=3DOhYeah` but
using MSBuild.

I hope this removes any confusion.

> Hrm, isn't the real reason here that before 5/7 this would error out,
> because while NO_GETTEXT=3DY was implicit and we picked it up from the
> config.mak.uname, we just had the $(MOFILES) in the archive-tar list
> unconditionally.
>
> So after 5/7 that's not the case, so we don't need this change anymore,
> but we're making this change anyway? Seems like the result of this being
> the first try at a fix, and then re-sequencing the two & keeping the
> now-redundant hotfix.

Excuse me?

This here patch sets `NO_GETTEXT` when building Git in the `vs-build` job,
and consequently sets `NO_GETTEXT` when bundling up the artifacts tar.

It does so to accelerate the build which is legitimate because we already
test the gettext stuff in the `windows-build`/`windows-test` jobs.

There is nothing "hotfix" about this.

Ciao,
Johannes

> > Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
> > Helped-by: Matthias A=C3=9Fhauer <mha1993@live.de>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .github/workflows/main.yml | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index 0f7516c9ef3..c99628681ef 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -159,7 +159,7 @@ jobs:
> >        shell: bash
> >        run: |
> >          cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=3D`pwd`=
/compat/vcbuild/vcpkg/installed/x64-windows \
> > -        -DMSGFMT_EXE=3DC:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -=
DPERL_TESTS=3DOFF -DPYTHON_TESTS=3DOFF -DCURL_NO_CURL_CMAKE=3DON
> > +        -DNO_GETTEXT=3DYesPlease -DPERL_TESTS=3DOFF -DPYTHON_TESTS=3D=
OFF -DCURL_NO_CURL_CMAKE=3DON
> >      - name: MSBuild
> >        run: msbuild git.sln -property:Configuration=3DRelease -propert=
y:Platform=3Dx64 -maxCpuCount:4 -property:PlatformToolset=3Dv142
> >      - name: bundle artifact tar
> > @@ -169,7 +169,7 @@ jobs:
> >          VCPKG_ROOT: ${{github.workspace}}\compat\vcbuild\vcpkg
> >        run: |
> >          mkdir -p artifacts &&
> > -        eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=3DYes=
Please ARTIFACTS_DIRECTORY=3Dartifacts 2>&1 | grep ^tar)"
> > +        eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=3DYes=
Please ARTIFACTS_DIRECTORY=3Dartifacts NO_GETTEXT=3DYesPlease 2>&1 | grep =
^tar)"
> >      - name: zip up tracked files
> >        run: git archive -o artifacts/tracked.tar.gz HEAD
> >      - name: upload tracked files and build artifacts
>
>
>

--8323328-773851311-1625489060=:8230--

[gitgitgadget]

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This Action not only downloads and extracts git-sdk-64-minimal _outside_
> the worktree (making it no longer necessary to meddle with
> `.gitignore` or `.git/info/exclude`), it also adds the `bash.exe` to the
> `PATH` and sets the environment variable `MSYSTEM` (an implementation
> detail that Git's workflow should never have needed to know about).
>
> This allows us to convert all those funny PowerShell tasks that wanted
> to call git-sdk-64-minimal's `bash.exe`: they all are now regular `bash`
> scriptlets.
>
> This finally lets us get rid of the funny quoting and escaping where we
> had to pay attention not only to quote and escape the Bash scriptlets
> properly, but also to add a second level of escaping (with backslashes
> for double quotes and backticks for dollar signs) to stop PowerShell
> from doing unintended things.
>
> Further, this Action uses a fast caching strategy native to GitHub
> Actions that should accelerate the download across CI runs:
> git-sdk-64-minimal is usually updated once per 24h, and needs to be
> cached only once within that period. Caching it (unfortunately only on
> a per-branch basis) speeds up the download step, and makes it much more
> robust at the same time by virtue of accessing a cache location that is
> closer in the network topology.

So nice.

[gitgitgadget]

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

"Dennis Ameling via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Dennis Ameling <dennis@dennisameling.com>
>
> We already build Git for Windows with `NO_GETTEXT` when compiling with
> GCC. Let's do the same with Visual C, too.
>
> Note that we do not technically _need_ to pass `NO_GETTEXT` explicitly
> in that `make artifacts-tar` invocation because we do this while `MSVC`
> is set (which will set `uname_S := Windows`, which in turn will set
> `NO_GETTEXT = YesPlease`). But it is definitely nicer to be explicit
> here.

In other words, is this a no-op but makes the recipe more readable?

[gitgitgadget]

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

Hi Junio,

On Tue, 6 Jul 2021, Junio C Hamano wrote:

> "Dennis Ameling via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Dennis Ameling <dennis@dennisameling.com>
> >
> > We already build Git for Windows with `NO_GETTEXT` when compiling with
> > GCC. Let's do the same with Visual C, too.
> >
> > Note that we do not technically _need_ to pass `NO_GETTEXT` explicitly
> > in that `make artifacts-tar` invocation because we do this while `MSVC`
> > is set (which will set `uname_S := Windows`, which in turn will set
> > `NO_GETTEXT = YesPlease`). But it is definitely nicer to be explicit
> > here.
>
> In other words, is this a no-op but makes the recipe more readable?

Yes. And it also removes some puzzlement from the thorough reviewer
(Matthias stumbled over it and was wondering why this even works without
`NO_GETTEXT`).

Thanks,
Dscho

[gitgitgadget]

This patch series was integrated into seen via git@3811d25.

[gitgitgadget]

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

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This patch series upgrades to newer versions of a couple GitHub Actions we
> use, and also streamlines the Windows jobs using the relatively new
> setup-git-for-windows-sdk Action
> [https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for
> Windows is running with this Action for a while now, getting all the kinks
> out).
>
> This patch series should also address the problem where seen was pushed so
> rapidly that the windows-test jobs failed because they no longer checked out
> the identical revision as the windows-build job.
>
> Changes since v2:
>
>  * Made the handwaving make [...] NO_GETTEXT comment in the commit message
>    of the patch "artifacts-tar: respect NO_GETTEXT" more explicit, by
>    setting NO_GETTEXT to a bogus value as required by make.
>  * Added an explicit NO_GETTEXT=YesPlease to the make artifacts-tar
>    invocation in the vs-build job, as well as an explanation in the
>    corresponding commit message why this explicit mention is technically not
>    required.

The above indeed helps the CI at GitHub.  The last run with 'seen'
can be seen at https://github.com/git/git/actions/runs/1005977569

Thanks.

[gitgitgadget]

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

GitHub Actions / CI update.

[gitgitgadget]

This patch series was integrated into seen via git@8e2a985.

[gitgitgadget]

This patch series was integrated into seen via git@52ced65.

[gitgitgadget]

This patch series was integrated into seen via git@2d9b3bb.

[gitgitgadget]

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

GitHub Actions / CI update.

Will merge to 'next'.

[gitgitgadget]

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

[gitgitgadget]

This patch series was integrated into seen via git@5514c42.

[gitgitgadget]

This patch series was integrated into next via git@329771e.

[gitgitgadget]

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

GitHub Actions / CI update.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@9a332c6.

[gitgitgadget]

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

GitHub Actions / CI update.

Will merge to 'master'.

[gitgitgadget]

This patch series was integrated into seen via git@67737fc.

[gitgitgadget]

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

GitHub Actions / CI update.

Will merge to 'master'.

[gitgitgadget]

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

[gitgitgadget]

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

[gitgitgadget]

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

[gitgitgadget]

Closed via dae59cb.