git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Dennis Ameling via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Dennis Ameling <dennis@dennisameling.com>
Subject: Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
Date: Wed, 14 Jul 2021 09:54:02 +0200	[thread overview]
Message-ID: <87r1g11et1.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2107131403040.76@tvgsbejvaqbjf.bet>


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/c31d2e7f44a8b27210dbde9bc6938ce16a9e0c17.1625347592.git.gitgitgadget@gmail.com/

  parent reply	other threads:[~2021-07-14  8:26 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23 15:24 [PATCH 0/6] ci: speed-up the Windows parts of our GitHub workflow Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 1/6] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 2/6] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 3/6] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 4/6] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-06-23 15:24 ` [PATCH 5/6] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-04  8:27   ` Ævar Arnfjörð Bjarmason
2021-07-04  8:52     ` Ævar Arnfjörð Bjarmason
2021-07-13 12:19     ` Johannes Schindelin
2021-07-13 15:11       ` Philip Oakley
2021-07-14  8:51         ` Johannes Schindelin
2021-07-14  7:54       ` Ævar Arnfjörð Bjarmason [this message]
2021-06-23 15:24 ` [PATCH 6/6] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-01 22:03 ` [PATCH 0/6] ci: speed-up the Windows parts of our GitHub workflow Junio C Hamano
2021-07-03 21:26 ` [PATCH v2 0/7] " Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 1/7] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 2/7] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 3/7] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 4/7] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT Johannes Schindelin via GitGitGadget
2021-07-04  8:30     ` Ævar Arnfjörð Bjarmason
2021-07-04 22:52       ` Johannes Schindelin
2021-07-05  6:33         ` Ævar Arnfjörð Bjarmason
2021-07-03 21:26   ` [PATCH v2 6/7] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-03 21:26   ` [PATCH v2 7/7] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-04  8:54     ` Ævar Arnfjörð Bjarmason
2021-07-04 22:37       ` Johannes Schindelin
2021-07-04 22:55   ` [PATCH v3 0/7] ci: speed-up the Windows parts of our GitHub workflow Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 1/7] ci: use the new GitHub Action to download git-sdk-64-minimal Johannes Schindelin via GitGitGadget
2021-07-06 19:16       ` Junio C Hamano
2021-07-04 22:55     ` [PATCH v3 2/7] ci (vs-build): use `cmd` to copy the DLLs, not `powershell` Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 3/7] ci: upgrade to using actions/{up,down}load-artifacts v2 Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 4/7] ci(windows): transfer also the Git-tracked files to the test jobs Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 5/7] artifacts-tar: respect NO_GETTEXT Johannes Schindelin via GitGitGadget
2021-07-04 22:55     ` [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT Dennis Ameling via GitGitGadget
2021-07-05  6:45       ` Ævar Arnfjörð Bjarmason
2021-07-05 12:44         ` Johannes Schindelin
2021-07-06 19:19       ` Junio C Hamano
2021-07-14  8:47         ` Johannes Schindelin
2021-07-04 22:55     ` [PATCH v3 7/7] ci: accelerate the checkout Johannes Schindelin via GitGitGadget
2021-07-06 23:20     ` [PATCH v3 0/7] ci: speed-up the Windows parts of our GitHub workflow Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r1g11et1.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=dennis@dennisameling.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).