ci: speed-up the Windows parts of our GitHub workflow
cover ci: speed-up the Windows parts of our GitHub workflow
This patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action
[https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for Windows is running with this Action for a while now, getting all the kinks out).
This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.
Dennis Ameling (1):
ci(vs-build): build with NO_GETTEXT
Johannes Schindelin (5):
ci: use the new GitHub Action to download git-sdk-64-minimal ci (vs-build): use cmd to copy the DLLs, not powershell
ci: upgrade to using actions/{up,down}load-artifacts v2 ci(windows): transfer also the Git-tracked files to the test jobs
ci: accelerate the checkout
.github/workflows/main.yml | 157 +++++++++++-------------------------- 1 file changed, 46 insertions(+), 111 deletions(-)
base-commit: 670b81a890388c60b7032a4f5b879f2ece8c4558
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-878%2Fdscho%2Fuse-setup-git-for-windows-sdk-action-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-878/dscho/use-setup-git-for-windows-sdk-action-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/878
Signature · 1 line
gitgitgadget
PATCH 1/6 ci: use the new GitHub Action to download git-sdk-64-minimal
From: Johannes Schindelin <[email protected]>
In our continuous builds, Windows is the odd cookie that requires a complete development environment to be downloaded because there is no suitable one installed by default on Windows.
Side note: technically, there _is_ a development environment present in GitHub Actions' build agents: MSYS2. But it differs from Git for Windows' SDK in subtle points, unfortunately enough so to prevent Git's test suite from running without failures.
Traditionally, we support downloading this environment (which we nicknamed git-sdk-64-minimal) via a PowerShell scriptlet that accesses the build artifacts of a dedicated Azure Pipeline (which packages a tiny subset of the full Git for Windows SDK, containing just enough to build Git and run its test suite).
This PowerShell script is unfortunately not very robust and sometimes fails due to network issues.
Of course, we could add code to detect that situation, wait a little, try again, if it fails again wait a little longer, lather, rinse and repeat.
Instead of doing all of this in Git's own .github/workflows/, though, let's offload this logic to the new GitHub Action at
https://github.com/marketplace/actions/setup-git-for-windows-sdk
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.
With this we can drop the home-rolled caching where we try to accelerate the test phase by uploading git-sdk-64-minimal as a workflow artifact after using it to build Git, and then download it as workflow artifact in the test phase.
Even better: the vs-test job no longer needs to depend on the windows-build job. The only reason it depended on it was to ensure that the git-sdk-64-minimal workflow artifact was available.
--- .github/workflows/main.yml | 103 +++++++------------------------------ 1 file changed, 19 insertions(+), 84 deletions(-)
.github/workflows/main.yml +19 -84
@@ -82,43 +82,18 @@ jobs: 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: 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 - "@ + run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts uses: actions/upload-artifact@v1 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]
@@ -136,25 +111,14 @@ jobs: - name: extract 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/ + - 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
@@ -165,27 +129,12 @@ jobs: 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: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2 with:
@@ -211,19 +160,17 @@ jobs: 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 + -DMSGFMT_EXE=C:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -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)\" - "@ + 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 with:
@@ -231,18 +178,14 @@ jobs: 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/ + - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts uses: actions/download-artifact@v1 with:
@@ -252,23 +195,15 @@ jobs: shell: bash run: tar xf artifacts.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
Signature · 1 line
gitgitgadget
PATCH 2/6 ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We use a .bat script to copy the DLLs in the vs-build job, and those type of scripts are native to CMD, not to PowerShell.
--- .github/workflows/main.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
.github/workflows/main.yml +2 -4
@@ -152,10 +152,8 @@ 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: |
Signature · 1 line
gitgitgadget
PATCH 3/6 ci: upgrade to using actions/{up,down}load-artifacts v2
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The GitHub Actions to upload/download workflow artifacts saw a major upgrade since Git's GitHub workflow was established. Let's use it.
--- .github/workflows/main.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
.github/workflows/main.yml +8 -8
@@ -90,7 +90,7 @@ jobs: NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts - uses: actions/upload-artifact@v1 + uses: actions/upload-artifact@v2 with: name: windows-artifacts path: artifacts
@@ -104,7 +104,7 @@ jobs: steps: - uses: actions/checkout@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: windows-artifacts path: ${{github.workspace}}
@@ -121,7 +121,7 @@ jobs: 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}}
@@ -170,7 +170,7 @@ jobs: 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 + uses: actions/upload-artifact@v2 with: name: vs-artifacts path: artifacts
@@ -185,7 +185,7 @@ jobs: - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: vs-artifacts path: ${{github.workspace}}
@@ -204,7 +204,7 @@ jobs: 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}}
@@ -242,7 +242,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}}
@@ -269,7 +269,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}}
Signature · 1 line
gitgitgadget
PATCH 4/6 ci(windows): transfer also the Git-tracked files to the test jobs
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Git's test suite is excruciatingly slow on Windows, mainly due to the fact that it executes a lot of shell script code, and that's simply not native to Windows.
To help with that, we established the pattern where the artifacts are first built in one job, and then multiple test jobs run in parallel using the artifacts built in the first job.
We take pains in transferring only the build outputs, and letting actions/checkout fill in the rest of the files.
One major downside of that strategy is that the test jobs might fail to check out the intended revision (e.g. because the branch has been updated while the build was running, as is frequently the case with the seen branch).
Let's transfer also the files tracked by Git, and skip the checkout step in the test jobs.
--- .github/workflows/main.yml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
.github/workflows/main.yml +12 -10
@@ -89,7 +89,9 @@ jobs: HOME: ${{runner.workspace}} NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - - name: upload build 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
@@ -102,15 +104,14 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - - name: download build artifacts + - 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 + run: tar xf artifacts.tar.gz && tar xf tracked.tar.gz - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: test shell: bash
@@ -169,7 +170,9 @@ jobs: run: | mkdir -p artifacts && eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)" - - name: upload build 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: vs-artifacts
@@ -182,16 +185,15 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - - name: download build artifacts + - 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: bash env:
Signature · 1 line
gitgitgadget
PATCH 5/6 ci(vs-build): build with NO_GETTEXT
In reply to Johannes Schindelin via GitGitGadgetFrom: Dennis Ameling <[email protected]>
We already build Git for Windows with NO_GETTEXT when compiling with GCC. Let's do the same with Visual C, too.
--- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
.github/workflows/main.yml +1 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
Quoted 16 lines · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.Signed-off-by Dennis Ameling <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-).github/workflows/main.yml +0
-0
.github/workflows/main.yml jobs: · line 165
Quoted 8 lines
.github/workflows/main.yml +1
-1@@ -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?
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
Quoted 32 lines · Ævar Arnfjörð Bjarmason
Quoted 16 lines · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.Signed-off-by Dennis Ameling <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-).github/workflows/main.yml +0
-0.github/workflows/main.yml jobs: · line 165
Quoted 8 lines
.github/workflows/main.yml +1
-1@@ -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 tarTo 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?
Quoted 8 lines
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.
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
In reply to Ævar Arnfjörð BjarmasonHi Ævar,
Quoted 18 lines · Ævar Arnfjörð Bjarmason
.github/workflows/main.yml jobs: · line 165
Quoted 13 lines · Dennis Ameling via GitGitGadget
.github/workflows/main.yml +1
-1@@ -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 tarTo 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
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
Quoted 43 lines · Johannes Schindelin
Hi Ævar,
Quoted 17 lines · Ævar Arnfjörð Bjarmason
.github/workflows/main.yml jobs: · line 165
Quoted 13 lines · Dennis Ameling via GitGitGadget
.github/workflows/main.yml +1
-1@@ -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 tarTo 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
msgfmtare 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-buildjob is really about verifying that we can build with that compiler, and _iff_ we usedmsgfmthere, it would not be Visual Studio's (because it doesn't have anymsgfmt) but the regular one from Git for Windows' SDK. Triply so because the regular one from Git for Windows' SDK_isactuallyused_in thewindows-buildjob, 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?)
Quoted 8 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
.mofiles aren't used, and that it would therefore be a waste of electrons to generate them in each and everyvs-buildrun.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
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
Hi Philip,
Quoted 29 lines · Philip Oakley
Quoted 20 lines · Johannes Schindelin
Quoted 17 lines · Ævar Arnfjörð Bjarmason
.github/workflows/main.yml jobs: · line 165
Quoted 13 lines · Dennis Ameling via GitGitGadget
.github/workflows/main.yml +1
-1@@ -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 tarTo a first approximation isn't the point of CI to test a diversity of setups?
Quoted 1 line
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.
Quoted 3 lines
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)
.mofiles.
You will be happy to hear that the fix was in v2 of this patch series (you responded to v1).
Ciao,
Dscho
Re: [PATCH 5/6] ci(vs-build): build with NO_GETTEXT
In reply to Johannes SchindelinQuoted 28 lines · Johannes Schindelin
Hi Ævar,
Quoted 18 lines · Ævar Arnfjörð Bjarmason
.github/workflows/main.yml jobs: · line 165
Quoted 13 lines · Dennis Ameling via GitGitGadget
.github/workflows/main.yml +1
-1@@ -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 tarTo 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
msgfmtare not even used, let alone tested.
Quoted 7 lines
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-buildjob is really about verifying that we can build with that compiler, and _iff_ we usedmsgfmthere, it would not be Visual Studio's (because it doesn't have anymsgfmt) 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)....
Quoted 4 lines
[...] Triply so because the regular one from Git for Windows' SDK
_isactuallyused_in thewindows-buildjob, 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.
Quoted 5 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.
Quoted 5 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
.mofiles aren't used, and that it would therefore be a waste of electrons to generate them in each and everyvs-buildrun.
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.
PATCH 6/6 ci: accelerate the checkout
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- .github/workflows/main.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
.github/workflows/main.yml +5 -5
@@ -81,7 +81,7 @@ jobs: if: needs.ci-config.outputs.enabled == 'yes' runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: build shell: bash
@@ -134,7 +134,7 @@ jobs: GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'" runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2
@@ -237,7 +237,7 @@ 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
@@ -282,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:
@@ -292,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
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 10 lines · writes:
This patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action
[https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for Windows is running with this Action for a while now, getting all the kinks out).This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.
https://github.com/git/git/actions/runs/991636423 is the run on the real 'seen' for the day, which fails vs-build. The same 'seen', with this topic excluded, seems to make everything pass and can be seen at https://github.com/git/git/actions/runs/991543592
CI/PR(seen) runs in the past few days all include this topic, and all of them have failed the vs-build job.
Thanks.
v2 cover ci: speed-up the Windows parts of our GitHub workflow
In reply to Johannes Schindelin via GitGitGadgetThis patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action
[https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for Windows is running with this Action for a while now, getting all the kinks out).
This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.
Changes since v1:
* Added a patch to fix make NO_GETTEXT=Yep artifacts-tar (not to include .mo files), as suggested by Matthias Aßauer in the GitGitGadget PR, which should fix the CI failure in seen that Junio pointed out. The bug was unhidden by mr/cmake fixing the CMake build (which ignored NO_GETTEXT before).
Dennis Ameling (1):
ci(vs-build): build with NO_GETTEXT
Johannes Schindelin (6):
ci: use the new GitHub Action to download git-sdk-64-minimal ci (vs-build): use cmd to copy the DLLs, not powershell
ci: upgrade to using actions/{up,down}load-artifacts v2 ci(windows): transfer also the Git-tracked files to the test jobs
artifacts-tar: respect NO_GETTEXT
ci: accelerate the checkout
.github/workflows/main.yml | 157 +++++++++++-------------------------- Makefile | 5 +- 2 files changed, 50 insertions(+), 112 deletions(-)
base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-878%2Fdscho%2Fuse-setup-git-for-windows-sdk-action-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-878/dscho/use-setup-git-for-windows-sdk-action-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/878
Range-diff vs v1:
1: 8da66c425a1 = 1: 2e4db688deb ci: use the new GitHub Action to download git-sdk-64-minimal
2: a913af64f9d = 2: 6b12fe2284c ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
3: 7930465e809 = 3: c256bbf4b1c ci: upgrade to using actions/{up,down}load-artifacts v2
4: 1520a9ffb57 = 4: 59dc44428fb ci(windows): transfer also the Git-tracked files to the test jobs
-: ----------- > 5: c31d2e7f44a artifacts-tar: respect NO_GETTEXT
5: a5084795ab0 = 6: 8bab4c17b8a ci(vs-build): build with NO_GETTEXT
6: e772d8f1d9b = 7: 88a44863cd0 ci: accelerate the checkoutSignature · 1 line
gitgitgadget
PATCH v2 1/7 ci: use the new GitHub Action to download git-sdk-64-minimal
From: Johannes Schindelin <[email protected]>
In our continuous builds, Windows is the odd cookie that requires a complete development environment to be downloaded because there is no suitable one installed by default on Windows.
Side note: technically, there _is_ a development environment present in GitHub Actions' build agents: MSYS2. But it differs from Git for Windows' SDK in subtle points, unfortunately enough so to prevent Git's test suite from running without failures.
Traditionally, we support downloading this environment (which we nicknamed git-sdk-64-minimal) via a PowerShell scriptlet that accesses the build artifacts of a dedicated Azure Pipeline (which packages a tiny subset of the full Git for Windows SDK, containing just enough to build Git and run its test suite).
This PowerShell script is unfortunately not very robust and sometimes fails due to network issues.
Of course, we could add code to detect that situation, wait a little, try again, if it fails again wait a little longer, lather, rinse and repeat.
Instead of doing all of this in Git's own .github/workflows/, though, let's offload this logic to the new GitHub Action at
https://github.com/marketplace/actions/setup-git-for-windows-sdk
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.
With this we can drop the home-rolled caching where we try to accelerate the test phase by uploading git-sdk-64-minimal as a workflow artifact after using it to build Git, and then download it as workflow artifact in the test phase.
Even better: the vs-test job no longer needs to depend on the windows-build job. The only reason it depended on it was to ensure that the git-sdk-64-minimal workflow artifact was available.
--- .github/workflows/main.yml | 103 +++++++------------------------------ 1 file changed, 19 insertions(+), 84 deletions(-)
.github/workflows/main.yml +19 -84
@@ -82,43 +82,18 @@ jobs: 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: 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 - "@ + run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts uses: actions/upload-artifact@v1 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]
@@ -136,25 +111,14 @@ jobs: - name: extract 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/ + - 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
@@ -165,27 +129,12 @@ jobs: 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: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2 with:
@@ -211,19 +160,17 @@ jobs: 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 + -DMSGFMT_EXE=C:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -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)\" - "@ + 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 with:
@@ -231,18 +178,14 @@ jobs: 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/ + - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts uses: actions/download-artifact@v1 with:
@@ -252,23 +195,15 @@ jobs: shell: bash run: tar xf artifacts.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
Signature · 1 line
gitgitgadget
PATCH v2 2/7 ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We use a .bat script to copy the DLLs in the vs-build job, and those type of scripts are native to CMD, not to PowerShell.
--- .github/workflows/main.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
.github/workflows/main.yml +2 -4
@@ -152,10 +152,8 @@ 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: |
Signature · 1 line
gitgitgadget
PATCH v2 3/7 ci: upgrade to using actions/{up,down}load-artifacts v2
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The GitHub Actions to upload/download workflow artifacts saw a major upgrade since Git's GitHub workflow was established. Let's use it.
--- .github/workflows/main.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
.github/workflows/main.yml +8 -8
@@ -90,7 +90,7 @@ jobs: NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts - uses: actions/upload-artifact@v1 + uses: actions/upload-artifact@v2 with: name: windows-artifacts path: artifacts
@@ -104,7 +104,7 @@ jobs: steps: - uses: actions/checkout@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: windows-artifacts path: ${{github.workspace}}
@@ -121,7 +121,7 @@ jobs: 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}}
@@ -170,7 +170,7 @@ jobs: 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 + uses: actions/upload-artifact@v2 with: name: vs-artifacts path: artifacts
@@ -185,7 +185,7 @@ jobs: - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: vs-artifacts path: ${{github.workspace}}
@@ -204,7 +204,7 @@ jobs: 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}}
@@ -242,7 +242,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}}
@@ -269,7 +269,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}}
Signature · 1 line
gitgitgadget
PATCH v2 4/7 ci(windows): transfer also the Git-tracked files to the test jobs
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Git's test suite is excruciatingly slow on Windows, mainly due to the fact that it executes a lot of shell script code, and that's simply not native to Windows.
To help with that, we established the pattern where the artifacts are first built in one job, and then multiple test jobs run in parallel using the artifacts built in the first job.
We take pains in transferring only the build outputs, and letting actions/checkout fill in the rest of the files.
One major downside of that strategy is that the test jobs might fail to check out the intended revision (e.g. because the branch has been updated while the build was running, as is frequently the case with the seen branch).
Let's transfer also the files tracked by Git, and skip the checkout step in the test jobs.
--- .github/workflows/main.yml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
.github/workflows/main.yml +12 -10
@@ -89,7 +89,9 @@ jobs: HOME: ${{runner.workspace}} NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - - name: upload build 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
@@ -102,15 +104,14 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - - name: download build artifacts + - 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 + run: tar xf artifacts.tar.gz && tar xf tracked.tar.gz - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: test shell: bash
@@ -169,7 +170,9 @@ jobs: run: | mkdir -p artifacts && eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)" - - name: upload build 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: vs-artifacts
@@ -182,16 +185,15 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - - name: download build artifacts + - 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: bash env:
Signature · 1 line
gitgitgadget
PATCH v2 5/7 artifacts-tar: respect NO_GETTEXT
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We obviously do not want to bundle .mo files during make artifacts-tar NO_GETTEXT, but that was the case.
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.
--- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Makefile +4 -1
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT
Quoted 4 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
We obviously do not want to bundle
.mofiles duringmake 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)
Quoted 3 lines
To fix that, go a step beyond just fixing the symptom, and simply define the lists of
.poand.mofiles as empty ifNO_GETTEXTis set.
How about fixing the cause instead of the symptom...
Quoted 14 lines
Helped-by Matthias Aßhauer <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Makefile +0
-0@@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE .PHONY: pot pot: po/git.pot
Makefile po/git.pot: $(GENERATED_H) FORCE · line 2686
Quoted 10 lines
Makefile +4
-1+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)
[...]
endifOne 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 :)
Makefile +12 -3
@@ -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)"
Re: [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT
Hi Ævar,
Quoted 4 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
Quoted 8 lines
Quoted 2 lines
We obviously do not want to bundle
.mofiles duringmake 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.
Quoted 5 lines
Quoted 3 lines
To fix that, go a step beyond just fixing the symptom, and simply define the lists of
.poand.mofiles as empty ifNO_GETTEXTis set.How about fixing the cause instead of the symptom...
Yes, from my point of view, I did that.
Quoted 44 lines
Quoted 14 lines
Helped-by Matthias Aßhauer <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Makefile +0
-0@@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE .PHONY: pot pot: po/git.potMakefile po/git.pot: $(GENERATED_H) FORCE · line 2686
Quoted 10 lines
Makefile +4
-1+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
Makefilelogic.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.:
Quoted 8 lines
ifndef NO_PERL all:: $(LIB_PERL_GEN) [...] endifOne 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".)
Quoted 24 lines
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 :)
Makefile +2
-3@@ -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
Makefile ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),) · line 3160
Quoted 10 lines
Makefile +10
-0+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), 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
Quoted 5 lines
Makefile +0
-0$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \ SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' test -n "$(ARTIFACTS_DIRECTORY)"
Re: [PATCH v2 5/7] artifacts-tar: respect NO_GETTEXT
Quoted 80 lines · Johannes Schindelin
Hi Ævar,
Quoted 4 lines · Ævar Arnfjörð Bjarmason
Quoted 2 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
Quoted 8 lines
Quoted 2 lines
We obviously do not want to bundle
.mofiles duringmake 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.
Quoted 5 lines
Quoted 3 lines
To fix that, go a step beyond just fixing the symptom, and simply define the lists of
.poand.mofiles as empty ifNO_GETTEXTis set.How about fixing the cause instead of the symptom...
Yes, from my point of view, I did that.
Quoted 44 lines
Quoted 14 lines
Helped-by Matthias Aßhauer <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)Makefile +0
-0@@ -2675,10 +2675,13 @@ po/git.pot: $(GENERATED_H) FORCE .PHONY: pot pot: po/git.potMakefile po/git.pot: $(GENERATED_H) FORCE · line 2686
Quoted 10 lines
Makefile +4
-1+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
Makefilelogic.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.:
Quoted 8 lines
ifndef NO_PERL all:: $(LIB_PERL_GEN) [...] endifOne 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).
Quoted 6 lines
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.pofiles 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>
...
Quoted 29 lines
Quoted 29 lines
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 :)
Makefile +7
-3@@ -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)
Quoted 8 lines
Makefile ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),) · line 3160
Quoted 5 lines
Makefile +5
-0+ifndef NO_GETTEXT +ARTIFACTS_TAR += $(MOFILES) +endif + +artifacts-tar:: $(ARTIFACTS_TAR)Apart from going out of its way to retain the construction of the
.pofile list (which is totally pointless when operating underNO_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.
Quoted 1 line
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.
Quoted 8 lines
Thank you for trying to assist in improving this patch series, Dscho
Quoted 5 lines
Makefile +0
-0$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \ SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)' test -n "$(ARTIFACTS_DIRECTORY)"
PATCH v2 6/7 ci(vs-build): build with NO_GETTEXT
In reply to Johannes Schindelin via GitGitGadgetFrom: Dennis Ameling <[email protected]>
We already build Git for Windows with NO_GETTEXT when compiling with GCC. Let's do the same with Visual C, too.
--- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
.github/workflows/main.yml +1 -1
@@ -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
Signature · 1 line
gitgitgadget
PATCH v2 7/7 ci: accelerate the checkout
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- .github/workflows/main.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
.github/workflows/main.yml +5 -5
@@ -81,7 +81,7 @@ jobs: if: needs.ci-config.outputs.enabled == 'yes' runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: build shell: bash
@@ -134,7 +134,7 @@ jobs: GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'" runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2
@@ -237,7 +237,7 @@ 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
@@ -282,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:
@@ -292,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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 7/7] ci: accelerate the checkout
Quoted 9 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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@v2seems 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 :)
Re: [PATCH v2 7/7] ci: accelerate the checkout
Hi Ævar,
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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.
Quoted 7 lines
Quoted 3 lines
Note that
actions/checkout@v2seems 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.
Quoted 4 lines
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
v3 cover ci: speed-up the Windows parts of our GitHub workflow
In reply to Johannes Schindelin via GitGitGadgetThis patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action
[https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for Windows is running with this Action for a while now, getting all the kinks out).
This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.
Changes since v2:
* Made the handwaving make [...] NO_GETTEXT comment in the commit message of the patch "artifacts-tar: respect NO_GETTEXT" more explicit, by setting NO_GETTEXT to a bogus value as required by make.
* Added an explicit NO_GETTEXT=YesPlease to the make artifacts-tar invocation in the vs-build job, as well as an explanation in the corresponding commit message why this explicit mention is technically not required.
Changes since v1:
* Added a patch to fix make NO_GETTEXT=Yep artifacts-tar (not to include .mo files), as suggested by Matthias Aßauer in the GitGitGadget PR, which should fix the CI failure in seen that Junio pointed out. The bug was unhidden by mr/cmake fixing the CMake build (which ignored NO_GETTEXT before).
Dennis Ameling (1):
ci(vs-build): build with NO_GETTEXT
Johannes Schindelin (6):
ci: use the new GitHub Action to download git-sdk-64-minimal ci (vs-build): use cmd to copy the DLLs, not powershell
ci: upgrade to using actions/{up,down}load-artifacts v2 ci(windows): transfer also the Git-tracked files to the test jobs
artifacts-tar: respect NO_GETTEXT
ci: accelerate the checkout
.github/workflows/main.yml | 157 +++++++++++-------------------------- Makefile | 5 +- 2 files changed, 50 insertions(+), 112 deletions(-)
base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-878%2Fdscho%2Fuse-setup-git-for-windows-sdk-action-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-878/dscho/use-setup-git-for-windows-sdk-action-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/878
Range-diff vs v2:
1: 2e4db688deb = 1: 2e4db688deb ci: use the new GitHub Action to download git-sdk-64-minimal
2: 6b12fe2284c = 2: 6b12fe2284c ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
3: c256bbf4b1c = 3: c256bbf4b1c ci: upgrade to using actions/{up,down}load-artifacts v2
4: 59dc44428fb = 4: 59dc44428fb ci(windows): transfer also the Git-tracked files to the test jobs
5: c31d2e7f44a ! 5: 64f7b1f4e23 artifacts-tar: respect NO_GETTEXT @@ Commit message
artifacts-tar: respect NO_GETTEXT
We obviously do not want to bundle .mo files during `make
- artifacts-tar NO_GETTEXT`, but that was the case.
+ artifacts-tar NO_GETTEXT=Yep`, but that was the case.
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
6: 8bab4c17b8a ! 6: 2c4cd9dd1c8 ci(vs-build): build with NO_GETTEXT
@@ Commit message
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.
+
+ Helped-by: Matthias Aßhauer <[email protected]>
## .github/workflows/main.yml ##
@@ .github/workflows/main.yml: jobs:
- name: MSBuild
run: msbuild git.sln -property:Configuration=Release -property:Platform=x64 -maxCpuCount:4 -property:PlatformToolset=v142
- name: bundle artifact tar
+@@ .github/workflows/main.yml: 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
7: 88a44863cd0 = 7: db54bf9a779 ci: accelerate the checkout
Signature · 1 line
gitgitgadget
PATCH v3 1/7 ci: use the new GitHub Action to download git-sdk-64-minimal
From: Johannes Schindelin <[email protected]>
In our continuous builds, Windows is the odd cookie that requires a complete development environment to be downloaded because there is no suitable one installed by default on Windows.
Side note: technically, there _is_ a development environment present in GitHub Actions' build agents: MSYS2. But it differs from Git for Windows' SDK in subtle points, unfortunately enough so to prevent Git's test suite from running without failures.
Traditionally, we support downloading this environment (which we nicknamed git-sdk-64-minimal) via a PowerShell scriptlet that accesses the build artifacts of a dedicated Azure Pipeline (which packages a tiny subset of the full Git for Windows SDK, containing just enough to build Git and run its test suite).
This PowerShell script is unfortunately not very robust and sometimes fails due to network issues.
Of course, we could add code to detect that situation, wait a little, try again, if it fails again wait a little longer, lather, rinse and repeat.
Instead of doing all of this in Git's own .github/workflows/, though, let's offload this logic to the new GitHub Action at
https://github.com/marketplace/actions/setup-git-for-windows-sdk
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.
With this we can drop the home-rolled caching where we try to accelerate the test phase by uploading git-sdk-64-minimal as a workflow artifact after using it to build Git, and then download it as workflow artifact in the test phase.
Even better: the vs-test job no longer needs to depend on the windows-build job. The only reason it depended on it was to ensure that the git-sdk-64-minimal workflow artifact was available.
--- .github/workflows/main.yml | 103 +++++++------------------------------ 1 file changed, 19 insertions(+), 84 deletions(-)
.github/workflows/main.yml +19 -84
@@ -82,43 +82,18 @@ jobs: 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: 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 - "@ + run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts uses: actions/upload-artifact@v1 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]
@@ -136,25 +111,14 @@ jobs: - name: extract 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/ + - 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
@@ -165,27 +129,12 @@ jobs: 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: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2 with:
@@ -211,19 +160,17 @@ jobs: 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 + -DMSGFMT_EXE=C:/git-sdk-64-minimal/mingw64/bin/msgfmt.exe -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)\" - "@ + 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 with:
@@ -231,18 +178,14 @@ jobs: 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/ + - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts uses: actions/download-artifact@v1 with:
@@ -252,23 +195,15 @@ jobs: shell: bash run: tar xf artifacts.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
Signature · 1 line
gitgitgadget
Re: [PATCH v3 1/7] ci: use the new GitHub Action to download git-sdk-64-minimal
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 16 lines · writes:
This Action not only downloads and extracts git-sdk-64-minimal _outside_ the worktree (making it no longer necessary to meddle with
.gitignoreor.git/info/exclude), it also adds thebash.exeto thePATHand sets the environment variableMSYSTEM(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 regularbashscriptlets.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.
Quoted 7 lines
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.
PATCH v3 2/7 ci (vs-build): use `cmd` to copy the DLLs, not `powershell`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We use a .bat script to copy the DLLs in the vs-build job, and those type of scripts are native to CMD, not to PowerShell.
--- .github/workflows/main.yml | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
.github/workflows/main.yml +2 -4
@@ -152,10 +152,8 @@ 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: |
Signature · 1 line
gitgitgadget
PATCH v3 3/7 ci: upgrade to using actions/{up,down}load-artifacts v2
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
The GitHub Actions to upload/download workflow artifacts saw a major upgrade since Git's GitHub workflow was established. Let's use it.
--- .github/workflows/main.yml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
.github/workflows/main.yml +8 -8
@@ -90,7 +90,7 @@ jobs: NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - name: upload build artifacts - uses: actions/upload-artifact@v1 + uses: actions/upload-artifact@v2 with: name: windows-artifacts path: artifacts
@@ -104,7 +104,7 @@ jobs: steps: - uses: actions/checkout@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: windows-artifacts path: ${{github.workspace}}
@@ -121,7 +121,7 @@ jobs: 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}}
@@ -170,7 +170,7 @@ jobs: 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 + uses: actions/upload-artifact@v2 with: name: vs-artifacts path: artifacts
@@ -185,7 +185,7 @@ jobs: - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: download build artifacts - uses: actions/download-artifact@v1 + uses: actions/download-artifact@v2 with: name: vs-artifacts path: ${{github.workspace}}
@@ -204,7 +204,7 @@ jobs: 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}}
@@ -242,7 +242,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}}
@@ -269,7 +269,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}}
Signature · 1 line
gitgitgadget
PATCH v3 4/7 ci(windows): transfer also the Git-tracked files to the test jobs
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
Git's test suite is excruciatingly slow on Windows, mainly due to the fact that it executes a lot of shell script code, and that's simply not native to Windows.
To help with that, we established the pattern where the artifacts are first built in one job, and then multiple test jobs run in parallel using the artifacts built in the first job.
We take pains in transferring only the build outputs, and letting actions/checkout fill in the rest of the files.
One major downside of that strategy is that the test jobs might fail to check out the intended revision (e.g. because the branch has been updated while the build was running, as is frequently the case with the seen branch).
Let's transfer also the files tracked by Git, and skip the checkout step in the test jobs.
--- .github/workflows/main.yml | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
.github/workflows/main.yml +12 -10
@@ -89,7 +89,9 @@ jobs: HOME: ${{runner.workspace}} NO_PERL: 1 run: ci/make-test-artifacts.sh artifacts - - name: upload build 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
@@ -102,15 +104,14 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - - name: download build artifacts + - 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 + run: tar xf artifacts.tar.gz && tar xf tracked.tar.gz - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: test shell: bash
@@ -169,7 +170,9 @@ jobs: run: | mkdir -p artifacts && eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts 2>&1 | grep ^tar)" - - name: upload build 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: vs-artifacts
@@ -182,16 +185,15 @@ jobs: matrix: nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] steps: - - uses: actions/checkout@v1 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - - name: download build artifacts + - 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: bash env:
Signature · 1 line
gitgitgadget
PATCH v3 5/7 artifacts-tar: respect NO_GETTEXT
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We obviously do not want to bundle .mo files during make artifacts-tar NO_GETTEXT=Yep, but that was the case.
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.
--- Makefile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Makefile +4 -1
@@ -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
Signature · 1 line
gitgitgadget
PATCH v3 6/7 ci(vs-build): build with NO_GETTEXT
In reply to Johannes Schindelin via GitGitGadgetFrom: Dennis Ameling <[email protected]>
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.
--- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
.github/workflows/main.yml +2 -2
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT
Quoted 1 line · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
Re the v3 cover letter & my v2 comment:
Quoted 8 lines
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.Note that we do not technically _need_ to pass
NO_GETTEXTexplicitly in thatmake artifacts-tarinvocation because we do this whileMSVCis set (which will setuname_S := Windows, which in turn will setNO_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.
Quoted 29 lines
Signed-off-by Dennis Ameling <[email protected]>Helped-by Matthias Aßhauer <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-).github/workflows/main.yml +2
-2@@ -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
Re: [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT
Hi Ævar,
Quoted 9 lines · Ævar Arnfjörð Bjarmason
Quoted 1 line · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
Re the v3 cover letter & my v2 comment:
Quoted 3 lines
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.
Quoted 7 lines
Quoted 5 lines
Note that we do not technically _need_ to pass
NO_GETTEXTexplicitly in thatmake artifacts-tarinvocation because we do this whileMSVCis set (which will setuname_S := Windows, which in turn will setNO_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.
Quoted 5 lines
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
Quoted 8 lines
We already supply
NO_GETTEXTimplicitly 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.
Quoted 9 lines
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
Quoted 32 lines
Quoted 29 lines
Signed-off-by Dennis Ameling <[email protected]>Helped-by Matthias Aßhauer <[email protected]>Signed-off-by Johannes Schindelin <[email protected]>--- .github/workflows/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-).github/workflows/main.yml +2
-2@@ -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
Re: [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT
In reply to Dennis Ameling via GitGitGadgetQuoted 10 lines · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.Note that we do not technically _need_ to pass
NO_GETTEXTexplicitly in thatmake artifacts-tarinvocation because we do this whileMSVCis set (which will setuname_S := Windows, which in turn will setNO_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?
Re: [PATCH v3 6/7] ci(vs-build): build with NO_GETTEXT
Hi Junio,
Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Dennis Ameling via GitGitGadget
From: Dennis Ameling <[email protected]>
We already build Git for Windows with
NO_GETTEXTwhen compiling with GCC. Let's do the same with Visual C, too.
Quoted 7 lines
Quoted 5 lines
Note that we do not technically _need_ to pass
NO_GETTEXTexplicitly in thatmake artifacts-tarinvocation because we do this whileMSVCis set (which will setuname_S := Windows, which in turn will setNO_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
PATCH v3 7/7 ci: accelerate the checkout
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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.
--- .github/workflows/main.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
.github/workflows/main.yml +5 -5
@@ -81,7 +81,7 @@ jobs: if: needs.ci-config.outputs.enabled == 'yes' runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: build shell: bash
@@ -134,7 +134,7 @@ jobs: GIT_CONFIG_PARAMETERS: "'user.name=CI' 'user.email=ci@git'" runs-on: windows-latest steps: - - uses: actions/checkout@v1 + - uses: actions/checkout@v2 - uses: git-for-windows/setup-git-for-windows-sdk@v1 - name: initialize vcpkg uses: actions/checkout@v2
@@ -237,7 +237,7 @@ 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
@@ -282,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:
@@ -292,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
Signature · 1 line
gitgitgadget
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 13 lines · writes:
This patch series upgrades to newer versions of a couple GitHub Actions we use, and also streamlines the Windows jobs using the relatively new setup-git-for-windows-sdk Action
[https://github.com/marketplace/actions/setup-git-for-windows-sdk] (Git for Windows is running with this Action for a while now, getting all the kinks out).This patch series should also address the problem where seen was pushed so rapidly that the windows-test jobs failed because they no longer checked out the identical revision as the windows-build job.
Changes since v2:
Quoted 7 lines
* Made the handwaving make [...]
NO_GETTEXTcomment in the commit message of the patch "artifacts-tar: respectNO_GETTEXT" more explicit, by settingNO_GETTEXTto a bogus value as required by make.
* Added an explicit NO_GETTEXT=YesPlease to the make artifacts-tar invocation in the vs-build job, as well as an explanation in the corresponding commit message why this explicit mention is technically not required.
The above indeed helps the CI at GitHub. The last run with 'seen' can be seen at https://github.com/git/git/actions/runs/1005977569
Thanks.