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

Commit Message

Dennis Ameling June 23, 2021, 3:24 p.m. UTC

From: Dennis Ameling <[e-mail]>

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

Signed-off-by: Dennis Ameling <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 .github/workflows/main.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason July 4, 2021, 8:27 a.m. UTC | #1

On Wed, Jun 23 2021, Dennis Ameling via GitGitGadget wrote:

> From: Dennis Ameling <[e-mail]>
>
> We already build Git for Windows with `NO_GETTEXT` when compiling with
> GCC. Let's do the same with Visual C, too.
>
> Signed-off-by: Dennis Ameling <[e-mail]>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
>  .github/workflows/main.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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? So "we already do X in job A, let's do it X in job B" seems like
the opposite of a self-justifying rationale, surely our default approach
would be to do things differently?

Perhaps this change makes sense, just commenting on the explanation in
the commit message. I think it should cover:

 * Why: Is it being done for speed, hard to install libintl or whatever?

 * Coverage: Before we tested on this platform with this knob, do we
   have another CI job that covers what we'll lose in coverage here, if
   not is it OK because it's tested in some other way etc?

Ævar Arnfjörð Bjarmason July 4, 2021, 8:52 a.m. UTC | #2

On Sun, Jul 04 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jun 23 2021, Dennis Ameling via GitGitGadget wrote:
>
>> From: Dennis Ameling <[e-mail]>
>>
>> We already build Git for Windows with `NO_GETTEXT` when compiling with
>> GCC. Let's do the same with Visual C, too.
>>
>> Signed-off-by: Dennis Ameling <[e-mail]>
>> Signed-off-by: Johannes Schindelin <[e-mail]>
>> ---
>>  .github/workflows/main.yml | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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? So "we already do X in job A, let's do it X in job B" seems like
> the opposite of a self-justifying rationale, surely our default approach
> would be to do things differently?
>
> Perhaps this change makes sense, just commenting on the explanation in
> the commit message. I think it should cover:
>
>  * Why: Is it being done for speed, hard to install libintl or whatever?
>
>  * Coverage: Before we tested on this platform with this knob, do we
>    have another CI job that covers what we'll lose in coverage here, if
>    not is it OK because it's tested in some other way etc?

Aside: I should have replied to the v2[1] version of this, but the same
comment applies to it. Sorry about the confusion.

1. http://lore.kernel.org/git/8bab[e-mail]

Johannes Schindelin July 13, 2021, 12:19 p.m. UTC | #3

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

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

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.

Ciao,
Johannes

Philip Oakley July 13, 2021, 3:11 p.m. UTC | #4

On 13/07/2021 13:19, Johannes Schindelin wrote:
> 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. 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.
>
> 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?)
>
> 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.
>
> Ciao,
> Johannes
Folks,

This problem (vcpkg/VS/CI build breakage for the tar artifact) has also
just come up for me on
https://github.com/git-for-windows/git/pull/3306#issuecomment-879093798
"[RFC] making Git for Windows start buildins in modern Visual Studio
#3306" which is a similar build situation.

Hopefully we can chose a suitable solution regarding what, if any, extra
checks this part of the CI build ought to do with the (unused in this
build) `.mo` files.

--
Philip

Ævar Arnfjörð Bjarmason July 14, 2021, 7:54 a.m. UTC | #5

On Tue, Jul 13 2021, Johannes Schindelin wrote:

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

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

> 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/c31d[e-mail]/

Johannes Schindelin July 14, 2021, 8:51 a.m. UTC | #6

Hi Philip,

On Tue, 13 Jul 2021, Philip Oakley wrote:

> On 13/07/2021 13:19, Johannes Schindelin wrote:
> >
> > 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.
>
> This problem (vcpkg/VS/CI build breakage for the tar artifact) has also
> just come up for me on
> https://github.com/git-for-windows/git/pull/3306#issuecomment-879093798
> "[RFC] making Git for Windows start buildins in modern Visual Studio
> #3306" which is a similar build situation.
>
> Hopefully we can chose a suitable solution regarding what, if any, extra
> checks this part of the CI build ought to do with the (unused in this
> build) `.mo` files.

You will be happy to hear that the fix was in v2 of this patch series (you
responded to v1).

Ciao,
Dscho

Patch

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