| Message ID | pull.1129.git.1654160735.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Integrate Scalar into the CI builds | expand |
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
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.
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
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
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? :)
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
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.