Hi Danh,
On Sat, 25 Apr 2020, Danh Doan wrote:
> On 2020-04-24 04:01:37+0000, Sibi Siddharthan via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote:
> > From: Sibi Siddharthan <sibisiddharthan.github@xxxxxxxxx>
> >
> > @@ -98,6 +91,13 @@ jobs:
> > Remove-Item compat.zip
> > - name: add msbuild to PATH
> > uses: microsoft/setup-msbuild@v1.0.0
> > + - name: generate Visual Studio solution
> > + shell: powershell
> > + run: |
> > + & .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
> > + cmake . -DCMAKE_PREFIX_PATH=./compat/vcbuild/vcpkg/installed/x64-windows -DMSGFMT_EXE=./git-sdk-64-minimal/mingw64/bin/msgfmt.exe -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
> > + "@
> > + if (!$?) { exit(1) }
>
> If you intended to modified some steps to provide a better script for
> that step, I believe you should change in that step.
>
> If the order of those steps need to be changed, please clarify your
> reasoning!
It is the order that needs to be changed, indeed: The CMake step requires
the vcpkg artifacts (i.e. pre-built dependencies like curl), whereas the
previous `make vcxproj` did not require them.
And yes, that's good material for the commit message!
> > - name: MSBuild
> > run: msbuild git.sln -property:Configuration=Release -property:Platform=x64 -maxCpuCount:4 -property:PlatformToolset=v142
> > - name: bundle artifact tar
> > @@ -125,9 +125,9 @@ jobs:
> > nr: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]
> > steps:
> > - uses: actions/checkout@v1
> > - - name: download git-64-portable
> > + - name: download git-sdk-64-minimal
> > shell: bash
> > - run: a=git-64-portable && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
> > + run: a=git-sdk-64-minimal && mkdir -p $a && curl -# https://wingit.blob.core.windows.net/ci-artifacts/$a.tar.xz | tar -C $a -xJf -
> > - name: download build artifacts
> > uses: actions/download-artifact@v1
> > with:
> > @@ -136,23 +136,30 @@ jobs:
> > - name: extract build artifacts
> > shell: bash
> > run: tar xf artifacts.tar.gz
> > - - name: test (parallel)
> > + - name: test
>
> So the test couldn't be run in parallel anymore?
> It's a regression!
> And we don't need the matrix anymore!!!!!
>
> I wonder if Dscho's effort is wasted, he tried very hard to make
> those tests run in parallel.
No need to worry, it's still parallel.
The reason why it looks incorrect is that Sibi used the `windows-test` job
as an example rather than the `vs-test` one. For that reason, it is the
full `git-sdk-64-minimal` rather than the portable Git in which the tests
are run.
I would prefer to re-revert this again, to use the `(parallel)` and the
`git-64-portable` again.
> He even tried to re-order the matrix in GfW project to let longest
> test run first!
I did! And it was quite a bit of manual work because the times are not
really obvious from the logs, you have to script something to get at them.
In the end, it was not worth the effort, as the overall runtime was only
minimally faster, and I did not like how the order could become stale and
less helpful over time.
> > shell: powershell
> > env:
> > MSYSTEM: MINGW64
> > NO_SVN_TESTS: 1
> > GIT_TEST_SKIP_REBASE_P: 1
> > run: |
> > - & git-64-portable\git-cmd.exe --command=usr\bin\bash.exe -lc @"
> > - # Let Git ignore the SDK and the test-cache
> > - printf '%s\n' /git-64-portable/ /test-cache/ >>.git/info/exclude
> > + & .\git-sdk-64-minimal\usr\bin\bash.exe -lc @"
> > + # Let Git ignore the SDK
> > + printf '%s\n' /git-sdk-64-minimal/ >>.git/info/exclude
> >
> > - cd t &&
> > - PATH=\"`$PWD/helper:`$PATH\" &&
> > - test-tool.exe run-command testsuite --jobs=10 -V -x --write-junit-xml \
> > - `$(test-tool.exe path-utils slice-tests \
> > - ${{matrix.nr}} 10 t[0-9]*.sh)
> > + 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
>
> What is changed?
This also is a consequence of copy/editing the `windows-test` job instead
of the `vs-test` one: The portable Git does not bring enough stuff to run
`prove`, therefore we have to "roll our own runner" here.
Unless we use the substantially larger `git-sdk-64-minimal`. But the idea
here _was_ to test in as similar a fashion to developers on Windows: they
would not want to download the full Git for Windows SDK, but run the test
in the Bash of a regular Git for Windows instead.
Indeed, you could say that in a way, this entire patch series is about
trying to make it unnecessary for developers on Windows to download more
than 500MB just to start tinkering with the Git source code.
Or maybe the patch series is about more than just that, but that alone
would be reason enough to want it. And I think we do want it.
Ciao,
Dscho
