Integrate Scalar into the CI builds

From: Johannes Schindelin <[email protected]>

Unlike the Makefile-based build, the CMake configuration unfortunately does not let us easily encapsulate Scalar's build definition in the contrib/scalar/ subdirectory: The scalar executable needs to link in libgit.a and common-main.o, for example.

To alleviate that lack of encapsulation somewhat, we guard the Scalar parts in CMakeLists.txt via the INCLUDE_SCALAR environment variable. This not only allows the CMake-based build to exclude Scalar by default, but also gives better visual cues as to which sections are related to Scalar.

Signed-off-by Johannes Schindelin <[email protected]>

---
 contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

contrib/buildsystems/CMakeLists.txt +7 -0

index 185f56f414f..c8a802463ba 100644
@@ -753,6 +753,13 @@ if(CURL_FOUND)
 	endif()
 endif()
 
+if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "")
+	add_executable(scalar ${CMAKE_SOURCE_DIR}/contrib/scalar/scalar.c)
+	target_link_libraries(scalar common-main)
+	set_target_properties(scalar PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG ${CMAKE_BINARY_DIR}/contrib/scalar)
+	set_target_properties(scalar PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE ${CMAKE_BINARY_DIR}/contrib/scalar)
+endif()
+
 parse_makefile_for_executables(git_builtin_extra "BUILT_INS")
 
 option(SKIP_DASHED_BUILT_INS "Skip hardlinking the dashed versions of the built-ins")

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

[...]

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 .github/workflows/main.yml | 15 +++++++++++++++
 ci/run-build-and-tests.sh  |  2 ++
 ci/run-test-slice.sh       |  5 +++++
 3 files changed, 22 insertions(+)

.github/workflows/main.yml +0 -0

index c35200defb9..785222aa7b3 100644

.github/workflows/main.yml +6 -0

       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

ci/run-build-and-tests.sh +0 -0

index 280dda7d285..661edb85d1b 100755

ci/run-build-and-tests.sh +0 -0

 save_good_tree

ci/run-test-slice.sh +0 -0

index f8c2c3106a2..b741fd8f361 100755

Quoted 18 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 .github/workflows/main.yml | 15 +++++++++++++++
 ci/run-build-and-tests.sh  |  2 ++
 ci/run-test-slice.sh       |  5 +++++
 3 files changed, 22 insertions(+)

.github/workflows/main.yml +0 -0

index c35200defb9..785222aa7b3 100644

.github/workflows/main.yml jobs: · line 100

Quoted 10 lines

.github/workflows/main.yml +7 -0

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

Quoted 4 lines

.github/workflows/main.yml +0 -0

     - 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:

.github/workflows/main.yml jobs: · line 172

Quoted 5 lines

.github/workflows/main.yml +2 -0

       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?

Quoted 22 lines

.github/workflows/main.yml +6 -0

       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

ci/run-build-and-tests.sh +0 -0

index 280dda7d285..661edb85d1b 100755

ci/run-build-and-tests.sh esac · line 55

Quoted 5 lines

ci/run-build-and-tests.sh +2 -0

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

Quoted 9 lines

ci/run-build-and-tests.sh +0 -0

 save_good_tree

ci/run-test-slice.sh +0 -0

index f8c2c3106a2..b741fd8f361 100755
@@ -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' ' ')"
 

ci/run-test-slice.sh make --quiet -C t T="$(cd t && · line 21

Quoted 5 lines

ci/run-test-slice.sh +5 -0

+if test 0 = "$1"
+then
+	make -C contrib/scalar test
+fi
+

This is still necessary for now.

During the review of the initial Scalar patch series, it was suggested to include Scalar in Git's CI builds. Due to some conflicts, this was postponed to a later patch series: This patch series.

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

Quoted 6 lines · Johannes Schindelin via GitGitGadget

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

I expect that this won't be a full remote, since we will still want to exclude Scalar from the build without INCLUDE_SCALAR enabled.

Quoted 8 lines · Derrick Stolee

Quoted 6 lines · Johannes Schindelin via GitGitGadget

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

Quoted 2 lines

I expect that this won't be a full remote, since we will still want to exclude Scalar from the build without INCLUDE_SCALAR enabled.

"a full remote"?

Scalar (well, scalar.o, not scalar the binary) has been included in the default build (including CI) for a while now.

What we haven't been doing until this series it to link it with libgit.a or running its tests.

So perhaps that's what you mean, but in an earlier series it wasn't building scalar.o, and I remember there being some confusion on this point in the past, seemingly based on a mental model of the scalar patches that pre-dated the re-roll that eventually got merged.

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 8 lines · Derrick Stolee

Quoted 6 lines · Johannes Schindelin via GitGitGadget

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

Quoted 2 lines

Quoted 2 lines

I expect that this won't be a full remote, since we will still want to exclude Scalar from the build without INCLUDE_SCALAR enabled.

Quoted 2 lines

Scalar (well, scalar.o, not scalar the binary) has been included in the default build (including CI) for a while now.

I'm talking about scalar the binary being important. I'm glad that scalar.o has been built already.

Hi Stolee,

Quoted 12 lines · Derrick Stolee

Quoted 11 lines · Ævar Arnfjörð Bjarmason

Quoted 8 lines · Derrick Stolee

Quoted 6 lines · Johannes Schindelin via GitGitGadget

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

I expect that this won't be a full remote, since we will still want to exclude Scalar from the build without INCLUDE_SCALAR enabled.

We had this INCLUDE_SCALAR condition in microsoft/git for a while but since I got the sense that many regulars were in favor of treating scalar like a top-level command (similar to gitk), I've since changed the over-all course to compiling it unconditionally.

The only remnant is the CMake definition, and only in the transitory phase while Scalar is still in contrib/scalar/, and only because I could not find a better way to encapsulate it.

But yes, if we decide to go with the INCLUDE_SCALAR approach, it won't be a full remove/revert.

Quoted 5 lines

Quoted 2 lines

Scalar (well, scalar.o, not scalar the binary) has been included in the default build (including CI) for a while now.

I'm talking about scalar the binary being important. I'm glad that scalar.o has been built already.

These are the raw logs of the linux-gcc job of the most recent CI build of seen, as of time of writing:

https://github.com/git/git/commit/7f1978ce8bfe41074df4fc96ff7f2a28e5807fd1/checks/6718714644/logs

When I download those logs and then let my browser search for the term "scalar", it comes up empty, even if, say, "range-diff.o" is found. Which is exactly according to my plan: no part of Scalar is to be built unless explicitly asked for.

The only job that touches it is the static-analysis job, which is a bit unfortunate. But I cannot justify the complexity of the patch it would take to address that.

In other words: The statement that scalar.o is included in the default build, without any qualifying note about static-analysis, is quite misleading.

During the review of the initial Scalar patch series, it was suggested to include Scalar in Git's CI builds. Due to some conflicts, this was postponed to a later patch series: This patch series.

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

This patch series is based on js/scalar-diagnose.

Johannes Schindelin (2):
cmake: optionally build scalar, too
ci: also run the scalar tests

Quoted 11 lines · Johannes Schindelin via GitGitGadget

During the review of the initial Scalar patch series, it was suggested to include Scalar in Git's CI builds. Due to some conflicts, this was postponed to a later patch series: This patch series.

Note that the changes to the GitHub workflow are somewhat transient in
nature: Based on the feedback I received on the Git mailing list, I see some appetite for turning Scalar into a full-fledged top-level command in Git, similar to gitk. Therefore my current plan is to do exactly that in the end (and I already have patches lined up to that end). This will essentially revert the ci/run-build-and-tests.sh change in this patch series.

Quoted 5 lines

This patch series is based on js/scalar-diagnose.

Johannes Schindelin (2):
cmake: optionally build scalar, too
ci: also run the scalar tests

Upon further reflection, I would like to retract these patches for now. They do seem a poor fit within the Scalar story arc: in the end, they won't be needed anyway (after moving Scalar out of contrib/).

I talked to Victoria and she kindly agreed to drive the Scalar upstreaming from here (after v2.37.0, I imagine).