* [PATCH 0/2] Integrate Scalar into the CI builds @ 2022-06-02 9:05 Johannes Schindelin via GitGitGadget 2022-06-02 9:05 ` [PATCH 1/2] cmake: optionally build `scalar`, too Johannes Schindelin via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-06-02 9:05 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin 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 .github/workflows/main.yml | 15 +++++++++++++++ ci/run-build-and-tests.sh | 2 ++ ci/run-test-slice.sh | 5 +++++ contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++ 4 files changed, 36 insertions(+) base-commit: 15d8adccab9a3146b760b089df59ce3e7ca2b451 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1129%2Fdscho%2Fscalar-and-ci-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1129/dscho/scalar-and-ci-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1129 -- gitgitgadget ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] cmake: optionally build `scalar`, too 2022-06-02 9:05 [PATCH 0/2] Integrate Scalar into the CI builds Johannes Schindelin via GitGitGadget @ 2022-06-02 9:05 ` Johannes Schindelin via GitGitGadget 2022-06-02 10:18 ` Ævar Arnfjörð Bjarmason 2022-06-02 9:05 ` [PATCH 2/2] ci: also run the `scalar` tests Johannes Schindelin via GitGitGadget ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-06-02 9:05 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Johannes Schindelin From: Johannes Schindelin <johannes.schindelin@gmx.de> 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. Also, `scalar.c` includes Git's header files, which means that `scalar.c` needs to be compiled with the very same flags as `libgit.a` lest `scalar.o` and `libgit.a` have different ideas of, say, `platform_SHA_CTX`, which would naturally lead to memory corruption, crashes and quite tricky debugging (talking from experience). 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 <johannes.schindelin@gmx.de> --- contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 185f56f414f..c8a802463ba 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -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") @@ -980,6 +987,13 @@ string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}") string(REPLACE "@@PROG@@" "git-cvsserver" content "${content}") file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver ${content}) +if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "") + file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME) + string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}") + string(REPLACE "@@PROG@@" "contrib/scalar/scalar${EXE_EXTENSION}" content "${content}") + file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/scalar ${content}) +endif() + #options for configuring test options option(PERL_TESTS "Perform tests that use perl" ON) option(PYTHON_TESTS "Perform tests that use python" ON) -- gitgitgadget ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] cmake: optionally build `scalar`, too 2022-06-02 9:05 ` [PATCH 1/2] cmake: optionally build `scalar`, too Johannes Schindelin via GitGitGadget @ 2022-06-02 10:18 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-02 10:18 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin On Thu, Jun 02 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. Are you using "encapsulation" here synonymously with "[mostly] lives in a different file"? If not I don't really see what distinction you're trying to draw here. The contrib/scalar/Makefile "shells out" to the top-level MAkefile to build libgit.a & common-main.o, as well as scalar.o itself. > 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 <johannes.schindelin@gmx.de> > --- > contrib/buildsystems/CMakeLists.txt | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt > index 185f56f414f..c8a802463ba 100644 > --- a/contrib/buildsystems/CMakeLists.txt > +++ b/contrib/buildsystems/CMakeLists.txt > @@ -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") > @@ -980,6 +987,13 @@ string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}") > string(REPLACE "@@PROG@@" "git-cvsserver" content "${content}") > file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/git-cvsserver ${content}) > > +if(DEFINED ENV{INCLUDE_SCALAR} AND NOT ENV{INCLUDE_SCALAR} STREQUAL "") > + file(STRINGS ${CMAKE_SOURCE_DIR}/wrap-for-bin.sh content NEWLINE_CONSUME) > + string(REPLACE "@@BUILD_DIR@@" "${CMAKE_BINARY_DIR}" content "${content}") > + string(REPLACE "@@PROG@@" "contrib/scalar/scalar${EXE_EXTENSION}" content "${content}") > + file(WRITE ${CMAKE_BINARY_DIR}/bin-wrappers/scalar ${content}) > +endif() We've been over this before direction we should take to fully integrate scalar (see [1]), but leaving that aside this seems by any definition to be more "encapsulated" than what we have in Makefile & contrib/scalar/Makefile. I.e. this is isolated in its own sections, whereas in the Makefile case we have the top-level building scalar, contrib/scalar/Makefile shelling out to it, and then adding its own behavior etc. I don't think there's much point to re-visit the discussions around the time of [1], clearly you feel differently, But just on the commit message it seems to have been drafted from some earlier version. Perhaps it was based on the earlier version of the scalar series before I'd pointed out the Makefile dependency issues around scalar.o (which led you to have contrib/scalar/MAkefile "shell out")? 1. https://lore.kernel.org/git/patch-1.1-86fb8d56307-20211028T185016Z-avarab@gmail.com/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] ci: also run the `scalar` tests 2022-06-02 9:05 [PATCH 0/2] Integrate Scalar into the CI builds Johannes Schindelin via GitGitGadget 2022-06-02 9:05 ` [PATCH 1/2] cmake: optionally build `scalar`, too Johannes Schindelin via GitGitGadget @ 2022-06-02 9:05 ` Johannes Schindelin via GitGitGadget 2022-06-02 10:24 ` Ævar Arnfjörð Bjarmason 2022-06-02 13:35 ` Derrick Stolee 2022-06-02 14:00 ` [PATCH 0/2] Integrate Scalar into the CI builds Derrick Stolee 2022-06-13 21:03 ` Johannes Schindelin 3 siblings, 2 replies; 14+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2022-06-02 9:05 UTC (permalink / raw) To: git; +Cc: Johannes Schindelin, Johannes Schindelin 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/ - 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 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 + 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 + check_unignored_build_artifacts -- gitgitgadget ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ci: also run the `scalar` tests 2022-06-02 9:05 ` [PATCH 2/2] ci: also run the `scalar` tests Johannes Schindelin via GitGitGadget @ 2022-06-02 10:24 ` Ævar Arnfjörð Bjarmason 2022-06-02 13:35 ` Derrick Stolee 1 sibling, 0 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-02 10:24 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin 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/ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ci: also run the `scalar` tests 2022-06-02 9:05 ` [PATCH 2/2] ci: also run the `scalar` tests Johannes Schindelin via GitGitGadget 2022-06-02 10:24 ` Ævar Arnfjörð Bjarmason @ 2022-06-02 13:35 ` Derrick Stolee 2022-06-03 10:33 ` Johannes Schindelin 1 sibling, 1 reply; 14+ messages in thread From: Derrick Stolee @ 2022-06-02 13:35 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin 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 ..." > - 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? > 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. Thanks, -Stolee ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ci: also run the `scalar` tests 2022-06-02 13:35 ` Derrick Stolee @ 2022-06-03 10:33 ` Johannes Schindelin 0 siblings, 0 replies; 14+ messages in thread From: Johannes Schindelin @ 2022-06-03 10:33 UTC (permalink / raw) To: Derrick Stolee; +Cc: Johannes Schindelin via GitGitGadget, git 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-02 9:05 [PATCH 0/2] Integrate Scalar into the CI builds Johannes Schindelin via GitGitGadget 2022-06-02 9:05 ` [PATCH 1/2] cmake: optionally build `scalar`, too Johannes Schindelin via GitGitGadget 2022-06-02 9:05 ` [PATCH 2/2] ci: also run the `scalar` tests Johannes Schindelin via GitGitGadget @ 2022-06-02 14:00 ` Derrick Stolee 2022-06-02 14:13 ` Ævar Arnfjörð Bjarmason 2022-06-13 21:03 ` Johannes Schindelin 3 siblings, 1 reply; 14+ messages in thread From: Derrick Stolee @ 2022-06-02 14:00 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget, git; +Cc: Johannes Schindelin On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote: > 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. It's good to start running Scalar builds and tests during CI before moving from contrib/. We can establish a pattern that the code is not causing build failures, and demonstrate that the tests succeed consistently. Better to do that while still in the mode where we can easily reverse course. > 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. Thanks, -Stolee ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-02 14:00 ` [PATCH 0/2] Integrate Scalar into the CI builds Derrick Stolee @ 2022-06-02 14:13 ` Ævar Arnfjörð Bjarmason 2022-06-02 14:30 ` Derrick Stolee 0 siblings, 1 reply; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-02 14:13 UTC (permalink / raw) To: Derrick Stolee Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin On Thu, Jun 02 2022, Derrick Stolee wrote: > On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote: >> 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. "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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-02 14:13 ` Ævar Arnfjörð Bjarmason @ 2022-06-02 14:30 ` Derrick Stolee 2022-06-03 10:04 ` Johannes Schindelin 0 siblings, 1 reply; 14+ messages in thread From: Derrick Stolee @ 2022-06-02 14:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin On 6/2/2022 10:13 AM, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Jun 02 2022, Derrick Stolee wrote: > >> On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote: >>> 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. > > "a full remote"? Whoops. My brain is mixed up with the work I've been doing in remote.c. I meant "a full revert". > 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. > 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. Yes, it is important that we revisit these patches with the previous changes in mind. In particular, I don't see a single reference to INCLUDE_SCALAR in the tree at the 'next' branch. This is different from the build in the microsoft/git fork, which is where I've done all of my own Scalar development. Thanks, -Stolee ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-02 14:30 ` Derrick Stolee @ 2022-06-03 10:04 ` Johannes Schindelin 2022-06-03 10:36 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 14+ messages in thread From: Johannes Schindelin @ 2022-06-03 10:04 UTC (permalink / raw) To: Derrick Stolee Cc: Ævar Arnfjörð Bjarmason, Johannes Schindelin via GitGitGadget, git [-- Attachment #1: Type: text/plain, Size: 2486 bytes --] Hi Stolee, On Thu, 2 Jun 2022, Derrick Stolee wrote: > On 6/2/2022 10:13 AM, Ævar Arnfjörð Bjarmason wrote: > > > > On Thu, Jun 02 2022, Derrick Stolee wrote: > > > >> On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote: > >>> 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. 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. > > 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. Ciao, Dscho ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-03 10:04 ` Johannes Schindelin @ 2022-06-03 10:36 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-03 10:36 UTC (permalink / raw) To: Johannes Schindelin Cc: Derrick Stolee, Johannes Schindelin via GitGitGadget, git On Fri, Jun 03 2022, Johannes Schindelin wrote: > Hi Stolee, > > On Thu, 2 Jun 2022, Derrick Stolee wrote: > >> On 6/2/2022 10:13 AM, Ævar Arnfjörð Bjarmason wrote: >> > >> > On Thu, Jun 02 2022, Derrick Stolee wrote: >> > >> >> On 6/2/2022 5:05 AM, Johannes Schindelin via GitGitGadget wrote: >> >>> 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. > > 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. > >> > 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. As the person making that claim: Yes that is really misleading, sorry. I was under the false recollection that since we added it to $(OBJECTS) we built it by default, as in "make" was building it. It *is* of course built my "make objects" etc., but due to how our dependency tree works not to create "git", or even "libgit.a" (the dependency relationship there being the other way around). But an you point out (and I'd missed this, but it make sense in retrospect) I was (accidentally!) right in the "CI" part of that since we're including it in "make sparse", which is because we create *.sp files from everything we have a *.o for. As an aside re the "justify the complexity" the patch to "fix" that would be rather trivial: diff --git a/Makefile b/Makefile index 18ca6744a50..aae16d140a5 100644 --- a/Makefile +++ b/Makefile @@ -2966,7 +2966,7 @@ t/helper/test-%$X: t/helper/test-%.o GIT-LDFLAGS $(GITLIBS) $(REFTABLE_TEST_LIB) check-sha1:: t/helper/test-tool$X t/helper/test-sha1.sh -SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ)) +SP_OBJ = $(patsubst %.o,%.sp,$(filter-out $(SCALAR_OBJECTS),$(C_OBJ))) $(SP_OBJ): %.sp: %.c %.o $(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \ But (and I've noted this before) I think the better fix is to just properly integrate scalar. We (accidentally) have been building it by default, which the patches to integrate it sought to explictly avoid to avoid bothering anyone. But ... nobody's been bothered, so I think if anything this should be a data point suggesting that we're being overly careful in this case. I.e. that we don't need the many intermediate steps of adding special-cases to various components, when there seems to be unanimous agreement on the end-goal. Can't we just skip to that already? :) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-02 9:05 [PATCH 0/2] Integrate Scalar into the CI builds Johannes Schindelin via GitGitGadget ` (2 preceding siblings ...) 2022-06-02 14:00 ` [PATCH 0/2] Integrate Scalar into the CI builds Derrick Stolee @ 2022-06-13 21:03 ` Johannes Schindelin 2022-06-23 10:41 ` Ævar Arnfjörð Bjarmason 3 siblings, 1 reply; 14+ messages in thread From: Johannes Schindelin @ 2022-06-13 21:03 UTC (permalink / raw) To: Johannes Schindelin via GitGitGadget; +Cc: git, Victoria Dye Hi, On Thu, 2 Jun 2022, Johannes Schindelin via GitGitGadget wrote: > 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 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). Thanks, Dscho ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/2] Integrate Scalar into the CI builds 2022-06-13 21:03 ` Johannes Schindelin @ 2022-06-23 10:41 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 14+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-23 10:41 UTC (permalink / raw) To: Johannes Schindelin Cc: Johannes Schindelin via GitGitGadget, git, Victoria Dye On Mon, Jun 13 2022, Johannes Schindelin wrote: > On Thu, 2 Jun 2022, Johannes Schindelin via GitGitGadget wrote: > >> 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 > > 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). I think at that point we'd basically be talking about integrating some version of the patch I sent to do that back in October. I re-rolled it now, including finishing the CMake part that I punted on before: https://lore.kernel.org/git/cover-v2-0.1-00000000000-20220623T100554Z-avarab@gmail.com/ As noted in the CL I saw Victoria pushed out a WIP version that was taking the same approach, but as she apparently wasn't aware of the previous effort in the area was bound to still run into the same issues with the parts she missed, which that 1-patch series addresses. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-06-23 10:48 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-02 9:05 [PATCH 0/2] Integrate Scalar into the CI builds Johannes Schindelin via GitGitGadget 2022-06-02 9:05 ` [PATCH 1/2] cmake: optionally build `scalar`, too Johannes Schindelin via GitGitGadget 2022-06-02 10:18 ` Ævar Arnfjörð Bjarmason 2022-06-02 9:05 ` [PATCH 2/2] ci: also run the `scalar` tests Johannes Schindelin via GitGitGadget 2022-06-02 10:24 ` Ævar Arnfjörð Bjarmason 2022-06-02 13:35 ` Derrick Stolee 2022-06-03 10:33 ` Johannes Schindelin 2022-06-02 14:00 ` [PATCH 0/2] Integrate Scalar into the CI builds Derrick Stolee 2022-06-02 14:13 ` Ævar Arnfjörð Bjarmason 2022-06-02 14:30 ` Derrick Stolee 2022-06-03 10:04 ` Johannes Schindelin 2022-06-03 10:36 ` Ævar Arnfjörð Bjarmason 2022-06-13 21:03 ` Johannes Schindelin 2022-06-23 10:41 ` Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).