| Message ID | 2c4cd9dd1c8d966c8df0349bb820449ae1290793.1625439315.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ab0b6612918b833bdec775f7c9bf22c4c4ddd07 |
| Headers | show |
| Series | ci: speed-up the Windows parts of our GitHub workflow | expand |
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
Hi Ævar, On Mon, 5 Jul 2021, Ævar Arnfjörð 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 := 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. > 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. > 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 > > 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 > > >
"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?
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
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