Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: speed-up the Windows parts of our GitHub workflow #878

Closed
wants to merge 7 commits into from
Closed

ci: speed-up the Windows parts of our GitHub workflow #878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2e4db68
ci: use the new GitHub Action to download git-sdk-64-minimal
dscho Feb 18, 2021
6b12fe2
ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
dscho Feb 18, 2021
c256bbf
ci: upgrade to using actions/{up,down}load-artifacts v2
dscho Feb 18, 2021
59dc444
ci(windows): transfer also the Git-tracked files to the test jobs
dscho Feb 18, 2021
64f7b1f
artifacts-tar: respect NO_GETTEXT
dscho Jul 3, 2021
2c4cd9d
ci(vs-build): build with NO_GETTEXT
dennisameling Feb 11, 2021
db54bf9
ci: accelerate the checkout
dscho Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 46 additions & 111 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,44 +81,21 @@ jobs:
if: needs.ci-config.outputs.enabled == 'yes'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> By upgrading from v1 to v2 of `actions/checkout`, we avoid fetching all
> the tags and the complete history: v2 only fetches one revision by
> default. This should make things a lot faster.
>
> Note that `actions/checkout@v2` seems to be incompatible with running in
> containers: https://github.com/actions/checkout/issues/151. Therefore,
> we stick with v1 there.

I'd suggest that we shouldn't link to a "closed" issue here and instead
to what seems to be the successor issue:
https://github.com/actions/checkout/issues/334

But looking at #151 most of the issue is a bazillion commit references
to this commit being rebased again and again, seems like github isn't
especially well set up for the "spam" our perpetual rebasing of the same
commits causes :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-602958541-1625438250=:8230
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi =C3=86var,

On Sun, 4 Jul 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:

>
> On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > By upgrading from v1 to v2 of `actions/checkout`, we avoid fetching al=
l
> > the tags and the complete history: v2 only fetches one revision by
> > default. This should make things a lot faster.
> >
> > Note that `actions/checkout@v2` seems to be incompatible with running =
in
> > containers: https://github.com/actions/checkout/issues/151. Therefore,
> > we stick with v1 there.
>
> I'd suggest that we shouldn't link to a "closed" issue here and instead
> to what seems to be the successor issue:
> https://github.com/actions/checkout/issues/334

I'd suggest that we can still link to this issue, even if it was closed
without the bug actually having been fixed. The ticket describes the
problem well.

> But looking at #151 most of the issue is a bazillion commit references
> to this commit being rebased again and again, seems like github isn't
> especially well set up for the "spam" our perpetual rebasing of the same
> commits causes :)

Sure, blame me.

Ciao,
Dscho

--8323328-602958541-1625438250=:8230--

runs-on: windows-latest
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-773851311-1625489060=:8230
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi =C3=86var,

On Mon, 5 Jul 2021, =C3=86var Arnfj=C3=B6r=C3=B0 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 :=3D Windows`, which in turn will set
> > `NO_GETTEXT =3D 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=3DYesPlea=
se
> 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=3DYesPlease,
> 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=3DOhYeah` 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=3DY 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=C3=9Fhauer <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=3D`pwd`=
/compat/vcbuild/vcpkg/installed/x64-windows \
> > -        -DMSGFMT_EXE=3DC:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -=
DPERL_TESTS=3DOFF -DPYTHON_TESTS=3DOFF -DCURL_NO_CURL_CMAKE=3DON
> > +        -DNO_GETTEXT=3DYesPlease -DPERL_TESTS=3DOFF -DPYTHON_TESTS=3D=
OFF -DCURL_NO_CURL_CMAKE=3DON
> >      - name: MSBuild
> >        run: msbuild git.sln -property:Configuration=3DRelease -propert=
y:Platform=3Dx64 -maxCpuCount:4 -property:PlatformToolset=3Dv142
> >      - 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=3DYes=
Please ARTIFACTS_DIRECTORY=3Dartifacts 2>&1 | grep ^tar)"
> > +        eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=3DYes=
Please ARTIFACTS_DIRECTORY=3Dartifacts NO_GETTEXT=3DYesPlease 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
>
>
>

--8323328-773851311-1625489060=:8230--

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> This Action not only downloads and extracts git-sdk-64-minimal _outside_
> the worktree (making it no longer necessary to meddle with
> `.gitignore` or `.git/info/exclude`), it also adds the `bash.exe` to the
> `PATH` and sets the environment variable `MSYSTEM` (an implementation
> detail that Git's workflow should never have needed to know about).
>
> This allows us to convert all those funny PowerShell tasks that wanted
> to call git-sdk-64-minimal's `bash.exe`: they all are now regular `bash`
> scriptlets.
>
> This finally lets us get rid of the funny quoting and escaping where we
> had to pay attention not only to quote and escape the Bash scriptlets
> properly, but also to add a second level of escaping (with backslashes
> for double quotes and backticks for dollar signs) to stop PowerShell
> from doing unintended things.
>
> Further, this Action uses a fast caching strategy native to GitHub
> Actions that should accelerate the download across CI runs:
> git-sdk-64-minimal is usually updated once per 24h, and needs to be
> cached only once within that period. Caching it (unfortunately only on
> a per-branch basis) speeds up the download step, and makes it much more
> robust at the same time by virtue of accessing a cache location that is
> closer in the network topology.

So nice.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

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

steps:
- uses: actions/checkout@v1
- name: download git-sdk-64-minimal
shell: bash
run: |
## Get artifact
urlbase=https://dev.azure.com/git-for-windows/git/_apis/build/builds
id=$(curl "$urlbase?definitions=22&statusFilter=completed&resultFilter=succeeded&\$top=1" |
jq -r ".value[] | .id")
download_url="$(curl "$urlbase/$id/artifacts" |
jq -r '.value[] | select(.name == "git-sdk-64-minimal").resource.downloadUrl')"
curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max-time 240 \
-o artifacts.zip "$download_url"

## Unzip and remove the artifact
unzip artifacts.zip
rm artifacts.zip
- uses: actions/checkout@v2
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: build
shell: powershell
shell: bash
env:
HOME: ${{runner.workspace}}
MSYSTEM: MINGW64
NO_PERL: 1
run: |
& .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
printf '%s\n' /git-sdk-64-minimal/ >>.git/info/exclude

ci/make-test-artifacts.sh artifacts
"@
- name: upload build artifacts
uses: actions/upload-artifact@v1
run: ci/make-test-artifacts.sh artifacts
- name: zip up tracked files
run: git archive -o artifacts/tracked.tar.gz HEAD
- name: upload tracked files and build artifacts
uses: actions/upload-artifact@v2
with:
name: windows-artifacts
path: artifacts
- name: upload git-sdk-64-minimal
uses: actions/upload-artifact@v1
with:
name: git-sdk-64-minimal
path: git-sdk-64-minimal
windows-test:
runs-on: windows-latest
needs: [windows-build]
Expand All @@ -127,65 +104,38 @@ jobs:
matrix:
nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
steps:
- uses: actions/checkout@v1
- name: download build artifacts
uses: actions/download-artifact@v1
- name: download tracked files and build artifacts
uses: actions/download-artifact@v2
with:
name: windows-artifacts
path: ${{github.workspace}}
- name: extract build artifacts
- name: extract tracked files and build artifacts
shell: bash
run: tar xf artifacts.tar.gz
- name: download git-sdk-64-minimal
uses: actions/download-artifact@v1
with:
name: git-sdk-64-minimal
path: ${{github.workspace}}/git-sdk-64-minimal/
run: tar xf artifacts.tar.gz && tar xf tracked.tar.gz
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: test
shell: powershell
run: |
& .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
# Let Git ignore the SDK
printf '%s\n' /git-sdk-64-minimal/ >>.git/info/exclude

ci/run-test-slice.sh ${{matrix.nr}} 10
"@
shell: bash
run: ci/run-test-slice.sh ${{matrix.nr}} 10
- name: ci/print-test-failures.sh
if: failure()
shell: powershell
run: |
& .\git-sdk-64-minimal\usr\bin\bash.exe -lc ci/print-test-failures.sh
shell: bash
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
uses: actions/upload-artifact@v1
uses: actions/upload-artifact@v2
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
vs-build:
needs: ci-config
if: needs.ci-config.outputs.enabled == 'yes'
env:
MSYSTEM: MINGW64
NO_PERL: 1
GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'"
runs-on: windows-latest
steps:
- uses: actions/checkout@v1
- name: download git-sdk-64-minimal
shell: bash
run: |
## Get artifact
urlbase=https://dev.azure.com/git-for-windows/git/_apis/build/builds
id=$(curl "$urlbase?definitions=22&statusFilter=completed&resultFilter=succeeded&\$top=1" |
jq -r ".value[] | .id")
download_url="$(curl "$urlbase/$id/artifacts" |
jq -r '.value[] | select(.name == "git-sdk-64-minimal").resource.downloadUrl')"
curl --connect-timeout 10 --retry 5 --retry-delay 0 --retry-max-time 240 \
-o artifacts.zip "$download_url"

## Unzip and remove the artifact
unzip artifacts.zip
rm artifacts.zip
- uses: actions/checkout@v2
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: initialize vcpkg
uses: actions/checkout@v2
with:
Expand All @@ -203,75 +153,60 @@ jobs:
- name: add msbuild to PATH
uses: microsoft/setup-msbuild@v1
- name: copy dlls to root
shell: powershell
run: |
& compat\vcbuild\vcpkg_copy_dlls.bat release
if (!$?) { exit(1) }
shell: cmd
run: compat\vcbuild\vcpkg_copy_dlls.bat release
- name: generate Visual Studio solution
shell: bash
run: |
cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows \
-DMSGFMT_EXE=`pwd`/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
shell: powershell
shell: bash
env:
MSVC: 1
VCPKG_ROOT: ${{github.workspace}}\compat\vcbuild\vcpkg
run: |
& git-sdk-64-minimal\usr\bin\bash.exe -lc @"
mkdir -p artifacts &&
eval \"`$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)\"
"@
- name: upload build artifacts
uses: actions/upload-artifact@v1
mkdir -p artifacts &&
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
uses: actions/upload-artifact@v2
with:
name: vs-artifacts
path: artifacts
vs-test:
runs-on: windows-latest
needs: [vs-build, windows-build]
needs: vs-build
strategy:
fail-fast: false
matrix:
nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
steps:
- uses: actions/checkout@v1
- name: download git-sdk-64-minimal
uses: actions/download-artifact@v1
with:
name: git-sdk-64-minimal
path: ${{github.workspace}}/git-sdk-64-minimal/
- name: download build artifacts
uses: actions/download-artifact@v1
- uses: git-for-windows/setup-git-for-windows-sdk@v1
- name: download tracked files and build artifacts
uses: actions/download-artifact@v2
with:
name: vs-artifacts
path: ${{github.workspace}}
- name: extract build artifacts
- name: extract tracked files and build artifacts
shell: bash
run: tar xf artifacts.tar.gz
run: tar xf artifacts.tar.gz && tar xf tracked.tar.gz
- name: test
shell: powershell
shell: bash
env:
MSYSTEM: MINGW64
NO_SVN_TESTS: 1
GIT_TEST_SKIP_REBASE_P: 1
run: |
& .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
# Let Git ignore the SDK and the test-cache
printf '%s\n' /git-sdk-64-minimal/ /test-cache/ >>.git/info/exclude

ci/run-test-slice.sh ${{matrix.nr}} 10
"@
run: ci/run-test-slice.sh ${{matrix.nr}} 10
- name: ci/print-test-failures.sh
if: failure()
shell: powershell
run: |
& .\git-sdk-64-minimal\usr\bin\bash.exe -lc ci/print-test-failures.sh
shell: bash
run: ci/print-test-failures.sh
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
uses: actions/upload-artifact@v1
uses: actions/upload-artifact@v2
with:
name: failed-tests-windows
path: ${{env.FAILED_TEST_ARTIFACTS}}
Expand Down Expand Up @@ -302,14 +237,14 @@ jobs:
jobname: ${{matrix.vector.jobname}}
runs-on: ${{matrix.vector.pool}}
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v2
- run: ci/install-dependencies.sh
- run: ci/run-build-and-tests.sh
- run: ci/print-test-failures.sh
if: failure()
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
uses: actions/upload-artifact@v1
uses: actions/upload-artifact@v2
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
Expand All @@ -336,7 +271,7 @@ jobs:
if: failure()
- name: Upload failed tests' directories
if: failure() && env.FAILED_TEST_ARTIFACTS != ''
uses: actions/upload-artifact@v1
uses: actions/upload-artifact@v2
with:
name: failed-tests-${{matrix.vector.jobname}}
path: ${{env.FAILED_TEST_ARTIFACTS}}
Expand All @@ -347,7 +282,7 @@ jobs:
jobname: StaticAnalysis
runs-on: ubuntu-18.04
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v2
- run: ci/install-dependencies.sh
- run: ci/run-static-analysis.sh
documentation:
Expand All @@ -357,6 +292,6 @@ jobs:
jobname: Documentation
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v1
- uses: actions/checkout@v2
- run: ci/install-dependencies.sh
- run: ci/test-documentation.sh
5 changes: 4 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE
.PHONY: pot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We obviously do not want to bundle `.mo` files during `make
> artifacts-tar NO_GETTEXT`, but that was the case.

Should be:

    make artifacts-tar NO_GETTEXT=YesPlease

(Without the =<something> we try to find a "NO_GETTEXT" target)

> To fix that, go a step beyond just fixing the symptom, and simply
> define the lists of `.po` and `.mo` files as empty if `NO_GETTEXT` is
> set.

How about fixing the cause instead of the symptom...

> Helped-by: Matthias Aßhauer <mha1993@live.de>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Makefile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index c3565fc0f8f..04e852be015 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE
>  .PHONY: pot
>  pot: po/git.pot
>  
> +ifdef NO_GETTEXT
> +POFILES :=
> +MOFILES :=
> +else
>  POFILES := $(wildcard po/*.po)
>  MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
>  
> -ifndef NO_GETTEXT
>  all:: $(MOFILES)
>  endif

...i.e. this patch just seems like odd (ab)use of Makefile logic.

Later on in the artifacts-tar rule we rely on our immediate dependency
list in $^ to feed to "tar czf", and here we're going to set $(MOFILES)
to an empty list, just to later interpolate that empty list into that
list of dependencies.

Wouldn't the mores straightforward thing to do be the diff I've got at
the end here, perhaps with a preceding commit just for the split-up of
the dependency list?

This matches how we do things elsewhere, i.e. we don't ifdef e.g. this away:

    LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
    LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))

rather we keep the list as-is, and ifdef the actual addition of the
dependency, e.g.:

    ifndef NO_PERL
    all:: $(LIB_PERL_GEN)
    [...]
    endif

One reason we do it like this is because we *don't* want to forget what
the MOFILES were, because you want e.g. "make clean" to clean them up
(not that it matters in this case, we rm -rf po/build).

Doesn't matter much here, but following this pattern leads to subtle
"bugs", e.g. an outstanding issue in your 179227d6e21 (Optionally skip
linking/copying the built-ins, 2020-09-21) (which I noted on-list in
passing before, IIRC) where during a build we end up with stale
built-ins from a previous build in the build directory, because we
pruned the list during definition time, as opposed to adding an inverse
"I should remove this then" rule.

("bug" because it doesn't have any actual effect I know of other than
bothering me that I have e.g. a git-add in my build-dir still :)

diff --git a/Makefile b/Makefile
index c3565fc0f8f..7fb1d4b6caa 100644
--- a/Makefile
+++ b/Makefile
@@ -3146,9 +3146,18 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
 OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
 endif
 
-artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
-		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
-		$(MOFILES)
+ARTIFACTS_TAR =
+ARTIFACTS_TAR += GIT-BUILD-OPTIONS
+ARTIFACTS_TAR += $(ALL_COMMANDS_TO_INSTALL)
+ARTIFACTS_TAR += $(SCRIPT_LIB)
+ARTIFACTS_TAR += $(OTHER_PROGRAMS) 
+ARTIFACTS_TAR += $(TEST_PROGRAMS)
+ARTIFACTS_TAR += $(test_bindir_programs)
+ifndef NO_GETTEXT
+ARTIFACTS_TAR += $(MOFILES)
+endif
+
+artifacts-tar:: $(ARTIFACTS_TAR)
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
 		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
 	test -n "$(ARTIFACTS_DIRECTORY)"

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1394615223-1625439168=:8230
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi =C3=86var,

On Sun, 4 Jul 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:

> On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > We obviously do not want to bundle `.mo` files during `make
> > artifacts-tar NO_GETTEXT`, but that was the case.
>
> Should be:
>
>     make artifacts-tar NO_GETTEXT=3DYesPlease
>
> (Without the =3D<something> we try to find a "NO_GETTEXT" target)

Correct. Will fix.

> > To fix that, go a step beyond just fixing the symptom, and simply
> > define the lists of `.po` and `.mo` files as empty if `NO_GETTEXT` is
> > set.
>
> How about fixing the cause instead of the symptom...

Yes, from my point of view, I did that.

> > Helped-by: Matthias A=C3=9Fhauer <mha1993@live.de>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Makefile | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c3565fc0f8f..04e852be015 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE
> >  .PHONY: pot
> >  pot: po/git.pot
> >
> > +ifdef NO_GETTEXT
> > +POFILES :=3D
> > +MOFILES :=3D
> > +else
> >  POFILES :=3D $(wildcard po/*.po)
> >  MOFILES :=3D $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,=
$(POFILES))
> >
> > -ifndef NO_GETTEXT
> >  all:: $(MOFILES)
> >  endif
>
> ...i.e. this patch just seems like odd (ab)use of Makefile logic.
>
> Later on in the artifacts-tar rule we rely on our immediate dependency
> list in $^ to feed to "tar czf", and here we're going to set $(MOFILES)
> to an empty list, just to later interpolate that empty list into that
> list of dependencies.
>
> Wouldn't the mores straightforward thing to do be the diff I've got at
> the end here, perhaps with a preceding commit just for the split-up of
> the dependency list?
>
> This matches how we do things elsewhere, i.e. we don't ifdef e.g. this a=
way:
>
>     LIB_PERL :=3D $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm p=
erl/Git/*/*/*.pm)
>     LIB_PERL_GEN :=3D $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PER=
L))
>
> rather we keep the list as-is, and ifdef the actual addition of the
> dependency, e.g.:
>
>     ifndef NO_PERL
>     all:: $(LIB_PERL_GEN)
>     [...]
>     endif
>
> One reason we do it like this is because we *don't* want to forget what
> the MOFILES were, because you want e.g. "make clean" to clean them up
> (not that it matters in this case, we rm -rf po/build).

We don't need to be careful about cleaning files we did not generate in
the first place.

Your suggestion amounts to unnecessary work. If we asked for NO_GETTEXT,
why bother generating the list of `.po` files at all? (Rhetorical
question; The answer is "we don't need to".)

> Doesn't matter much here, but following this pattern leads to subtle
> "bugs", e.g. an outstanding issue in your 179227d6e21 (Optionally skip
> linking/copying the built-ins, 2020-09-21) (which I noted on-list in
> passing before, IIRC) where during a build we end up with stale
> built-ins from a previous build in the build directory, because we
> pruned the list during definition time, as opposed to adding an inverse
> "I should remove this then" rule.
>
> ("bug" because it doesn't have any actual effect I know of other than
> bothering me that I have e.g. a git-add in my build-dir still :)
>
> diff --git a/Makefile b/Makefile
> index c3565fc0f8f..7fb1d4b6caa 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3146,9 +3146,18 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
>  OTHER_PROGRAMS +=3D $(shell echo *.dll t/helper/*.dll)
>  endif
>
> -artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRA=
MS) \
> -		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
> -		$(MOFILES)
> +ARTIFACTS_TAR =3D
> +ARTIFACTS_TAR +=3D GIT-BUILD-OPTIONS
> +ARTIFACTS_TAR +=3D $(ALL_COMMANDS_TO_INSTALL)
> +ARTIFACTS_TAR +=3D $(SCRIPT_LIB)
> +ARTIFACTS_TAR +=3D $(OTHER_PROGRAMS)
> +ARTIFACTS_TAR +=3D $(TEST_PROGRAMS)
> +ARTIFACTS_TAR +=3D $(test_bindir_programs)
> +ifndef NO_GETTEXT
> +ARTIFACTS_TAR +=3D $(MOFILES)
> +endif
> +
> +artifacts-tar:: $(ARTIFACTS_TAR)

Apart from going out of its way to retain the construction of the `.po`
file list (which is totally pointless when operating under `NO_GETTEXT`),
this is also a sore to my eyes. So I won't do that.

Thank you for trying to assist in improving this patch series,
Dscho

>  	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
>  		SHELL_PATH=3D'$(SHELL_PATH_SQ)' PERL_PATH=3D'$(PERL_PATH_SQ)'
>  	test -n "$(ARTIFACTS_DIRECTORY)"
>
>

--8323328-1394615223-1625439168=:8230--

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Mon, Jul 05 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Sun, 4 Jul 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Sat, Jul 03 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> >
>> > We obviously do not want to bundle `.mo` files during `make
>> > artifacts-tar NO_GETTEXT`, but that was the case.
>>
>> Should be:
>>
>>     make artifacts-tar NO_GETTEXT=YesPlease
>>
>> (Without the =<something> we try to find a "NO_GETTEXT" target)
>
> Correct. Will fix.
>
>> > To fix that, go a step beyond just fixing the symptom, and simply
>> > define the lists of `.po` and `.mo` files as empty if `NO_GETTEXT` is
>> > set.
>>
>> How about fixing the cause instead of the symptom...
>
> Yes, from my point of view, I did that.
>
>> > Helped-by: Matthias Aßhauer <mha1993@live.de>
>> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> > ---
>> >  Makefile | 5 ++++-
>> >  1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Makefile b/Makefile
>> > index c3565fc0f8f..04e852be015 100644
>> > --- a/Makefile
>> > +++ b/Makefile
>> > @@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE
>> >  .PHONY: pot
>> >  pot: po/git.pot
>> >
>> > +ifdef NO_GETTEXT
>> > +POFILES :=
>> > +MOFILES :=
>> > +else
>> >  POFILES := $(wildcard po/*.po)
>> >  MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))
>> >
>> > -ifndef NO_GETTEXT
>> >  all:: $(MOFILES)
>> >  endif
>>
>> ...i.e. this patch just seems like odd (ab)use of Makefile logic.
>>
>> Later on in the artifacts-tar rule we rely on our immediate dependency
>> list in $^ to feed to "tar czf", and here we're going to set $(MOFILES)
>> to an empty list, just to later interpolate that empty list into that
>> list of dependencies.
>>
>> Wouldn't the mores straightforward thing to do be the diff I've got at
>> the end here, perhaps with a preceding commit just for the split-up of
>> the dependency list?
>>
>> This matches how we do things elsewhere, i.e. we don't ifdef e.g. this away:
>>
>>     LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm)
>>     LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL))
>>
>> rather we keep the list as-is, and ifdef the actual addition of the
>> dependency, e.g.:
>>
>>     ifndef NO_PERL
>>     all:: $(LIB_PERL_GEN)
>>     [...]
>>     endif
>>
>> One reason we do it like this is because we *don't* want to forget what
>> the MOFILES were, because you want e.g. "make clean" to clean them up
>> (not that it matters in this case, we rm -rf po/build).
>
> We don't need to be careful about cleaning files we did not generate in
> the first place.
>
> Your suggestion amounts to unnecessary work. If we asked for NO_GETTEXT,
> why bother generating the list of `.po` files at all? (Rhetorical
> question; The answer is "we don't need to".)

I'm not saying that you in the Windows CI job generated them, but that
in general we want to support doing these in sequence:

    make NO_GETTEXT=Y <target>
    make NO_GETTEXT= <target>

...

>> Doesn't matter much here, but following this pattern leads to subtle
>> "bugs", e.g. an outstanding issue in your 179227d6e21 (Optionally skip
>> linking/copying the built-ins, 2020-09-21) (which I noted on-list in
>> passing before, IIRC) where during a build we end up with stale
>> built-ins from a previous build in the build directory, because we
>> pruned the list during definition time, as opposed to adding an inverse
>> "I should remove this then" rule.
>>
>> ("bug" because it doesn't have any actual effect I know of other than
>> bothering me that I have e.g. a git-add in my build-dir still :)
>>
>> diff --git a/Makefile b/Makefile
>> index c3565fc0f8f..7fb1d4b6caa 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -3146,9 +3146,18 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
>>  OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
>>  endif
>>
>> -artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
>> -		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
>> -		$(MOFILES)
>> +ARTIFACTS_TAR =
>> +ARTIFACTS_TAR += GIT-BUILD-OPTIONS
>> +ARTIFACTS_TAR += $(ALL_COMMANDS_TO_INSTALL)
>> +ARTIFACTS_TAR += $(SCRIPT_LIB)
>> +ARTIFACTS_TAR += $(OTHER_PROGRAMS)
>> +ARTIFACTS_TAR += $(TEST_PROGRAMS)
>> +ARTIFACTS_TAR += $(test_bindir_programs)
>> +ifndef NO_GETTEXT
>> +ARTIFACTS_TAR += $(MOFILES)
>> +endif
>> +
>> +artifacts-tar:: $(ARTIFACTS_TAR)
>
> Apart from going out of its way to retain the construction of the `.po`
> file list (which is totally pointless when operating under `NO_GETTEXT`),


..and that yes, generally speaking there *is* a point in doing
that. E.g. we have another discussion now on-list about incorrectly
spelled/copied config variables in po/*.po files.

It would be a natural thing to want to have some "lint" or "check"
target for that which used $(POFILES) as a source, and you'd not want
that:

    make check-pofiles

To do nothing under NO_GETTEXT=Y, but still use other Makefile
dependencies, e.g. use config-list.h as a source of truth.

To be clear I don't think anything's breaking now with your patch, I
just find the pattern of conflating the declaration of files in the
Makefile with the current logic of the rules that happen to need them
right now to be an anti-pattern.

> this is also a sore to my eyes. So I won't do that.

I agree that converting it is an eyesore, that's quite verbose, but it
makes any later patch much easier to read. You'll just need to add
line(s), not modify that big dependency list in-place.

> Thank you for trying to assist in improving this patch series,
> Dscho

>>  	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
>>  		SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
>>  	test -n "$(ARTIFACTS_DIRECTORY)"
>>
>>

pot: po/git.pot

ifdef NO_GETTEXT
POFILES :=
MOFILES :=
else
POFILES := $(wildcard po/*.po)
MOFILES := $(patsubst po/%.po,po/build/locale/%/LC_MESSAGES/git.mo,$(POFILES))

ifndef NO_GETTEXT
all:: $(MOFILES)
endif

Expand Down