Re: [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 5, 2021, 12:44 UTC
Message-ID: <nycvar.QRO.7.76.6.2107051431440.8230@tvgsbejvaqbjf.bet>
In-Reply-To: <87o8bh6yhf.fsf@evledraar.gmail.com>

Hi Ævar,

On Mon, 5 Jul 2021, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

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

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.

Show 13 quoted lines

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

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=OhYeah` but using MSBuild.

I hope this removes any confusion.

Show 9 quoted lines

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

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

Show 32 quoted lines

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