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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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