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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 14, 2021, 07:54 UTC
Message-ID: <87r1g11et1.fsf@evledraar.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2107131403040.76@tvgsbejvaqbjf.bet>

On Tue, Jul 13 2021, Johannes Schindelin wrote:

Show 35 quoted lines

> Hi Ævar,
>
> On Sun, 4 Jul 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Wed, Jun 23 2021, Dennis Ameling via GitGitGadget wrote:
>>
>> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> > index 0f7516c9ef..3b40c677ab 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
>>
>> To a first approximation isn't the point of CI to test a diversity of
>> setups?
>
> No, the point of CI is to find breakages.
>
> And in this instance, a careful analysis will reveal that the artifacts
> generated via `msgfmt` are not even used, let alone tested.
>
> Even more careful analysis will reveal that Git for Windows does not ship
> with translations. Therefore, it would be pointless to test the
> translations here. Doubly so because Git for Windows is not even built
> using MSVC, and this `vs-build` job is really about verifying that we can
> build with that compiler, and _iff_ we used `msgfmt` here, it would not be
> Visual Studio's (because it doesn't have any `msgfmt`) but the regular one
> from Git for Windows' SDK. [...]

Sure, to clarify my comment to Dennis Ameling were to the effect of "hrm, this commit message doesn't seem to justify this change", not that the change wasn't justified.

I.e. it *read* a bit similar to "this CI job already uses clang, let's change this job that uses gcc to use it too", and seemed to take it as a given that making them consistent was an improvement.

Which prompted my review to the effect "hang on a minute, by default we want things different in CI, surely?".

From your explanation here the reason seems closer to (but I'm not sure I got all the nuances involved) that the lack of -DNO_GETTEXT=YesPlease was always an omission here, and this change really changes nothing.

But then isn't this a change that belongs in config.mak.uname since we never use this on this platform? Or do you mean by "Git for Windows" the configuration you build & ship that release in, not "A Git for Windows" in the sense of what we're building here (which can/does support gettext, or not?).

(I see reading on that some of that, i.e. the config.mak.uname suggestion is wrong)....

> [...] Triply so because the regular one from Git for
> Windows' SDK _is actually used_ in the `windows-build` job, therefore we
> _really_ do not need to repeat the same exercise with the same gettext
> version and the same git/git revision in the same GitHub workflow run.

..., okey, So in any case we do test this elsewhere to bring this around the entirety of this thread I think could be addressed by just changing

    We already build Git for Windows with `NO_GETTEXT` when compiling with
    GCC. Let's do the same with Visual C, too.

To something like:

    In the XYZ job for Windows we don't need to build with GETTEXT
    because any coverage gained there is covered by the ABC job. The IJK
    job on the same platform already builds with -DNO_GETTEXT=YesPlease,
    let's do the same here.

Show 5 quoted lines

> In short: there is no diversity to be gained here. (Actually, that is not
> quite true: by using gentler language on a first-time contributor instead
> of immediately going into full critic mode without any fanfare, we could
> have invited more diversity onto this list. Maybe you could pay more
> attention to that in the future?)

I'll let others judge for themselves but I do think I'm using gentle language here, particularly since the quoted "To a first approximation isn't the point[...]" of mine is immediately followed by (which you elided) a:

    "Perhaps this change makes sense, just commenting on the explanation
    in the commit message."

I.e. I think it's quite clear that I'm commenting on my own confusion, not asserting something about the change itself.

Show 5 quoted lines

> Back to the issue you pointed out: could this be described in the commit
> message? Sure. But I think there is no much worth in it, the surrounding
> code and parent commit makes it clear that the `.mo` files aren't used,
> and that it would therefore be a waste of electrons to generate them in
> each and every `vs-build` run.

Yes. If we have NO_GETTEXT=Y we shouldn't make the *.mo files and bundle them up in the archive. It's clear from me from this patch & the following[1] that we unambiguously end up in a state of NO_GETTEXT=Y.

I was commenting on the "why" of that change in the context of assuming that the change made sense, but that the commit message wasn't per-es justifying it to someone unfamiliar with the area.

I think your citing of reasons for the change that aren't at all mentioned in the commit message supports that.

1. https://lore.kernel.org/git/c31d2e7f44a8b27210dbde9bc6938ce16a9e0c17.1625347592.git.gitgitgadget@gmail.com/