Re: [PATCH 2/2] ci: also run the `scalar` tests
On Thu, Jun 02 2022, Johannes Schindelin via GitGitGadget wrote:
Show 10 quoted lines
> 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....
Show 6 quoted lines
> @@ -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.
Show 9 quoted lines
> 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.
Show 12 quoted lines
> - 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/
Johannes Schindelin via GitGitGadget
