| Message ID | xmqq8rwu278d.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC/PATCH] Makefile: add test-all target | expand |
On 12/8/2021 3:04 PM, Junio C Hamano wrote: > We ship contrib/ stuff within our primary source tree but except for > the completion scripts that are tested from our primary test suite, > their test suites are not run in the CI. > > Teach the main Makefile a "test-extra" target, which goes into each > package in contrib/ whose Makefile has its own "test" target and > runs "make test" there. Add a "test-all" target to make it easy to > drive both the primary tests and these contrib tests from CI and use > it. > So, how about doing it this way? This is based on 'master' and does > not cover contrib/scalar, but if we want to go this route, it should > be trivial to do it on top of a merge of ab/ci-updates and js/scalar > into 'master'. Good idea? Terrible idea? Not good enough? > +# Additional tests from places in contrib/ that are prepared to take > +# "make -C $there test", but expects that the primary build is done > +# already. > +test-extra: all > + $(MAKE) -C contrib/diff-highlight test > + $(MAKE) -C contrib/mw-to-git test > + $(MAKE) -C contrib/subtree test I like how this is obviously extendible to include contrib/scalar in a later change, then remove it when Scalar moves. > +test-all:: test test-extra And this test-all implies that test runs before test-extra, so libgit.a is compiled appropriately. I think this approach looks good to me. > diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh > index cc62616d80..9da0f26665 100755 > --- i/ci/run-build-and-tests.sh > +++ w/ci/run-build-and-tests.sh > @@ -19,7 +19,7 @@ make > case "$jobname" in > linux-gcc) > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > - make test > + make test-all Since we are now building and testing things that we have not been testing recently, it is worth checking that we don't have any work to do to make this pass. I assume that you've run 'make test-all' on your own machine. It will be good to see what the full action reports (probably all good). Thanks, -Stolee
On Wed, Dec 08, 2021 at 12:04:50PM -0800, Junio C Hamano wrote: > > That is an interesting way to demonstrate how orthogonal the issues > > are, which in turn means that it is not such a big deal to add back > > the coverage to the part that goes to contrib/scalar/. As the actual > > implementation, it is a bit too icky, though. > > So, how about doing it this way? This is based on 'master' and does > not cover contrib/scalar, but if we want to go this route, it should > be trivial to do it on top of a merge of ab/ci-updates and js/scalar > into 'master'. Good idea? Terrible idea? Not good enough? I don't mind the general direction, but... > +# Additional tests from places in contrib/ that are prepared to take > +# "make -C $there test", but expects that the primary build is done > +# already. > +test-extra: all > + $(MAKE) -C contrib/diff-highlight test > + $(MAKE) -C contrib/mw-to-git test > + $(MAKE) -C contrib/subtree test I'm not sure of the quality of tests in some of the contrib stuff. The tests in diff-highlight worked for me when I added them, but it's not like I ever run them regularly, or that they've been tested on a wide variety of platforms. So I think this is as likely to cause somebody a headache due to a dumb portability problem or random bitrot as it is to actually find a bug. I guess test-extra wouldn't be run by default, but only via CI, so maybe that limits the blast radius sufficiently. For diff-highlight in particular, you need to have a working perl, so you'd probably want to at least wrap it with a NO_PERL ifndef. For mw-to-git, you need to have MediaWiki::API installed, though I think the tests at least notice this and skip everything if you don't. -Peff
Derrick Stolee <stolee@gmail.com> writes: >> +test-extra: all >> + $(MAKE) -C contrib/diff-highlight test >> + $(MAKE) -C contrib/mw-to-git test >> + $(MAKE) -C contrib/subtree test > > I like how this is obviously extendible to include contrib/scalar > in a later change, then remove it when Scalar moves. > >> +test-all:: test test-extra > > And this test-all implies that test runs before test-extra, so > libgit.a is compiled appropriately. I do not think this implies the ordering between the main test and the extra test. "make test-all" actually makes a confusing mess on the terminal by conflating outputs from the main test and tests run in contrib. But because test-extra depends on all, we are keeping the assumption that Makefiles in contrib/ may assume that the primary build has already been done. >> diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh >> index cc62616d80..9da0f26665 100755 >> --- i/ci/run-build-and-tests.sh >> +++ w/ci/run-build-and-tests.sh >> @@ -19,7 +19,7 @@ make >> case "$jobname" in >> linux-gcc) >> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main >> - make test >> + make test-all > > Since we are now building and testing things that we have not been > testing recently, it is worth checking that we don't have any work > to do to make this pass. I assume that you've run 'make test-all' > on your own machine. It will be good to see what the full action > reports (probably all good). Yes, I am tempted to queue this at the tip of 'seen'.
Jeff King <peff@peff.net> writes: > For diff-highlight in particular, you need to have a working perl, so > you'd probably want to at least wrap it with a NO_PERL ifndef. For > mw-to-git, you need to have MediaWiki::API installed, though I think the > tests at least notice this and skip everything if you don't. I know I locally lack MediaWiki::API and saw the test skipped everything. I do not know that would be true on a box without any perl, though.
On Wed, Dec 08 2021, Junio C Hamano wrote: > We ship contrib/ stuff within our primary source tree but except for > the completion scripts that are tested from our primary test suite, > their test suites are not run in the CI. > > Teach the main Makefile a "test-extra" target, which goes into each > package in contrib/ whose Makefile has its own "test" target and > runs "make test" there. Add a "test-all" target to make it easy to > drive both the primary tests and these contrib tests from CI and use > it. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Junio C Hamano <gitster@pobox.com> writes: > >> That is an interesting way to demonstrate how orthogonal the issues >> are, which in turn means that it is not such a big deal to add back >> the coverage to the part that goes to contrib/scalar/. As the actual >> implementation, it is a bit too icky, though. > > So, how about doing it this way? This is based on 'master' and does > not cover contrib/scalar, but if we want to go this route, it should > be trivial to do it on top of a merge of ab/ci-updates and js/scalar > into 'master'. Good idea? Terrible idea? Not good enough? With the caveat that I think the greater direction here makes no sense, i.e. scalar didn't need its own build system etc. in the first place, so having hack-upon-hack to fix various integration issues is clearly worse than just having it behave like everything else.... ... then yes, adding this to the top-level Makefile makes more sense.... > Makefile | 12 +++++++++++- > ci/run-build-and-tests.sh | 10 +++++----- > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git i/Makefile w/Makefile > index d56c0e4aad..ca14558e3c 100644 > --- i/Makefile > +++ w/Makefile > @@ -2878,10 +2878,20 @@ export TEST_NO_MALLOC_CHECK > test: all > $(MAKE) -C t/ all > > +# Additional tests from places in contrib/ that are prepared to take > +# "make -C $there test", but expects that the primary build is done > +# already. > +test-extra: all > + $(MAKE) -C contrib/diff-highlight test > + $(MAKE) -C contrib/mw-to-git test > + $(MAKE) -C contrib/subtree test > + > +test-all:: test test-extra > + > perf: all > $(MAKE) -C t/perf/ all > > -.PHONY: test perf > +.PHONY: test test-extra test-all perf > > .PRECIOUS: $(TEST_OBJS) Which, if we're nitpicking this would be better, i.e. it allows them to run in parallel, as they won't be defined by only one rule, and will be listede individuall in the test-all and test-extra prereqs: diff --git a/Makefile b/Makefile index d892dbc6c6e..3f47c9f58ad 100644 --- a/Makefile +++ b/Makefile @@ -2878,15 +2878,25 @@ export TEST_NO_MALLOC_CHECK test: all $(MAKE) -C t/ all +define TMPL_test-extra +TEST_EXTRA_TARGETS += test-$(1) +.PHONY: test-$(1) +test-$(1): all + $$(MAKE) -C $(1) test +endef + # Additional tests from places in contrib/ that are prepared to take # "make -C $there test", but expects that the primary build is done # already. -test-extra: all - $(MAKE) -C contrib/diff-highlight test - $(MAKE) -C contrib/mw-to-git test - $(MAKE) -C contrib/subtree test +$(eval $(call TMPL_test-extra,contrib/diff-highlight)) +$(eval $(call TMPL_test-extra,contrib/mw-to-git)) +$(eval $(call TMPL_test-extra,contrib/subtree)) + +.PHONY: test-extra +test-extra:: all $(TEST_EXTRA_TARGETS) -test-all:: test test-extra +.PHONY: test-all +test-all: test $(TEST_EXTRA_TARGETS) perf: all $(MAKE) -C t/perf/ all > diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh > index cc62616d80..9da0f26665 100755 > --- i/ci/run-build-and-tests.sh > +++ w/ci/run-build-and-tests.sh > @@ -19,7 +19,7 @@ make > case "$jobname" in > linux-gcc) > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main > - make test > + make test-all > [...] But I think we're expanding the scope quite a bit here. The reason we were talking about testing scalar by default is because it uses libgit.a, so it's not decoupled at all, whereas the "contrib" programs are only using the built "git" command. I think it would probably be good to test these anyway, but it's an argument beyond that which applies to scalar. I also share Jeff's general concerns that the other stuff in contrib may not be all that stable. But I don't see why we should be pursuing this direction of running certain tests in CI only, as opposed to just under "make test", that distinction is something new in js/scalar (before that we run libgit.a test *modes* in CI, but not a different set of tests).
Jeff King <peff@peff.net> writes: > I don't mind the general direction, but... > >> +# Additional tests from places in contrib/ that are prepared to take >> +# "make -C $there test", but expects that the primary build is done >> +# already. >> +test-extra: all >> + $(MAKE) -C contrib/diff-highlight test >> + $(MAKE) -C contrib/mw-to-git test >> + $(MAKE) -C contrib/subtree test > > I'm not sure of the quality of tests in some of the contrib stuff. The > tests in diff-highlight worked for me when I added them, but it's not > like I ever run them regularly, or that they've been tested on a wide > variety of platforms. > > So I think this is as likely to cause somebody a headache due to a dumb > portability problem or random bitrot as it is to actually find a bug. I > guess test-extra wouldn't be run by default, but only via CI, so maybe > that limits the blast radius sufficiently. Yeah, that is the exact thought I had when I did it. Anybody who is not aware of test target other than 'test' will not be hurt, and we explicitly make the CI aware of 'test-all' to trigger it. But as long as somebody bothered to write the tests, exercising them to reveal bitrot-bugs either in the tested contrib stuff or the tests themselves to be fixed or removed would be a good thing to do. An updated version of the posted patch is in 'seen' that also covers credential/netrc; https://github.com/git/git/runs/4465323829 shows the logs from its jobs. It is not particularly interesting that most of the jobs are marked as failed, as t1092 was broken the same way in my local test. What I found interesting from my quick scan of randomly chosen jobs are that (1) nobody seemed to have failed test-extra, and (2) nobody had mediawiki installed to test mw-to-git. So I am tempted to do test-extra: all $(MAKE) -C contrib/credential/netrc test $(MAKE) -C contrib/diff-highlight test : $(MAKE) -C contrib/mw-to-git test $(MAKE) -C contrib/subtree test in the topic itself, while adding $(MAKE) -C contrib/scalar test before the subtree test (alphabetically) when it is merged to 'seen' with the js/scalar topic.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> So, how about doing it this way? This is based on 'master' and does >> not cover contrib/scalar, but if we want to go this route, it should >> be trivial to do it on top of a merge of ab/ci-updates and js/scalar >> into 'master'. Good idea? Terrible idea? Not good enough? > > With the caveat that I think the greater direction here makes no sense, > i.e. scalar didn't need its own build system etc. in the first place, so > having hack-upon-hack to fix various integration issues is clearly worse > than just having it behave like everything else.... We decided to start Scalar in contrib/, as it hasn't been proven that Scalar is in a good enough shape to deserve to be in this tree, and we are giving it a chance by adding it to contrib/ first, hoping that it may graduate to the more official status someday [*]. And 'test-extra' is a way to give test coverage to things already in contrib/ that has 'test' target in their Makefile. When js/scalar gets merged to a tree with 'test-extra' target, it may be tested in that target, too, because we want to have it behave like everything else. [Footnote] *1* You may not like the "try unproven things in contrib/ first and then we may graduate it later" approach, but that particular ship has sailed and this is not a time to complain and waste project's time.
On Thu, Dec 09 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> So, how about doing it this way? This is based on 'master' and does >>> not cover contrib/scalar, but if we want to go this route, it should >>> be trivial to do it on top of a merge of ab/ci-updates and js/scalar >>> into 'master'. Good idea? Terrible idea? Not good enough? >> >> With the caveat that I think the greater direction here makes no sense, >> i.e. scalar didn't need its own build system etc. in the first place, so >> having hack-upon-hack to fix various integration issues is clearly worse >> than just having it behave like everything else.... > > We decided to start Scalar in contrib/, as it hasn't been proven > that Scalar is in a good enough shape to deserve to be in this tree, > and we are giving it a chance by adding it to contrib/ first, hoping > that it may graduate to the more official status someday [*]. > > And 'test-extra' is a way to give test coverage to things already in > contrib/ that has 'test' target in their Makefile. When js/scalar > gets merged to a tree with 'test-extra' target, it may be tested in > that target, too, because we want to have it behave like everything > else. > > [Footnote] > I'm referring to the divide between testing things in CI v.s. "make test" making no sense. Not the path at which scalar is stored in-tree, which I don't care about, except insofar as it's used as a proxy for behavior that doesn't make sense. Scalar uses libgit.a, and is built by default in the js/scalar topic. So we don't have a choice to really ignore it. E.g. there's part of the refs.h API used only by it. If you're changing that API you need to test against scalar. Which makes sense, and I'd like to have scalar in-tree. I just don't think it makes any sense that I edit say refs.[ch], run "make test" locally, but only see that something broke in scalar's specific use of libgit.a later when I look at GitHub CI. If you're preparing a series for submission you'll need to get the CI passing. Except for portability issues etc. it should be trivial to run the same set of tests locally as we run in CI, that's the case now with any change you make to libgit and its consumers. With the scalar topic we lose that 1=1 mapping. I don't think doing that in the name of it living in contrib makes sense. If I'm preparing patches for submission I'll need to get CI passing, so I'll need to fix those tests & behavior either way as it's in-tree. Knowing about the failures later-not-sooner wastes more time, not less. > *1* You may not like the "try unproven things in contrib/ first and > then we may graduate it later" approach, but that particular > ship has sailed and this is not a time to complain and waste > project's time. We have t/t9902-completion.sh testing contrib/completion/, and we run that under "make test" and in CI alike, the same goes for t/t1021-rerere-in-workdir.sh and contrib/workdir/git-new-workdir, we've got similar (but optional) tests for contrib/credential in t/ too. The reason we do that with the completion is because some changes to e.g. tweak getopts will need to have a corresponding change to the completion. The reason we've not done that with contrib/{subtree,mw-to-git}/ is because those are thoroughly in the category of only incidentally being in-tree. I.e. we don't have any reason to think that testing those would stress core features of git itself any more than testing say out-of-tree software like git-lfs or git-annex. Testing those is still interesting, but that's for the benefit of that software itself not bitrotting, not that we're likely to introduce in-tree breakages by changing an API and missing one of its users. Scalar is thoroughly on the "completion" side of that divide, not "subtree".
On Thu, Dec 09, 2021 at 09:57:59AM -0800, Junio C Hamano wrote: > > So I think this is as likely to cause somebody a headache due to a dumb > > portability problem or random bitrot as it is to actually find a bug. I > > guess test-extra wouldn't be run by default, but only via CI, so maybe > > that limits the blast radius sufficiently. > > Yeah, that is the exact thought I had when I did it. Anybody who is > not aware of test target other than 'test' will not be hurt, and we > explicitly make the CI aware of 'test-all' to trigger it. But as > long as somebody bothered to write the tests, exercising them to > reveal bitrot-bugs either in the tested contrib stuff or the tests > themselves to be fixed or removed would be a good thing to do. I'm don't have strong feelings on it either way. But if we think those tests are worth running in CI, then... > So I am tempted to do > > test-extra: all > $(MAKE) -C contrib/credential/netrc test > $(MAKE) -C contrib/diff-highlight test > : $(MAKE) -C contrib/mw-to-git test > $(MAKE) -C contrib/subtree test ...we'd probably want to keep running mw-to-git tests, and teach one of the CI environments to install the appropriate perl modules to avoid skipping them. -Peff
On Fri, Dec 10, 2021 at 03:38:53AM +0100, Ævar Arnfjörð Bjarmason wrote: > I just don't think it makes any sense that I edit say refs.[ch], run > "make test" locally, but only see that something broke in scalar's > specific use of libgit.a later when I look at GitHub CI. I'm definitely sympathetic to this. Having been surprised by CI failure on something that worked locally is annoying at best, and downright frustrating when you can't easily reproduce the problem. But isn't that already true for most of the value that CI provides? While part of its purpose may be a back-stop for folks who don't run "make test" locally, I think the biggest value is that it covers a much wider variety of platforms and scenarios that you don't get out of "make test" already. In some of those cases you can reproduce the problem locally by tweaking build or test knobs. But in others it can be quite a bit more challenging (e.g., something that segfaults only on Windows). At least in the proposed change here you'd only be a "make test-all" away from reproducing the problem locally. I dunno. I don't feel that strongly either way about whether scalar tests should be part of "make test". Mostly just observing that this is not exactly a new case. > If I'm preparing patches for submission I'll need to get CI passing, so > I'll need to fix those tests & behavior either way as it's > in-tree. Knowing about the failures later-not-sooner wastes more time, > not less. I think there's probably a tradeoff here. How often you get a "late" notification of a bug (and how much of your time that wastes) versus how much time you spend locally running tests that you don't care about. I do agree that CI presents a bit of a conundrum for stuff at the edge of the project. It's become a de facto requirement for it to pass. In general that's good. But it means that features which were introduced under the notion of "the people who care about this area will tend to its maintenance" slowly become _everybody's_ problem as soon as they have any CI coverage. Another example here is the cmake stuff. Or the recent discussion about "-x" and bash. I wonder if there's a good way to make some CI results informational, rather than "failing". I.e., run scalar tests via CI, but if you're not working on scalar, you don't have to care. Folks who are interested in the area would keep tabs on those results and make sure that Junio's tree stays passing. That view disagrees with the final paragraph here, though: > The reason we do that with the completion is because some changes to > e.g. tweak getopts will need to have a corresponding change to the > completion. > > The reason we've not done that with contrib/{subtree,mw-to-git}/ is > because those are thoroughly in the category of only incidentally being > in-tree. > [...] > Scalar is thoroughly on the "completion" side of that divide, not > "subtree". I haven't followed the discussion closely, but in my mind "scalar" was still in the "it may live in-tree for convenience, but people who aren't working on it don't necessarily need to care about it" camp. Maybe that's not the plan, though. -Peff
On Fri, Dec 10 2021, Jeff King wrote: > On Fri, Dec 10, 2021 at 03:38:53AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> I just don't think it makes any sense that I edit say refs.[ch], run >> "make test" locally, but only see that something broke in scalar's >> specific use of libgit.a later when I look at GitHub CI. > > I'm definitely sympathetic to this. Having been surprised by CI failure > on something that worked locally is annoying at best, and downright > frustrating when you can't easily reproduce the problem. > > But isn't that already true for most of the value that CI provides? > While part of its purpose may be a back-stop for folks who don't run > "make test" locally, I think the biggest value is that it covers a much > wider variety of platforms and scenarios that you don't get out of "make > test" already. > > In some of those cases you can reproduce the problem locally by tweaking > build or test knobs. But in others it can be quite a bit more > challenging (e.g., something that segfaults only on Windows). At least > in the proposed change here you'd only be a "make test-all" away from > reproducing the problem locally. > > I dunno. I don't feel that strongly either way about whether scalar > tests should be part of "make test". Mostly just observing that this is > not exactly a new case. Yes. I'm not saying that "make test" should always run what a full CI run covers. Just that a proposed change that's really only adding one-more-test-file testing a thing in contrib in the sense that we test t/t9902-completion.sh should similarly be part of "make test". >> If I'm preparing patches for submission I'll need to get CI passing, so >> I'll need to fix those tests & behavior either way as it's >> in-tree. Knowing about the failures later-not-sooner wastes more time, >> not less. > > I think there's probably a tradeoff here. How often you get a "late" > notification of a bug (and how much of your time that wastes) versus how > much time you spend locally running tests that you don't care about. > > I do agree that CI presents a bit of a conundrum for stuff at the edge > of the project. It's become a de facto requirement for it to pass. In > general that's good. But it means that features which were introduced > under the notion of "the people who care about this area will tend to > its maintenance" slowly become _everybody's_ problem as soon as they > have any CI coverage. Another example here is the cmake stuff. Or the > recent discussion about "-x" and bash. > > I wonder if there's a good way to make some CI results informational, > rather than "failing". I.e., run scalar tests via CI, but if you're not > working on scalar, you don't have to care. Folks who are interested in > the area would keep tabs on those results and make sure that Junio's > tree stays passing. I think if we're not caring about its failures in combination with git.git changes there wouldn't be much point in having it in-tree at all. That would just be like what we've got with git-cinnabar.git. I would like it in tree. I just don' think the test/CI setup needs to be a special snowflake. > That view disagrees with the final paragraph here, though: > >> The reason we do that with the completion is because some changes to >> e.g. tweak getopts will need to have a corresponding change to the >> completion. >> >> The reason we've not done that with contrib/{subtree,mw-to-git}/ is >> because those are thoroughly in the category of only incidentally being >> in-tree. >> [...] >> Scalar is thoroughly on the "completion" side of that divide, not >> "subtree". > > I haven't followed the discussion closely, but in my mind "scalar" was > still in the "it may live in-tree for convenience, but people who aren't > working on it don't necessarily need to care about it" camp. Maybe > that's not the plan, though. Since v1 of the series[1] it's been compiled unconditionally, and there have been tests. We just didn't run the tests. In v6 the tests started being run as part of CI, which was ejected in v10 due to "[an] unrelated patch series does not interact well with them", which as I noted upthread in [2] isn't accurate, so I think the stated reason for ejecting the CI from the proposed topic doesn't reflect reality. Since then 1d855a6b335 (Merge branch 'ab/ci-updates' into next, 2021-12-07) landed, so I'd think that any narrow tweaks to get the CI working could be based on top of that topic. 1. https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/ 2. https://lore.kernel.org/git/211207.86ilw0matb.gmgdl@evledraar.gmail.com/
Hi Junio, On Wed, 8 Dec 2021, Junio C Hamano wrote: > We ship contrib/ stuff within our primary source tree but except for > the completion scripts that are tested from our primary test suite, > their test suites are not run in the CI. > > Teach the main Makefile a "test-extra" target, which goes into each > package in contrib/ whose Makefile has its own "test" target and > runs "make test" there. Add a "test-all" target to make it easy to > drive both the primary tests and these contrib tests from CI and use > it. That sends a strong message that the stuff in contrib/ is now fully under your maintenance, i.e. first-class supported. If I were you, I wouldn't. > Junio C Hamano <gitster@pobox.com> writes: > > > That is an interesting way to demonstrate how orthogonal the issues > > are, which in turn means that it is not such a big deal to add back > > the coverage to the part that goes to contrib/scalar/. I'd rather focus, _some_ focus, on the actual Scalar idea and code. > > As the actual implementation, it is a bit too icky, though. > > So, how about doing it this way? This is based on 'master' and does > not cover contrib/scalar, but if we want to go this route, it should > be trivial to do it on top of a merge of ab/ci-updates and js/scalar > into 'master'. Good idea? Terrible idea? Not good enough? Peff mentioned a couple of times how tedious it is to address CI failures e.g. in the Windows part of Git's CI runs. So it makes only sense to avoid the same problem with contrib/scalar/ altogether, especially as long as you keep saying that you are still uncertain whether it will make it into Git as a top-level command. Which is a strong argument in favor of just leaving the CI part of contrib/scalar/ out for now, and let it remain _my_ responsibility to react to any build/test problems arising from unrelated patch series entering `seen`. Doing it that way would also have the benefit of allowing more focus on the actual code in contrib/scalar/scalar.c. Not that it needs more review, I don't think, as both Stolee and Elijah gave their thumbs-up already, and I've not received any feedback that would require further changes to `scalar.c`, at least as of _this_ patch series. Ciao, Dscho
On Thu, Dec 9, 2021 at 10:12 AM Junio C Hamano <gitster@pobox.com> wrote: > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > >> So, how about doing it this way? This is based on 'master' and does > >> not cover contrib/scalar, but if we want to go this route, it should > >> be trivial to do it on top of a merge of ab/ci-updates and js/scalar > >> into 'master'. Good idea? Terrible idea? Not good enough? > > > > With the caveat that I think the greater direction here makes no sense, > > i.e. scalar didn't need its own build system etc. in the first place, so > > having hack-upon-hack to fix various integration issues is clearly worse > > than just having it behave like everything else.... > > We decided to start Scalar in contrib/, as it hasn't been proven > that Scalar is in a good enough shape to deserve to be in this tree, > and we are giving it a chance by adding it to contrib/ first, hoping > that it may graduate to the more official status someday [*]. Is that the hope? I thought the wish was for it to eventually "disappear" rather than "graduate", as per the following bits of Dscho's cover letter: """ The Scalar project was designed to be a self-destructing vehicle...For example, partial clone, sparse-checkout, and scheduled background maintenance have already been upstreamed and removed from Scalar proper...[Adding Scalar to contrib will] make it substantially easier to experiment with moving functionality from Scalar into core Git. """
Hi Peff, On Fri, 10 Dec 2021, Jeff King wrote: > On Fri, Dec 10, 2021 at 03:38:53AM +0100, Ævar Arnfjörð Bjarmason wrote: > > > I just don't think it makes any sense that I edit say refs.[ch], run > > "make test" locally, but only see that something broke in scalar's > > specific use of libgit.a later when I look at GitHub CI. > > I'm definitely sympathetic to this. Having been surprised by CI failure > on something that worked locally is annoying at best, and downright > frustrating when you can't easily reproduce the problem. I feel your frustration. Same here. > But isn't that already true for most of the value that CI provides? > While part of its purpose may be a back-stop for folks who don't run > "make test" locally, I think the biggest value is that it covers a much > wider variety of platforms and scenarios that you don't get out of "make > test" already. > > In some of those cases you can reproduce the problem locally by tweaking > build or test knobs. But in others it can be quite a bit more > challenging (e.g., something that segfaults only on Windows). At least > in the proposed change here you'd only be a "make test-all" away from > reproducing the problem locally. > > I dunno. I don't feel that strongly either way about whether scalar > tests should be part of "make test". Mostly just observing that this is > not exactly a new case. It isn't a new case. What is new is that we are talking about CI for patches targeting contrib/ specifically to introduce something cautiously that still has a chance of not ending up in Git proper (for whatever reasons), as Junio seems to be anxious to not give any premature "go" to integrate Scalar fully. In that light, I am somewhat surprised that we are still discussing putting a burden on contributors having to adapt contrib/scalar/ to their changes, when Junio still endeavors the option of not accepting that to-be-adapted code into core Git, after all. I fully expected everybody to be on board with leaving the responsibility to keep contrib/scalar/ building and passing the tests to _me_, until the day Scalar is accepted as a full Git command (which might not happen). > > If I'm preparing patches for submission I'll need to get CI passing, so > > I'll need to fix those tests & behavior either way as it's > > in-tree. Knowing about the failures later-not-sooner wastes more time, > > not less. > > I think there's probably a tradeoff here. How often you get a "late" > notification of a bug (and how much of your time that wastes) versus how > much time you spend locally running tests that you don't care about. > > I do agree that CI presents a bit of a conundrum for stuff at the edge > of the project. It's become a de facto requirement for it to pass. In > general that's good. But it means that features which were introduced > under the notion of "the people who care about this area will tend to > its maintenance" slowly become _everybody's_ problem as soon as they > have any CI coverage. Another example here is the cmake stuff. Or the > recent discussion about "-x" and bash. > > I wonder if there's a good way to make some CI results informational, > rather than "failing". I.e., run scalar tests via CI, but if you're not > working on scalar, you don't have to care. Folks who are interested in > the area would keep tabs on those results and make sure that Junio's > tree stays passing. > > That view disagrees with the final paragraph here, though: > > > The reason we do that with the completion is because some changes to > > e.g. tweak getopts will need to have a corresponding change to the > > completion. > > > > The reason we've not done that with contrib/{subtree,mw-to-git}/ is > > because those are thoroughly in the category of only incidentally being > > in-tree. > > [...] > > Scalar is thoroughly on the "completion" side of that divide, not > > "subtree". > > I haven't followed the discussion closely, but in my mind "scalar" was > still in the "it may live in-tree for convenience, but people who aren't > working on it don't necessarily need to care about it" camp. Maybe > that's not the plan, though. I had hoped for a clearer answer from Junio where he sees Scalar in the long term, for now he seems to be undecided. As a consequence, I kept targeting contrib/scalar/ with this first patch series, to leave the door open for keeping it in contrib/ as a "not maintained by Junio!" part of the tree. That is independent, of course, of my intention to keep maintaining Scalar's code (once we get a few steps further, that is, because we're still quite stuck here, the Scalar patch series has not seen any concerns in the last half dozen iterations about its design nor about its actual code). I intend to keep maintainig the Scalar code no matter whether it lives in contrib/ or whether it will be turned into a first-class command whose source code lives in the top-level directory. So yes, from my side I do not understand at all where this notion comes from that contrib/scalar/ should be treated any different than contrib/subtree/ for now. At least until contrib/scalar/ is feature-complete, that won't change. But of course, we can keep discussing back and forth the build process of Scalar, whether it should be tested in CI or not, whether it should be in contrib/ or in the top-level directory or not in Git at all, without getting the Scalar patches anywhere, for the next few years, in which case the outcome of that discussion will be completely moot because the Scalar patches would still be as stuck as they are right now. In which case it would be super annoying for any contributor who had to adapt the code in contrib/scalar/ to code changes in libgit.a, for no value in return whatsoever. So far, that contributor has been me. I sincerely hope that it won't come to that, and that we can move forward with this here patch series, with the next ones I have lined up to make Scalar feature-complete, and _then_ discuss the merits of making Scalar a first-class Git command or not. At that point we will automatically have the answer whether to build Scalar and run its tests as part of Git's CI. Ciao, Dscho
On 09/12/21 03.04, Junio C Hamano wrote: > We ship contrib/ stuff within our primary source tree but except for > the completion scripts that are tested from our primary test suite, > their test suites are not run in the CI. > > Teach the main Makefile a "test-extra" target, which goes into each > package in contrib/ whose Makefile has its own "test" target and > runs "make test" there. Add a "test-all" target to make it easy to > drive both the primary tests and these contrib tests from CI and use > it. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> No test failures found with test-all on my system. Tested-by: Bagas Sanjaya <bagasdotme@gmail.com>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Teach the main Makefile a "test-extra" target, which goes into each >> package in contrib/ whose Makefile has its own "test" target and >> runs "make test" there. Add a "test-all" target to make it easy to >> drive both the primary tests and these contrib tests from CI and use >> it. > > That sends a strong message that the stuff in contrib/ is now fully under > your maintenance, i.e. first-class supported. I do not think running tests on stuff in contrib/ sends any such message. It primarily helps _us_ to catch more regressions than we may otherwise miss. By the way, this is not limited to contrib/; if we had tests for gitk, we would have caught the recent regression in "diff -m" before it got inflicted on the general public, but that would not have been just to help "gitk", but to help keep "diff -m" sane and stable [*]. By running tests on in-tree contrib/ like scalar, at least we would notice when we are making breaking changes. At least, the need for scalar (either for the API broken by such a change to be kept unchanged or done in a different way, or the code that uses the API on the scalar side to be updated) would be noticed earlier than stuff totally outside and not even in contrib/. Of course, you have to bear the burden of (A) changing the way scalar uses the API, or (B) participating in the design of the change to the API that may break scalar's use so that everybody including scalar would be happy, or both. It's not like I am responsible for everything that happens in the tree, and it is our shared responsibility to maintain the health of the codebase. It is not limited to stuff inside or outside contrib/. There are projects that want to use libgit.a by binding us as a submodule and without interacting with us very much. And they are on their own when we change the internals. Do you mean that you want to make scalar into the same status as they are? > Not that it needs more review, I don't think, as both Stolee and Elijah > gave their thumbs-up already, and I've not received any feedback that > would require further changes to `scalar.c`, at least as of _this_ patch > series. So that argues even more to have a way to make sure we catch unintended breakages by any future mindless tree-wide "clean-ups" and interface changes, no? [Footnote] * I just double checked the candidates for "test-extra" to see if they are meant to run with a random Git they happen to see on the $PATH, or they are designed to test with the version of Git we just built, and it seems it is the latter for the ones nominated in the test-extra patch. Otherwise it would indeed reduce the benefit in half---we are not helping to catch regressions in the core stuff in such a case.
Elijah Newren <newren@gmail.com> writes: > On Thu, Dec 9, 2021 at 10:12 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> >> So, how about doing it this way? This is based on 'master' and does >> >> not cover contrib/scalar, but if we want to go this route, it should >> >> be trivial to do it on top of a merge of ab/ci-updates and js/scalar >> >> into 'master'. Good idea? Terrible idea? Not good enough? >> > >> > With the caveat that I think the greater direction here makes no sense, >> > i.e. scalar didn't need its own build system etc. in the first place, so >> > having hack-upon-hack to fix various integration issues is clearly worse >> > than just having it behave like everything else.... >> >> We decided to start Scalar in contrib/, as it hasn't been proven >> that Scalar is in a good enough shape to deserve to be in this tree, >> and we are giving it a chance by adding it to contrib/ first, hoping >> that it may graduate to the more official status someday [*]. > > Is that the hope? I thought the wish was for it to eventually > "disappear" rather than "graduate", as per the following bits of > Dscho's cover letter: > > """ > The Scalar project was designed to be a self-destructing vehicle...For > example, partial clone, sparse-checkout, and scheduled background > maintenance have already been upstreamed and removed from Scalar > proper...[Adding Scalar to contrib will] make it substantially easier > to experiment with moving functionality from Scalar into core Git. > """ I can go either way, but my impression from Dscho's messages has always been that there is no strong reason to switch existing scalar users to say "git clone <options that give behaviour like scalar>" when their fingers and scripts are used to say "scalar <this>", and a very thin shell may remain in some form in the ideal world.
Jeff King <peff@peff.net> writes: > I'm don't have strong feelings on it either way. But if we think those > tests are worth running in CI, then... > >> So I am tempted to do >> >> test-extra: all >> $(MAKE) -C contrib/credential/netrc test >> $(MAKE) -C contrib/diff-highlight test >> : $(MAKE) -C contrib/mw-to-git test >> $(MAKE) -C contrib/subtree test > > ...we'd probably want to keep running mw-to-git tests, and teach one of > the CI environments to install the appropriate perl modules to avoid > skipping them. I saw netrc credential helper break on one of the jobs that lack Perl, so the test there needs to be fixed before we can include it in test-extra.
On Mon, Dec 13, 2021 at 12:42:37AM -0800, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > >> Teach the main Makefile a "test-extra" target, which goes into each > >> package in contrib/ whose Makefile has its own "test" target and > >> runs "make test" there. Add a "test-all" target to make it easy to > >> drive both the primary tests and these contrib tests from CI and use > >> it. > > > > That sends a strong message that the stuff in contrib/ is now fully under > > your maintenance, i.e. first-class supported. > > I do not think running tests on stuff in contrib/ sends any such > message. It primarily helps _us_ to catch more regressions than we > may otherwise miss. By the way, this is not limited to contrib/; if > we had tests for gitk, we would have caught the recent regression in > "diff -m" before it got inflicted on the general public, but that > would not have been just to help "gitk", but to help keep "diff -m" > sane and stable [*]. I'd actually be a lot more sympathetic to automatically running gitk tests, because it's just consuming the public API of git (i.e., the scriptable plumbing interface). If we accidentally break that, it is the problem of the person who made the breaking change, and we would want them to know it as soon as possible. With something like scalar, though, it is adding new callers of the private API. It might be useful for somebody doing tree-wide refactoring to know they've broken something there. But it might also be a hassle, because now they have to care about fixing it, if they are interested in un-breaking their build (or un-breaking CI). The scalar code is now their problem, even though it's "just" in contrib/. In other words, it comes down to a question of where the burden for fixing things lies. Of course it is nice if somebody doing tree-wide refactoring fixes up scalar, too. But by making it optional to build and/or test stuff in contrib/ (rather than tying it to "make all" or to CI), it lets people decide how nice they want to be. For other stuff in contrib/, I'm not sure to what degree it applies. diff-highlight is pretty standalone for instance. I guess it _could_ be broken by a public-API change in Git, but I find it pretty unlikely. > Of course, you have to bear the burden of (A) changing the way > scalar uses the API, or (B) participating in the design of the > change to the API that may break scalar's use so that everybody > including scalar would be happy, or both. It's not like I am > responsible for everything that happens in the tree, and it is our > shared responsibility to maintain the health of the codebase. It is > not limited to stuff inside or outside contrib/. > > There are projects that want to use libgit.a by binding us as a > submodule and without interacting with us very much. And they are > on their own when we change the internals. Do you mean that you > want to make scalar into the same status as they are? I kind of thought that final paragraph was the plan, at least to start with. -Peff
On Tue, Dec 14, 2021 at 08:16:26AM -0500, Jeff King wrote: > > There are projects that want to use libgit.a by binding us as a > > submodule and without interacting with us very much. And they are > > on their own when we change the internals. Do you mean that you > > want to make scalar into the same status as they are? > > I kind of thought that final paragraph was the plan, at least to start > with. Oh, and just to be clear: I am really OK with either direction. I'm only claiming that I think both approaches are self-consistent and are making a tradeoff (finding bugs earlier, versus shifting burden of bug-fixing around). -Peff
diff --git i/Makefile w/Makefile index d56c0e4aad..ca14558e3c 100644 --- i/Makefile +++ w/Makefile @@ -2878,10 +2878,20 @@ export TEST_NO_MALLOC_CHECK test: all $(MAKE) -C t/ all +# Additional tests from places in contrib/ that are prepared to take +# "make -C $there test", but expects that the primary build is done +# already. +test-extra: all + $(MAKE) -C contrib/diff-highlight test + $(MAKE) -C contrib/mw-to-git test + $(MAKE) -C contrib/subtree test + +test-all:: test test-extra + perf: all $(MAKE) -C t/perf/ all -.PHONY: test perf +.PHONY: test test-extra test-all perf .PRECIOUS: $(TEST_OBJS) diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh index cc62616d80..9da0f26665 100755 --- i/ci/run-build-and-tests.sh +++ w/ci/run-build-and-tests.sh @@ -19,7 +19,7 @@ make case "$jobname" in linux-gcc) export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main - make test + make test-all export GIT_TEST_SPLIT_INDEX=yes export GIT_TEST_MERGE_ALGORITHM=recursive export GIT_TEST_FULL_IN_PACK_ARRAY=true @@ -33,20 +33,20 @@ linux-gcc) export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master export GIT_TEST_WRITE_REV_INDEX=1 export GIT_TEST_CHECKOUT_WORKERS=2 - make test + make test-all ;; linux-clang) export GIT_TEST_DEFAULT_HASH=sha1 - make test + make test-all export GIT_TEST_DEFAULT_HASH=sha256 - make test + make test-all ;; linux-gcc-4.8|pedantic) # Don't run the tests; we only care about whether Git can be # built with GCC 4.8 or with pedantic ;; *) - make test + make test-all ;; esac