git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jun 3, 2022, 10:33 UTC
Message-ID
<nycvar.QRO.7.76.6.2206031204470.349@tvgsbejvaqbjf.bet>
In-Reply-To
<3c6d4a5c-8b39-8771-f578-0ea3c5b57869@github.com>
Hi Stolee,
On Thu, 2 Jun 2022, Derrick Stolee wrote:
Show 36 quoted lines
> 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.

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
> > @@ -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.

Show 47 quoted lines
>
> >        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

Previous: Derrick StoleeNext: Derrick Stolee
Message 7 of 14 in “Integrate Scalar into the CI builds”
  1. 0/2 Integrate Scalar into the CI buildsJohannes Schindelin via GitGitGadget, Jun 2, 2022
  2. 1/2 cmake: optionally build `scalar`, tooJohannes Schindelin via GitGitGadget, Jun 2, 2022
  3. Ævar Arnfjörð BjarmasonJun 2, 2022
  4. 2/2 ci: also run the `scalar` testsJohannes Schindelin via GitGitGadget, Jun 2, 2022
  5. Ævar Arnfjörð BjarmasonJun 2, 2022
  6. Derrick StoleeJun 2, 2022
  7. Johannes SchindelinJun 3, 2022
  8. Derrick StoleeJun 2, 2022
  9. Ævar Arnfjörð BjarmasonJun 2, 2022
  10. Derrick StoleeJun 2, 2022
  11. Johannes SchindelinJun 3, 2022
  12. Ævar Arnfjörð BjarmasonJun 3, 2022
  13. Johannes SchindelinJun 13, 2022
  14. Ævar Arnfjörð BjarmasonJun 23, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.