diff mbox series

[2/2] ci: also run the `scalar` tests

Message ID 6ad0d3d401da7787d0e7afb3f804b705731bf2dd.1654160735.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Integrate Scalar into the CI builds | expand

Commit Message

Johannes Schindelin June 2, 2022, 9:05 a.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

Since Scalar depends on `libgit.a`, it makes sense to ensure in the CI
and the PR builds that it does not get broken in case of industrious
refactorings of the core Git code (speaking from experience here).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 .github/workflows/main.yml | 15 +++++++++++++++
 ci/run-build-and-tests.sh  |  2 ++
 ci/run-test-slice.sh       |  5 +++++
 3 files changed, 22 insertions(+)

Comments

Ævar Arnfjörð Bjarmason June 2, 2022, 10:24 a.m. UTC | #1 
On Thu, Jun 02 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> [...]
>        run: . /etc/profile && ci/make-test-artifacts.sh artifacts
> +    - name: build Scalar
> +      shell: bash
> +      run: |
> +        make -C contrib/scalar &&
> +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> +        cp bin-wrappers/scalar artifacts/bin-wrappers/

This seems like it belongs in the Makfile and/or
ci/make-test-artifacts.sh, why does the GitHub-specific CI step need to
know how to appropriately move scalar build artifacts around?

More importantly, this is the "win build", which uses the Makefile (as
opposed to cmake-using vs-build).

We already invoke ci/make-test-artifacts.sh which does a "make
artifacts-tar", which builds them & packs the "git" binary etc. into a
tarball.

But here we're after-the-fact building the scalar binary itself (we
already built scalar.o), so at the end we're left with a tarball that
doesn't contain scalar, but then a ...

>      - name: zip up tracked files
>        run: git archive -o artifacts/tracked.tar.gz HEAD
>      - name: upload tracked files and build artifacts

...zip file that does because we manually re-do it here? Or maybe I'm
misunderstanding this....

> @@ -161,6 +168,8 @@ jobs:
>        run: compat\vcbuild\vcpkg_copy_dlls.bat release
>      - name: generate Visual Studio solution
>        shell: bash
> +      env:
> +        INCLUDE_SCALAR: YesPlease

...doesn't it make more sense to just have the Makefile understand
INCLUDE_SCALAR instead of keeping all this logic in main.yml.

I don't really see the logic of insisting that we can't put scalar
special-cases into our Makefile because it's "in contrib", but then
doing that in our main.yml CI.

>       run: |
>         mkdir -p artifacts &&
>         eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts NO_GETTEXT=YesPlease 2>&1 | grep ^tar)"
> +    - name: copy Scalar
> +      shell: bash
> +      run: |
> +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> +        cp bin-wrappers/scalar artifacts/bin-wrappers/

Ditto duplicated from above. As the context shows if the "artifacts-tar"
target knew how to include scalar this & the above wouldn't be needed,
we'd just run those shell commands.

>      - name: zip up tracked files
>        run: git archive -o artifacts/tracked.tar.gz HEAD
>      - name: upload tracked files and build artifacts
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 280dda7d285..661edb85d1b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -51,4 +51,6 @@ esac
>  make $MAKE_TARGETS
>  check_unignored_build_artifacts
>  
> +make -C contrib/scalar $MAKE_TARGETS

At the start of "make test" we create t/test-results, that also goes for
contrib/scalar's "test" phase.

Then if we have failures we keep it around, if not we rm -rf it, and
when we run "test" again we rm -rf the old one.

Have you tested what happens when e.g. one/both of scalar tests & "main"
tests fail? I haven't, but I'm fairly sure it's one of:

 * We racily run both, and they'll rm -rf or trip over one another's
   t/test-results.

 * We'll end up with one or the other, but we want the union of both
   (report on all failed tests & their output at the end)

 * We run one at a time (because we don't have "make -k"?), but now if
   e.g. one scalar test happens to fail (or the other way around) we
   won't spot failures in the tests we didn't run.

So I'm not sure, and haven't tested this, but what I can't see from
being familiar with the Makefile logic involve is how this would Just
Work and do what we want, but maybe I'm missing something.

I don't know if we touched on this in particular in past
rounds/discussions, but this edge case is one of many that led to [1],
i.e. if we simply run the tests in the top-level t/ (as other stuff in
contrib does, like the bash completion) we avoid all of these caveats.

> [...]

1. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/
Derrick Stolee June 2, 2022, 1:35 p.m. UTC | #2 
On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Since Scalar depends on `libgit.a`, it makes sense to ensure in the CI
> and the PR builds that it does not get broken in case of industrious
> refactorings of the core Git code (speaking from experience here).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  .github/workflows/main.yml | 15 +++++++++++++++
>  ci/run-build-and-tests.sh  |  2 ++
>  ci/run-test-slice.sh       |  5 +++++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index c35200defb9..785222aa7b3 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -91,6 +91,13 @@ jobs:
>          HOME: ${{runner.workspace}}
>          NO_PERL: 1
>        run: . /etc/profile && ci/make-test-artifacts.sh artifacts
> +    - name: build Scalar
> +      shell: bash
> +      run: |
> +        make -C contrib/scalar &&
> +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> +        cp bin-wrappers/scalar artifacts/bin-wrappers/

I see later you have a "copy Scalar" step which has some duplication
here. The only difference is that you have "make -C contrib/scalar".

Doesn't Scalar get built in our basic "make" build when the
environment includes INCLUDE_SCALAR=YesPlease? So, for that reason I
expected the environment to change, but not need this "make -C ..."

>      - name: zip up tracked files
>        run: git archive -o artifacts/tracked.tar.gz HEAD
>      - name: upload tracked files and build artifacts
> @@ -161,6 +168,8 @@ jobs:
>        run: compat\vcbuild\vcpkg_copy_dlls.bat release
>      - name: generate Visual Studio solution
>        shell: bash
> +      env:
> +        INCLUDE_SCALAR: YesPlease

This is a bit isolated. Is there a way to specify the environment
more generally?

>        run: |
>          cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows \
>          -DNO_GETTEXT=YesPlease -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
> @@ -174,6 +183,12 @@ jobs:
>        run: |
>          mkdir -p artifacts &&
>          eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts NO_GETTEXT=YesPlease 2>&1 | grep ^tar)"
> +    - name: copy Scalar
> +      shell: bash
> +      run: |
> +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> +        cp bin-wrappers/scalar artifacts/bin-wrappers/
>      - name: zip up tracked files
>        run: git archive -o artifacts/tracked.tar.gz HEAD
>      - name: upload tracked files and build artifacts
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index 280dda7d285..661edb85d1b 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -51,4 +51,6 @@ esac
>  make $MAKE_TARGETS
>  check_unignored_build_artifacts
>  
> +make -C contrib/scalar $MAKE_TARGETS
> +

Again, this should "just work" when using INCLUDE_SCALAR in the
environment, right?

>  save_good_tree
> diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
> index f8c2c3106a2..b741fd8f361 100755
> --- a/ci/run-test-slice.sh
> +++ b/ci/run-test-slice.sh
> @@ -14,4 +14,9 @@ make --quiet -C t T="$(cd t &&
>  	./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh |
>  	tr '\n' ' ')"
>  
> +if test 0 = "$1"
> +then
> +	make -C contrib/scalar test
> +fi
> +

This is still necessary for now.

Thanks,
-Stolee
Johannes Schindelin June 3, 2022, 10:33 a.m. UTC | #3 
Hi Stolee,

On Thu, 2 Jun 2022, Derrick Stolee wrote:

> On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Since Scalar depends on `libgit.a`, it makes sense to ensure in the CI
> > and the PR builds that it does not get broken in case of industrious
> > refactorings of the core Git code (speaking from experience here).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  .github/workflows/main.yml | 15 +++++++++++++++
> >  ci/run-build-and-tests.sh  |  2 ++
> >  ci/run-test-slice.sh       |  5 +++++
> >  3 files changed, 22 insertions(+)
> >
> > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> > index c35200defb9..785222aa7b3 100644
> > --- a/.github/workflows/main.yml
> > +++ b/.github/workflows/main.yml
> > @@ -91,6 +91,13 @@ jobs:
> >          HOME: ${{runner.workspace}}
> >          NO_PERL: 1
> >        run: . /etc/profile && ci/make-test-artifacts.sh artifacts
> > +    - name: build Scalar
> > +      shell: bash
> > +      run: |
> > +        make -C contrib/scalar &&
> > +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> > +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> > +        cp bin-wrappers/scalar artifacts/bin-wrappers/
>
> I see later you have a "copy Scalar" step which has some duplication
> here. The only difference is that you have "make -C contrib/scalar".
>
> Doesn't Scalar get built in our basic "make" build when the
> environment includes INCLUDE_SCALAR=YesPlease? So, for that reason I
> expected the environment to change, but not need this "make -C ..."

I originally did it that way, but somewhere along the refactoring, I
removed that `INCLUDE_SCALAR` conditional (except in the CMake
definition).

Hmm.

Now that you mention it, I guess it would be a good idea to reinstate it.
Let me play with that for a bit.

> >      - name: zip up tracked files
> >        run: git archive -o artifacts/tracked.tar.gz HEAD
> >      - name: upload tracked files and build artifacts
> > @@ -161,6 +168,8 @@ jobs:
> >        run: compat\vcbuild\vcpkg_copy_dlls.bat release
> >      - name: generate Visual Studio solution
> >        shell: bash
> > +      env:
> > +        INCLUDE_SCALAR: YesPlease
>
> This is a bit isolated. Is there a way to specify the environment more
> generally?

I did that on purpose, to limit the scope as much as possible.

It would of course be an option to set that environment variable for the
entire `vs-build` job. Or even for the entire workflow.

But I thought there was value in keeping the scope focused, so that
contributors who investigate failing builds could figure out quickly, say,
how to reproduce the issue locally.

>
> >        run: |
> >          cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows \
> >          -DNO_GETTEXT=YesPlease -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
> > @@ -174,6 +183,12 @@ jobs:
> >        run: |
> >          mkdir -p artifacts &&
> >          eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts NO_GETTEXT=YesPlease 2>&1 | grep ^tar)"
> > +    - name: copy Scalar
> > +      shell: bash
> > +      run: |
> > +        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
> > +        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
> > +        cp bin-wrappers/scalar artifacts/bin-wrappers/
> >      - name: zip up tracked files
> >        run: git archive -o artifacts/tracked.tar.gz HEAD
> >      - name: upload tracked files and build artifacts
> > diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> > index 280dda7d285..661edb85d1b 100755
> > --- a/ci/run-build-and-tests.sh
> > +++ b/ci/run-build-and-tests.sh
> > @@ -51,4 +51,6 @@ esac
> >  make $MAKE_TARGETS
> >  check_unignored_build_artifacts
> >
> > +make -C contrib/scalar $MAKE_TARGETS
> > +
>
> Again, this should "just work" when using INCLUDE_SCALAR in the
> environment, right?
>
> >  save_good_tree
> > diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
> > index f8c2c3106a2..b741fd8f361 100755
> > --- a/ci/run-test-slice.sh
> > +++ b/ci/run-test-slice.sh
> > @@ -14,4 +14,9 @@ make --quiet -C t T="$(cd t &&
> >  	./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh |
> >  	tr '\n' ' ')"
> >
> > +if test 0 = "$1"
> > +then
> > +	make -C contrib/scalar test
> > +fi
> > +
>
> This is still necessary for now.

Thank you for your review! I will play around with the idea to modify the
top-level `Makefile` so that a `make INCLUDE_SCALAR=YepOfCourse` would
automatically include Scalar. I am just concerned that this would already
open the discussion about taking Scalar out of `contrib/`, and I do want
to discourage this discussion before the remaining upstreamable patches
from https://github.com/microsoft/git/pull/479/commits made it into Git's
main branch (except of course the patch to move Scalar out of `contrib`,
https://github.com/microsoft/git/pull/479/commits/0e7b7653b29a).

Thanks,
Dscho
diff mbox series

Patch

diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index c35200defb9..785222aa7b3 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -91,6 +91,13 @@  jobs:
         HOME: ${{runner.workspace}}
         NO_PERL: 1
       run: . /etc/profile && ci/make-test-artifacts.sh artifacts
+    - name: build Scalar
+      shell: bash
+      run: |
+        make -C contrib/scalar &&
+        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
+        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
+        cp bin-wrappers/scalar artifacts/bin-wrappers/
     - name: zip up tracked files
       run: git archive -o artifacts/tracked.tar.gz HEAD
     - name: upload tracked files and build artifacts
@@ -161,6 +168,8 @@  jobs:
       run: compat\vcbuild\vcpkg_copy_dlls.bat release
     - name: generate Visual Studio solution
       shell: bash
+      env:
+        INCLUDE_SCALAR: YesPlease
       run: |
         cmake `pwd`/contrib/buildsystems/ -DCMAKE_PREFIX_PATH=`pwd`/compat/vcbuild/vcpkg/installed/x64-windows \
         -DNO_GETTEXT=YesPlease -DPERL_TESTS=OFF -DPYTHON_TESTS=OFF -DCURL_NO_CURL_CMAKE=ON
@@ -174,6 +183,12 @@  jobs:
       run: |
         mkdir -p artifacts &&
         eval "$(make -n artifacts-tar INCLUDE_DLLS_IN_ARTIFACTS=YesPlease ARTIFACTS_DIRECTORY=artifacts NO_GETTEXT=YesPlease 2>&1 | grep ^tar)"
+    - name: copy Scalar
+      shell: bash
+      run: |
+        mkdir -p artifacts/bin-wrappers artifacts/contrib/scalar &&
+        cp contrib/scalar/scalar.exe artifacts/contrib/scalar/ &&
+        cp bin-wrappers/scalar artifacts/bin-wrappers/
     - name: zip up tracked files
       run: git archive -o artifacts/tracked.tar.gz HEAD
     - name: upload tracked files and build artifacts
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 280dda7d285..661edb85d1b 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -51,4 +51,6 @@  esac
 make $MAKE_TARGETS
 check_unignored_build_artifacts
 
+make -C contrib/scalar $MAKE_TARGETS
+
 save_good_tree
diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
index f8c2c3106a2..b741fd8f361 100755
--- a/ci/run-test-slice.sh
+++ b/ci/run-test-slice.sh
@@ -14,4 +14,9 @@  make --quiet -C t T="$(cd t &&
 	./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh |
 	tr '\n' ' ')"
 
+if test 0 = "$1"
+then
+	make -C contrib/scalar test
+fi
+
 check_unignored_build_artifacts