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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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/