| Message ID | cover-v2-00.25-00000000000-20220325T182534Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | CI: run "make [test]" directly, use $GITHUB_ENV | expand |
Ævar Arnfjörð Bjarmason wrote: > A re-roll of my improvements my series to simplify the CI setup a lot > (see diffstat), much of it was dealing with constraints that went away > with Travis et al. CI for this series (OSX runners failing for > unrelated reasons): > > https://github.com/avar/git/actions/runs/2040223909 > > For a much more detailed summary of how the output looks before/after > see v1[]. > > This series heavily conflicts with Johannes's > js/ci-github-workflow-markup in "seen", but in the v1 I suggested > basing that series on top of this one, because it can benefit a lot > from these simplifications. > > I'll reply to this series with a proposed rebasing of that series on > top of this one, which allows for removing almost all of its changes > to "ci/" with no harm to its end-goals, i.e. the splitting up of > "make" and "make test" output is something it'll get for free from > this series. > > Junio: Since that series has been stalled on still-outstanding > performance issues for a couple of months I was hoping we could queue > this instead, and perhaps in addition if Johannes approves of the > proposed re-roll on top of his. > > There's some forward progress on the performance issues (this[2] reply > of Victoria Dye's from yesterday), but fully resolving those will > probably take a bit... > > Whereas even though this one is relatively large I don't think there's > anything controversial here. The one concern that's been raised has > been Johannes's objection to removing some of the dead Azure code > (which was needed to move forward here). I asked how he'd prefer to > move forward with that in [3], but there hasn't been a reply to that > in >1 month. > While the largeness of a series shouldn't necessarily block it, the lack of overarching structure or purpose in this one makes it really difficult for me to review with much confidence (I can't speak for everyone, but it may be one of the reasons for the general lack of feedback). If you believe all of these patches as thematically-related enough to warrant being in a single series, then it would help a lot if you could: 1. Clearly describe the purpose of the series (yes they're all CI improvements, but *why* these particular improvements, and why do they all need to go together?) 2. Outline the "path" these commits take to accomplishing that purpose ("The first 3 commits do X because Y. Then, the next 4 commits do A because B." etc. or whatever format fits your writing style, as long as the information is there). 3. Reorganize commits as necessary to keep the above outline from jumping back and forth between topics. Personally, I think this could (should?) be split into at least two series: one that breaks up 'run-build-and-tests.sh' (and is more directly relevant to dscho's series), and one that does the cleanup/flag change/other work. The two appear to be independent, and the resulting two series would be a much more manageable 10-15 commits each. > I think just removing it is OK, we can always bring it back if needed, > and doing so is actually going to be simpler on top of this since the > CI is now less special, and leans more heavily on the logic of our > normal build process. > > 1. https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com > 2. https://lore.kernel.org/git/6b83bb83-32b9-20c9-fa02-c1c3170351c3@github.com/ > 3. https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/ > > Ævar Arnfjörð Bjarmason (25): > CI: run "set -ex" early in ci/lib.sh > CI: make "$jobname" explicit, remove fallback > CI: remove more dead Travis CI support > CI: remove dead "tree skipping" code > CI: remove unused Azure ci/* code > CI: don't have "git grep" invoke a pager in tree content check > CI: have "static-analysis" run a "make ci-static-analysis" target > CI: have "static-analysis" run "check-builtins", not "documentation" > CI: move p4 and git-lfs variables to ci/install-dependencies.sh > CI: consistently use "export" in ci/lib.sh > CI: export variables via a wrapper > CI: remove "run-build-and-tests.sh", run "make [test]" directly > CI: check ignored unignored build artifacts in "win[+VS] build" too > CI: invoke "make artifacts-tar" directly in windows-build > CI: split up and reduce "ci/test-documentation.sh" > CI: combine ci/install{,-docker}-dependencies.sh > CI: move "env" definitions into ci/lib.sh > ci/run-test-slice.sh: replace shelling out with "echo" > CI: pre-select test slice in Windows & VS tests > CI: only invoke ci/lib.sh as "steps" in main.yml > CI: narrow down variable definitions in --build and --test > CI: add more variables to MAKEFLAGS, except under vs-build > CI: set CC in MAKEFLAGS directly, don't add it to the environment > CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case > CI: don't use "set -x" in "ci/lib.sh" output > > .github/workflows/main.yml | 100 +++++--- > Makefile | 31 ++- > ci/check-directional-formatting.bash | 2 +- > ci/check-unignored-build-artifacts.sh | 20 ++ > ci/install-dependencies.sh | 53 ++++- > ci/install-docker-dependencies.sh | 22 -- > ci/lib-ci-type.sh | 10 + > ci/lib-tput.sh | 2 + > ci/lib.sh | 316 +++++++++++++------------- > ci/make-test-artifacts.sh | 12 - > ci/mount-fileshare.sh | 25 -- > ci/print-test-failures.sh | 16 +- > ci/run-build-and-tests.sh | 54 ----- > ci/run-docker-build.sh | 66 ------ > ci/run-docker.sh | 47 ---- > ci/run-static-analysis.sh | 32 --- > ci/run-test-slice.sh | 17 -- > ci/select-test-slice.sh | 10 + > ci/test-documentation.sh | 37 +-- > ci/util/extract-trash-dirs.sh | 50 ---- > shared.mak | 1 + > 21 files changed, 346 insertions(+), 577 deletions(-) > create mode 100755 ci/check-unignored-build-artifacts.sh > delete mode 100755 ci/install-docker-dependencies.sh > create mode 100644 ci/lib-ci-type.sh > create mode 100644 ci/lib-tput.sh > delete mode 100755 ci/make-test-artifacts.sh > delete mode 100755 ci/mount-fileshare.sh > delete mode 100755 ci/run-build-and-tests.sh > delete mode 100755 ci/run-docker-build.sh > delete mode 100755 ci/run-docker.sh > delete mode 100755 ci/run-static-analysis.sh > delete mode 100755 ci/run-test-slice.sh > create mode 100755 ci/select-test-slice.sh > delete mode 100755 ci/util/extract-trash-dirs.sh > > Range-diff against v1: > 1: 970849a227f = 1: 4addbd70213 CI: run "set -ex" early in ci/lib.sh > 2: eda7fb18064 = 2: b23aa99fd37 CI: make "$jobname" explicit, remove fallback > 3: 3ee32815cf3 = 3: eec15a95879 CI: remove more dead Travis CI support > 4: c81c1b3f504 = 4: 73c894f1665 CI: remove dead "tree skipping" code > 5: 4738a22a36d = 5: b5e6d538554 CI: remove unused Azure ci/* code > 6: 59e4f41e86c = 6: a4b9febbdca CI: don't have "git grep" invoke a pager in tree content check > 7: 836ef20fdcc ! 7: 5d46d5b34c9 CI: have "static-analysis" run a "make ci-static-analysis" target > @@ .github/workflows/main.yml: jobs: > if: needs.ci-config.outputs.enabled == 'yes' > > ## Makefile ## > -@@ Makefile: ifndef V > - QUIET_HDR = @echo ' ' HDR $(<:hcc=h); > - QUIET_RC = @echo ' ' RC $@; > - QUIET_SPATCH = @echo ' ' SPATCH $<; > -+ QUIET_CHECK = @echo ' ' CHECK $@; > - QUIET_SUBDIR0 = +@subdir= > - QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ > - $(MAKE) $(PRINT_DIR) -C $$subdir > @@ Makefile: coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/c > # See contrib/coccinelle/README > coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) > @@ ci/run-static-analysis.sh (deleted) > - > -make hdr-check || > -exit 1 > + > + ## shared.mak ## > +@@ shared.mak: ifndef V > + QUIET_HDR = @echo ' ' HDR $(<:hcc=h); > + QUIET_RC = @echo ' ' RC $@; > + QUIET_SPATCH = @echo ' ' SPATCH $<; > ++ QUIET_CHECK = @echo ' ' CHECK $@; > + > + ## Used in "Documentation/Makefile" > + QUIET_ASCIIDOC = @echo ' ' ASCIIDOC $@; > 8: 95cd496868e = 8: 81e06f13d84 CI: have "static-analysis" run "check-builtins", not "documentation" > 9: a1d0796259e = 9: 3be91c26d44 CI: move p4 and git-lfs variables to ci/install-dependencies.sh > 10: b6a61a786c5 = 10: 9dc148341ba CI: consistently use "export" in ci/lib.sh > 11: f2fcee5d6e4 = 11: e9c7ba492e8 CI: export variables via a wrapper > 12: dfd823f2e7d = 12: 86d5a48d59a CI: remove "run-build-and-tests.sh", run "make [test]" directly > 13: 46459fff296 = 13: 649ad1ae249 CI: check ignored unignored build artifacts in "win[+VS] build" too > 14: aecd3ebaafe = 14: b1b5b083389 CI: invoke "make artifacts-tar" directly in windows-build > 15: 4f1564af70f = 15: dfa91ac8938 CI: split up and reduce "ci/test-documentation.sh" > 16: 868613a5b06 = 16: ba4ed216769 CI: combine ci/install{,-docker}-dependencies.sh > 17: 5d854e8ff36 = 17: b5e89a33340 CI: move "env" definitions into ci/lib.sh > 18: a6106525b7f = 18: 571f4d0f441 ci/run-test-slice.sh: replace shelling out with "echo" > 19: e9c6c4dd293 = 19: 2edea06ee4d CI: pre-select test slice in Windows & VS tests > 20: 40d86e8c1dc = 20: ef9daa6882f CI: only invoke ci/lib.sh as "steps" in main.yml > 21: abf9c504740 = 21: 44e3ace5fbe CI: narrow down variable definitions in --build and --test > 22: 9f4c2798a82 = 22: 5f56b922e08 CI: add more variables to MAKEFLAGS, except under vs-build > 23: 8a8b7ecf16b ! 23: b45b7cec94e CI: stop over-setting the $CC variable > @@ Metadata > Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > ## Commit message ## > - CI: stop over-setting the $CC variable > + CI: set CC in MAKEFLAGS directly, don't add it to the environment > > - As detailed in 2c8921db2b8 (travis-ci: build with the right compiler, > - 2019-01-17) the reason we started using $CC in $MAKEFLAGS as opposed > - to setting it in the environment was due to Travis CI clobbering $CC > - in the environment. > + Rather than pass a "$CC" and "$CC_PACKAGE" in the environment to be > + picked up in ci/lib.sh let's instead have ci/lib.sh itself add it > + directly to MAKEFLAGS. > > - We don't need to set it unconditionally to accomplish that, but rather > - just have it set for those jobs that need them. E.g. the "win+VS > - build" job confusingly has CC=gcc set, even though it builds with > - MSVC. > + Setting CC=gcc by default made for confusing trace output, and since a > + preceding change to carry it and others over across "steps" in the > + GitHub CI it's been even more misleading. E.g. the "win+VS build" job > + confusingly has CC=gcc set, even though it builds with MSVC. > + > + Let's instead reply on the Makefile default of CC=cc, and only > + override it for those jobs where it's needed. This does mean that > + we'll need to set it for the "pedantic" job, which previously relied > + on the default CC=gcc in case "clang" become the default on that > + platform. > > This partially reverts my 707d2f2fe86 (CI: use "$runs_on_pool", not > "$jobname" to select packages & config, 2021-11-23), i.e. we're now > @@ ci/lib.sh: linux-TEST-vars) > setenv --test GIT_TEST_DEFAULT_HASH sha256 > ;; > pedantic) > ++ CC=gcc > + # Don't run the tests; we only care about whether Git can be > + # built. > + setenv --build DEVOPTS pedantic > @@ ci/lib.sh: linux-musl) > MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8" > ;; > 24: d7b472b4a52 = 24: 06bf8d9f61b CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case > 25: 08a9776c259 = 25: 6dc96ba8b82 CI: don't use "set -x" in "ci/lib.sh" output
On Fri, Mar 25 2022, Victoria Dye wrote: > Ævar Arnfjörð Bjarmason wrote: >> A re-roll of my improvements my series to simplify the CI setup a lot >> (see diffstat), much of it was dealing with constraints that went away >> with Travis et al. CI for this series (OSX runners failing for >> unrelated reasons): >> >> https://github.com/avar/git/actions/runs/2040223909 >> >> For a much more detailed summary of how the output looks before/after >> see v1[]. >> >> This series heavily conflicts with Johannes's >> js/ci-github-workflow-markup in "seen", but in the v1 I suggested >> basing that series on top of this one, because it can benefit a lot >> from these simplifications. >> >> I'll reply to this series with a proposed rebasing of that series on >> top of this one, which allows for removing almost all of its changes >> to "ci/" with no harm to its end-goals, i.e. the splitting up of >> "make" and "make test" output is something it'll get for free from >> this series. >> >> Junio: Since that series has been stalled on still-outstanding >> performance issues for a couple of months I was hoping we could queue >> this instead, and perhaps in addition if Johannes approves of the >> proposed re-roll on top of his. >> >> There's some forward progress on the performance issues (this[2] reply >> of Victoria Dye's from yesterday), but fully resolving those will >> probably take a bit... >> >> Whereas even though this one is relatively large I don't think there's >> anything controversial here. The one concern that's been raised has >> been Johannes's objection to removing some of the dead Azure code >> (which was needed to move forward here). I asked how he'd prefer to >> move forward with that in [3], but there hasn't been a reply to that >> in >1 month. >> > > While the largeness of a series shouldn't necessarily block it, the lack of > overarching structure or purpose in this one makes it really difficult for > me to review with much confidence (I can't speak for everyone, but it may be > one of the reasons for the general lack of feedback). If you believe all of > these patches as thematically-related enough to warrant being in a single > series, then it would help a lot if you could: > > 1. Clearly describe the purpose of the series (yes they're all CI > improvements, but *why* these particular improvements, and why do they > all need to go together?) > 2. Outline the "path" these commits take to accomplishing that purpose ("The > first 3 commits do X because Y. Then, the next 4 commits do A because B." > etc. or whatever format fits your writing style, as long as the > information is there). > 3. Reorganize commits as necessary to keep the above outline from jumping > back and forth between topics. The v1 summary described it about as clearly as I was able to: https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com/ > Personally, I think this could (should?) be split into at least two series: > one that breaks up 'run-build-and-tests.sh' (and is more directly relevant > to dscho's series), and one that does the cleanup/flag change/other work. > The two appear to be independent, and the resulting two series would be a > much more manageable 10-15 commits each. To rephrase that a bit, every commit here passes CI and is atomic, so it could be split up into 25 parts (at least). But it's really not doing different things, it's a single-topic series: It's changing CI "step" targets that are shellscripts that do N things to instead be single command invocations at the "step" level, driven by the CI recipe itself. To do that we need to pass state that we previously re-setup for every "step" via $GITHUB_ENV, whose state we then helpfully show (this is just a standard GitHub CI feature) in a drop-down at the start of every "step". So yes, it could be split up in the sense that we could get partway there and continue with the rest some other time, but I really think the UX experience is *much better* if it's not a partial conversion. I.e. at the tip of this series you can reliably look at that $GITHUB_ENV view to see what the full and relevant environment was for that "make", "make test" or whatever. If we just do that partially you might get that for "make" if it failed, but if your tests failed we'd have a failure in the proverbial ci/load-lib.sh-do-setup-stuff-and-run-make-test-at-some-point.sh. Which I think makes the UX much less useful, i.e. you really want to *always* find this information in the same place.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > A re-roll of my improvements my series to simplify the CI setup a lot > (see diffstat), much of it was dealing with constraints that went away > with Travis et al. CI for this series (OSX runners failing for > unrelated reasons): > > https://github.com/avar/git/actions/runs/2040223909 > > For a much more detailed summary of how the output looks before/after > see v1[]. > > This series heavily conflicts with Johannes's > js/ci-github-workflow-markup in "seen", but in the v1 I suggested > basing that series on top of this one, because it can benefit a lot > from these simplifications. > > I'll reply to this series with a proposed rebasing of that series on > top of this one, which allows for removing almost all of its changes > to "ci/" with no harm to its end-goals, i.e. the splitting up of > "make" and "make test" output is something it'll get for free from > this series. A sample run for this can be seen at https://github.com/git/git/runs/5715128999?check_suite_focus=true With the output, I can point at a specific line, e.g. https://github.com/git/git/runs/5715128999?check_suite_focus=true#step:7:1150 The URLs with Dscho's one that correspond to the above two are https://github.com/git/git/runs/5699946885?check_suite_focus=true https://github.com/git/git/runs/5699946885?check_suite_focus=true#step:4:1826 The specific breakage (which has little to do with the comparison between CI error pages) is that diff_setup() is called and populate diff_options.parseopts member but diff_setup_done() does not seem to be called and ends up leaking it. I wonder why options->parseopts is not freed immediately after diff_opt_parse() calls parse_options(), but need to be kept until diff_setup_done().
Hi Ævar, On Fri, 25 Mar 2022, Ævar Arnfjörð Bjarmason wrote: > A re-roll of my improvements my series to simplify the CI setup a lot > (see diffstat), much of it was dealing with constraints that went away > with Travis et al. This type of work causes me a lot of follow-up work e.g. many merge conflicts in the latest Git for Windows rebases. Perhaps it is worth taking a step back and evaluating the return on this investment in the CI setup. While this can be characterized as a simplification taking the diffstat as proof, one could challenge that the diffstat does not actually measure whether the code is simple or not, it just measures whether there are less lines in the end. If the diffstat was a good measure, then the optimum would be one 0-byte `.c` file (which some C compilers compile without error). Another obvious way to optimize the diffstat would be to remove all code comments. Would I suggest to do either? Of course not. The reduction in code size of this patch series also comes at quite a steep cost: After all, the way Lars and Gábor set things up was already easy to reuse with Azure Pipeline and GitHub Actions. Removing this type of generic, easily-to-adapt code can remove a lot of lines at the expense of making the code less generic and harder to adapt, and leads us directly to CI lock-in. A better approach would be to use the already-generic code and adapt it e.g. to extend to the CirrusCI definition we have. Even if you do not care about extending our FreeBSD coverage, I would like to ask to slow down on refactoring as it is done in this patch series. As indicated in my comment above, these types of refactorings lead to a lot of complications in Git for Windows's processes, which are time-consuming to resolve. I understand your motivation, but if you wouldn't mind taking some time to weigh the criticality of these changes against the overhead incurred for maintainers, it would be appreciated. > I think just removing it is OK, we can always bring it back if needed, > and doing so is actually going to be simpler on top of this since the CI > is now less special, and leans more heavily on the logic of our normal > build process. Removing and re-adding things does take time, though. Again, I think it would be helpful to step back and try to understand the value of this removal versus the projected time it would take (from all involved) to re-add. Besides, given how much is shuffled around in this patch series (e.g. some files in ci/ are removed altogether and their equivalent code is moved into various other places), doubt must be cast on the idea that it would be simple to bring back anything here. Ciao, Johannes
On Tue, Apr 05 2022, Johannes Schindelin wrote: > Hi Ævar, > > On Fri, 25 Mar 2022, Ævar Arnfjörð Bjarmason wrote: > >> A re-roll of my improvements my series to simplify the CI setup a lot >> (see diffstat), much of it was dealing with constraints that went away >> with Travis et al. > > This type of work causes me a lot of follow-up work e.g. many merge > conflicts in the latest Git for Windows rebases. Can you elaborate a bit on why? These patches are currently in "seen" and not "next", and I thought GFW merged git.git's "master" for new releases, and e.g. in: git diff --stat -p v2.36.0-rc0..v2.36.0-rc0.windows.1 -- ci t I don't see anything related to those sorts of CI changes. I.e. I understand that you may have something out-of-git.git that may conflict with this, I'm confused about the GFW reference in particular. > Perhaps it is worth taking a step back and evaluating the return on this > investment in the CI setup. While this can be characterized as a > simplification taking the diffstat as proof, one could challenge that the > diffstat does not actually measure whether the code is simple or not, it > just measures whether there are less lines in the end. > > If the diffstat was a good measure, then the optimum would be one 0-byte > `.c` file (which some C compilers compile without error). Another obvious > way to optimize the diffstat would be to remove all code comments. Would I > suggest to do either? Of course not. This is a misunderstanding I really didn't expect, but no. I'm not suggesting that the diffstat being shorter is a value in and of itself. Clearly that's ridiculous, as we could make the code "better" by removing subsequent newlines or whatever. I thought it was clear in context that the v2 CL's reference to this is a shorthand for us being able to generally "do more with less" summarized in the v1: https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com > The reduction in code size of this patch series also comes at quite a > steep cost: After all, the way Lars and Gábor set things up was already > easy to reuse with Azure Pipeline and GitHub Actions. > > Removing this type of generic, easily-to-adapt code can remove a lot of > lines at the expense of making the code less generic and harder to adapt, > and leads us directly to CI lock-in. It's really hard to try to come to some sort of landing with changes in ci/ when it looks as though you haven't read my side of the patches or why these changes were made. So in liue of some long summary of that I'll just say briefly that the reason for why I made those changes is covered in detail in commits you haven't replied to, and which argue the opposite of what you do here. Briefly: The entire reason I changed those bits is exactly to avoid the sort of CI lock-in you're talking about, to th point where this series effectively adds another CI target: You can run everything it's doing with a normal "make" invocation. > A better approach would be to use the already-generic code and adapt it > e.g. to extend to the CirrusCI definition we have. This series doesn't change .cirrus.yml or how it functions, but it just does: build_script: - su git -c gmake test_script: - su git -c 'gmake test' Which, after this series is exactly what your "main" CI does. So we're set up to make it easier to unify the two. > Even if you do not care about extending our FreeBSD coverage, FWIW I do care, and I've sent in various portability patches for FreeBSD, but I haven't used that CI in particular. > I would like > to ask to slow down on refactoring as it is done in this patch series. As > indicated in my comment above, these types of refactorings lead to a lot > of complications in Git for Windows's processes, which are time-consuming > to resolve. I understand your motivation, but if you wouldn't mind taking > some time to weigh the criticality of these changes against the overhead > incurred for maintainers, it would be appreciated. Sure, I'm happy to accommodate that. As this v2 CL notes I'd asked you a month before submitting it in [1] what I could do to make the parts of it you seemed to find most objectionable easier or different for you. >> I think just removing it is OK, we can always bring it back if needed, >> and doing so is actually going to be simpler on top of this since the CI >> is now less special, and leans more heavily on the logic of our normal >> build process. > > Removing and re-adding things does take time, though. Again, I think it > would be helpful to step back and try to understand the value of this > removal versus the projected time it would take (from all involved) to > re-add. Having prepped this v2 + the RFC of your (then smaller) patches on top I'm pretty sure it would be trivial for either of us compared to continuing this verbal back & forth. So, to that end I re-rolled the two in combination, I think it would be much more helpful to comment on the substance of the changes here. E.g. we can now see the sum of my patches and yours in "seen", they both change the CI UI in what I think are complimentary ways (issues of e.g. added slowness in your patches aside, which has been discussed). For easy comparison I prepared this the other day, and committed the same "make CI fail" commit on top to all of them: * Your original patches, just rebased on master: https://github.com/avar/git/tree/avar-pr-1117/dscho/use-grouping-in-ci-v2-FAIL-TEST * Just my v2 series here: https://github.com/avar/git/tree/avar/ci-unroll-make-commands-to-ci-recipe-FAIL-TEST * The sum of the two, i.e. the v2 + your "RFC" (the RFC label being mine) on top: https://github.com/avar/git/tree/avar-dscho/use-grouping-in-ci-v2-FAIL-TEST If you've got some conflicts or whatever with something out of tree you'd like me to accommodate or even resolve for you I'd be happy to do so, just send me the specifics. > Besides, given how much is shuffled around in this patch series (e.g. some > files in ci/ are removed altogether and their equivalent code is moved > into various other places), doubt must be cast on the idea that it would > be simple to bring back anything here. I'm assuming you're talking about resurrecting Azure CI, i.e. (mostly) this would semantically conflict: git grep ci/ 6081d3898fe^ -- azure-pipelines.yml One thing I can see right away that would need adjusting is that I "git rm" the "ci/mount-fileshare.sh" script as unused. It's part of what I asked about in [1], but I could just leave that in place, would you like me to do that for a re-roll & would that help? For the rest "ci/install-dependencies.sh" is the same, "ci/run-build-and-tests.sh" would be an easy matter of making it run "make test" instead after invoking ci/lib.sh. We're really just talking about a few lines of boilerplate here, I really don't see what the big deal is. As my patches also note there's things that the azure-pipelines.yml assumed that are no longer true (before any changes I made), so some of this would need adjusting in any case. It also does a test run followed by ci/print-test-failures.sh, which as I understand you've been maintaining quite adamantly insisting is pretty much useless compared to the new GitHub-specific output your series adds, and which is CI-specific. Presumably most of the work of getting Azure running again would be (in your version) to adjust that to something Azure-specific. Whereas on the CI portability front I noted in [2] and [3] that we could do pretty much the same without adding CI-specific output targets. Anyway, I really hope we can find some way past what seems to be an impasse with these various CI changes. All the best. 1. https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/ 2. https://lore.kernel.org/git/220324.8635j7nyvw.gmgdl@evledraar.gmail.com/ 3. https://lore.kernel.org/git/220302.86mti87cj2.gmgdl@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Briefly: The entire reason I changed those bits is exactly to avoid the > sort of CI lock-in you're talking about, to th point where this series > effectively adds another CI target: You can run everything it's doing > with a normal "make" invocation. I'll comment only on this part. FWIW, the above matches my impression fromt he base part of the rewrite. As long as a particular CI integration runs shell scriptlets that invoke "make", moving the code that is shared common among CI platforms from the shell scriptlets to the recipe in Makefile should not make it harder to support it, and ... >> A better approach would be to use the already-generic code and adapt it >> e.g. to extend to the CirrusCI definition we have. > > This series doesn't change .cirrus.yml or how it functions, but it just > does: > > build_script: > - su git -c gmake > test_script: > - su git -c 'gmake test' > > Which, after this series is exactly what your "main" CI does. So we're > set up to make it easier to unify the two. ... BSDs or any POSIXy system I would expect to fall into the same "shell scriptlets eventually driving make" pattern. The same argument may not apply if another CI platform uses shell scriptlets but does not want to use "make" at all. The "do more in 'make'" movement could be hurting such a user. I do not know if Windows' preference to use cmake plays a role there, or if Dscho's resistance comes from there, though. > Anyway, I really hope we can find some way past what seems to be an > impasse with these various CI changes. All the best. Yup. Thanks.
Hi Ævar On 25/03/2022 20:43, Victoria Dye wrote: > Ævar Arnfjörð Bjarmason wrote: >> A re-roll of my improvements my series to simplify the CI setup a lot >> (see diffstat), much of it was dealing with constraints that went away >> with Travis et al. CI for this series (OSX runners failing for >> unrelated reasons): >> >> https://github.com/avar/git/actions/runs/2040223909 >> >> For a much more detailed summary of how the output looks before/after >> see v1[]. >> >> This series heavily conflicts with Johannes's >> js/ci-github-workflow-markup in "seen", but in the v1 I suggested >> basing that series on top of this one, because it can benefit a lot >> from these simplifications. >> >> I'll reply to this series with a proposed rebasing of that series on >> top of this one, which allows for removing almost all of its changes >> to "ci/" with no harm to its end-goals, i.e. the splitting up of >> "make" and "make test" output is something it'll get for free from >> this series. >> >> Junio: Since that series has been stalled on still-outstanding >> performance issues for a couple of months I was hoping we could queue >> this instead, and perhaps in addition if Johannes approves of the >> proposed re-roll on top of his. >> >> There's some forward progress on the performance issues (this[2] reply >> of Victoria Dye's from yesterday), but fully resolving those will >> probably take a bit... >> >> Whereas even though this one is relatively large I don't think there's >> anything controversial here. The one concern that's been raised has >> been Johannes's objection to removing some of the dead Azure code >> (which was needed to move forward here). I asked how he'd prefer to >> move forward with that in [3], but there hasn't been a reply to that >> in >1 month. >> > > While the largeness of a series shouldn't necessarily block it, the lack of > overarching structure or purpose in this one makes it really difficult for > me to review with much confidence (I can't speak for everyone, but it may be > one of the reasons for the general lack of feedback). That applies to me, I think this would be easier to follow if it was split up. As far as I can tell there are several groups of changes and I think it would be easier for reviewers if they were split into separate shorter series. 1. Split building git and running the tests into separate steps. This is I think the only user facing change. I think it is worthwhile and could be done in a single patch that just moved make test into a separate script without a lot of refactoring. In particular I do not see the need for separate steps that setup the environment for the build and tests (especially as the build environment variables are still present when the tests are run). Without the refactoring anyone resurrecting the azure pipelines for testing security fixes would easily be able to update those build definitions. 2. Moving the static analysis ci job into the Makefile. I'm not sure what the practical benefit of this is but it could be a self contained series. 3. Being able to run the ci scripts locally. Does this have a use beyond debugging them locally? Could this be implemented with a wrapper script rather than a large refactoring? 4. Cleanups. To be honest I think we could happily live without a some of these, especially patches 24-26 of v3 that rework code introduced in this series. I see v4 has been posted while I was writing this and is even longer than v3. Best Wishes Phillip > If you believe all of > these patches as thematically-related enough to warrant being in a single > series, then it would help a lot if you could: > > 1. Clearly describe the purpose of the series (yes they're all CI > improvements, but *why* these particular improvements, and why do they > all need to go together?) > 2. Outline the "path" these commits take to accomplishing that purpose ("The > first 3 commits do X because Y. Then, the next 4 commits do A because B." > etc. or whatever format fits your writing style, as long as the > information is there). > 3. Reorganize commits as necessary to keep the above outline from jumping > back and forth between topics. > > Personally, I think this could (should?) be split into at least two series: > one that breaks up 'run-build-and-tests.sh' (and is more directly relevant > to dscho's series), and one that does the cleanup/flag change/other work. > The two appear to be independent, and the resulting two series would be a > much more manageable 10-15 commits each. > >> I think just removing it is OK, we can always bring it back if needed, >> and doing so is actually going to be simpler on top of this since the >> CI is now less special, and leans more heavily on the logic of our >> normal build process. >> >> 1. https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com >> 2. https://lore.kernel.org/git/6b83bb83-32b9-20c9-fa02-c1c3170351c3@github.com/ >> 3. https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/ >> >> Ævar Arnfjörð Bjarmason (25): >> CI: run "set -ex" early in ci/lib.sh >> CI: make "$jobname" explicit, remove fallback >> CI: remove more dead Travis CI support >> CI: remove dead "tree skipping" code >> CI: remove unused Azure ci/* code >> CI: don't have "git grep" invoke a pager in tree content check >> CI: have "static-analysis" run a "make ci-static-analysis" target >> CI: have "static-analysis" run "check-builtins", not "documentation" >> CI: move p4 and git-lfs variables to ci/install-dependencies.sh >> CI: consistently use "export" in ci/lib.sh >> CI: export variables via a wrapper >> CI: remove "run-build-and-tests.sh", run "make [test]" directly >> CI: check ignored unignored build artifacts in "win[+VS] build" too >> CI: invoke "make artifacts-tar" directly in windows-build >> CI: split up and reduce "ci/test-documentation.sh" >> CI: combine ci/install{,-docker}-dependencies.sh >> CI: move "env" definitions into ci/lib.sh >> ci/run-test-slice.sh: replace shelling out with "echo" >> CI: pre-select test slice in Windows & VS tests >> CI: only invoke ci/lib.sh as "steps" in main.yml >> CI: narrow down variable definitions in --build and --test >> CI: add more variables to MAKEFLAGS, except under vs-build >> CI: set CC in MAKEFLAGS directly, don't add it to the environment >> CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case >> CI: don't use "set -x" in "ci/lib.sh" output >> >> .github/workflows/main.yml | 100 +++++--- >> Makefile | 31 ++- >> ci/check-directional-formatting.bash | 2 +- >> ci/check-unignored-build-artifacts.sh | 20 ++ >> ci/install-dependencies.sh | 53 ++++- >> ci/install-docker-dependencies.sh | 22 -- >> ci/lib-ci-type.sh | 10 + >> ci/lib-tput.sh | 2 + >> ci/lib.sh | 316 +++++++++++++------------- >> ci/make-test-artifacts.sh | 12 - >> ci/mount-fileshare.sh | 25 -- >> ci/print-test-failures.sh | 16 +- >> ci/run-build-and-tests.sh | 54 ----- >> ci/run-docker-build.sh | 66 ------ >> ci/run-docker.sh | 47 ---- >> ci/run-static-analysis.sh | 32 --- >> ci/run-test-slice.sh | 17 -- >> ci/select-test-slice.sh | 10 + >> ci/test-documentation.sh | 37 +-- >> ci/util/extract-trash-dirs.sh | 50 ---- >> shared.mak | 1 + >> 21 files changed, 346 insertions(+), 577 deletions(-) >> create mode 100755 ci/check-unignored-build-artifacts.sh >> delete mode 100755 ci/install-docker-dependencies.sh >> create mode 100644 ci/lib-ci-type.sh >> create mode 100644 ci/lib-tput.sh >> delete mode 100755 ci/make-test-artifacts.sh >> delete mode 100755 ci/mount-fileshare.sh >> delete mode 100755 ci/run-build-and-tests.sh >> delete mode 100755 ci/run-docker-build.sh >> delete mode 100755 ci/run-docker.sh >> delete mode 100755 ci/run-static-analysis.sh >> delete mode 100755 ci/run-test-slice.sh >> create mode 100755 ci/select-test-slice.sh >> delete mode 100755 ci/util/extract-trash-dirs.sh >> >> Range-diff against v1: >> 1: 970849a227f = 1: 4addbd70213 CI: run "set -ex" early in ci/lib.sh >> 2: eda7fb18064 = 2: b23aa99fd37 CI: make "$jobname" explicit, remove fallback >> 3: 3ee32815cf3 = 3: eec15a95879 CI: remove more dead Travis CI support >> 4: c81c1b3f504 = 4: 73c894f1665 CI: remove dead "tree skipping" code >> 5: 4738a22a36d = 5: b5e6d538554 CI: remove unused Azure ci/* code >> 6: 59e4f41e86c = 6: a4b9febbdca CI: don't have "git grep" invoke a pager in tree content check >> 7: 836ef20fdcc ! 7: 5d46d5b34c9 CI: have "static-analysis" run a "make ci-static-analysis" target >> @@ .github/workflows/main.yml: jobs: >> if: needs.ci-config.outputs.enabled == 'yes' >> >> ## Makefile ## >> -@@ Makefile: ifndef V >> - QUIET_HDR = @echo ' ' HDR $(<:hcc=h); >> - QUIET_RC = @echo ' ' RC $@; >> - QUIET_SPATCH = @echo ' ' SPATCH $<; >> -+ QUIET_CHECK = @echo ' ' CHECK $@; >> - QUIET_SUBDIR0 = +@subdir= >> - QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ >> - $(MAKE) $(PRINT_DIR) -C $$subdir >> @@ Makefile: coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/c >> # See contrib/coccinelle/README >> coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) >> @@ ci/run-static-analysis.sh (deleted) >> - >> -make hdr-check || >> -exit 1 >> + >> + ## shared.mak ## >> +@@ shared.mak: ifndef V >> + QUIET_HDR = @echo ' ' HDR $(<:hcc=h); >> + QUIET_RC = @echo ' ' RC $@; >> + QUIET_SPATCH = @echo ' ' SPATCH $<; >> ++ QUIET_CHECK = @echo ' ' CHECK $@; >> + >> + ## Used in "Documentation/Makefile" >> + QUIET_ASCIIDOC = @echo ' ' ASCIIDOC $@; >> 8: 95cd496868e = 8: 81e06f13d84 CI: have "static-analysis" run "check-builtins", not "documentation" >> 9: a1d0796259e = 9: 3be91c26d44 CI: move p4 and git-lfs variables to ci/install-dependencies.sh >> 10: b6a61a786c5 = 10: 9dc148341ba CI: consistently use "export" in ci/lib.sh >> 11: f2fcee5d6e4 = 11: e9c7ba492e8 CI: export variables via a wrapper >> 12: dfd823f2e7d = 12: 86d5a48d59a CI: remove "run-build-and-tests.sh", run "make [test]" directly >> 13: 46459fff296 = 13: 649ad1ae249 CI: check ignored unignored build artifacts in "win[+VS] build" too >> 14: aecd3ebaafe = 14: b1b5b083389 CI: invoke "make artifacts-tar" directly in windows-build >> 15: 4f1564af70f = 15: dfa91ac8938 CI: split up and reduce "ci/test-documentation.sh" >> 16: 868613a5b06 = 16: ba4ed216769 CI: combine ci/install{,-docker}-dependencies.sh >> 17: 5d854e8ff36 = 17: b5e89a33340 CI: move "env" definitions into ci/lib.sh >> 18: a6106525b7f = 18: 571f4d0f441 ci/run-test-slice.sh: replace shelling out with "echo" >> 19: e9c6c4dd293 = 19: 2edea06ee4d CI: pre-select test slice in Windows & VS tests >> 20: 40d86e8c1dc = 20: ef9daa6882f CI: only invoke ci/lib.sh as "steps" in main.yml >> 21: abf9c504740 = 21: 44e3ace5fbe CI: narrow down variable definitions in --build and --test >> 22: 9f4c2798a82 = 22: 5f56b922e08 CI: add more variables to MAKEFLAGS, except under vs-build >> 23: 8a8b7ecf16b ! 23: b45b7cec94e CI: stop over-setting the $CC variable >> @@ Metadata >> Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> ## Commit message ## >> - CI: stop over-setting the $CC variable >> + CI: set CC in MAKEFLAGS directly, don't add it to the environment >> >> - As detailed in 2c8921db2b8 (travis-ci: build with the right compiler, >> - 2019-01-17) the reason we started using $CC in $MAKEFLAGS as opposed >> - to setting it in the environment was due to Travis CI clobbering $CC >> - in the environment. >> + Rather than pass a "$CC" and "$CC_PACKAGE" in the environment to be >> + picked up in ci/lib.sh let's instead have ci/lib.sh itself add it >> + directly to MAKEFLAGS. >> >> - We don't need to set it unconditionally to accomplish that, but rather >> - just have it set for those jobs that need them. E.g. the "win+VS >> - build" job confusingly has CC=gcc set, even though it builds with >> - MSVC. >> + Setting CC=gcc by default made for confusing trace output, and since a >> + preceding change to carry it and others over across "steps" in the >> + GitHub CI it's been even more misleading. E.g. the "win+VS build" job >> + confusingly has CC=gcc set, even though it builds with MSVC. >> + >> + Let's instead reply on the Makefile default of CC=cc, and only >> + override it for those jobs where it's needed. This does mean that >> + we'll need to set it for the "pedantic" job, which previously relied >> + on the default CC=gcc in case "clang" become the default on that >> + platform. >> >> This partially reverts my 707d2f2fe86 (CI: use "$runs_on_pool", not >> "$jobname" to select packages & config, 2021-11-23), i.e. we're now >> @@ ci/lib.sh: linux-TEST-vars) >> setenv --test GIT_TEST_DEFAULT_HASH sha256 >> ;; >> pedantic) >> ++ CC=gcc >> + # Don't run the tests; we only care about whether Git can be >> + # built. >> + setenv --build DEVOPTS pedantic >> @@ ci/lib.sh: linux-musl) >> MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8" >> ;; >> 24: d7b472b4a52 = 24: 06bf8d9f61b CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case >> 25: 08a9776c259 = 25: 6dc96ba8b82 CI: don't use "set -x" in "ci/lib.sh" output >
A re-roll of my improvements my series to simplify the CI setup a lot (see diffstat), much of it was dealing with constraints that went away with Travis et al. CI for this series (OSX runners failing for unrelated reasons): https://github.com/avar/git/actions/runs/2040223909 For a much more detailed summary of how the output looks before/after see v1[]. This series heavily conflicts with Johannes's js/ci-github-workflow-markup in "seen", but in the v1 I suggested basing that series on top of this one, because it can benefit a lot from these simplifications. I'll reply to this series with a proposed rebasing of that series on top of this one, which allows for removing almost all of its changes to "ci/" with no harm to its end-goals, i.e. the splitting up of "make" and "make test" output is something it'll get for free from this series. Junio: Since that series has been stalled on still-outstanding performance issues for a couple of months I was hoping we could queue this instead, and perhaps in addition if Johannes approves of the proposed re-roll on top of his. There's some forward progress on the performance issues (this[2] reply of Victoria Dye's from yesterday), but fully resolving those will probably take a bit... Whereas even though this one is relatively large I don't think there's anything controversial here. The one concern that's been raised has been Johannes's objection to removing some of the dead Azure code (which was needed to move forward here). I asked how he'd prefer to move forward with that in [3], but there hasn't been a reply to that in >1 month. I think just removing it is OK, we can always bring it back if needed, and doing so is actually going to be simpler on top of this since the CI is now less special, and leans more heavily on the logic of our normal build process. 1. https://lore.kernel.org/git/cover-00.25-00000000000-20220221T143936Z-avarab@gmail.com 2. https://lore.kernel.org/git/6b83bb83-32b9-20c9-fa02-c1c3170351c3@github.com/ 3. https://lore.kernel.org/git/220222.86y2236ndp.gmgdl@evledraar.gmail.com/ Ævar Arnfjörð Bjarmason (25): CI: run "set -ex" early in ci/lib.sh CI: make "$jobname" explicit, remove fallback CI: remove more dead Travis CI support CI: remove dead "tree skipping" code CI: remove unused Azure ci/* code CI: don't have "git grep" invoke a pager in tree content check CI: have "static-analysis" run a "make ci-static-analysis" target CI: have "static-analysis" run "check-builtins", not "documentation" CI: move p4 and git-lfs variables to ci/install-dependencies.sh CI: consistently use "export" in ci/lib.sh CI: export variables via a wrapper CI: remove "run-build-and-tests.sh", run "make [test]" directly CI: check ignored unignored build artifacts in "win[+VS] build" too CI: invoke "make artifacts-tar" directly in windows-build CI: split up and reduce "ci/test-documentation.sh" CI: combine ci/install{,-docker}-dependencies.sh CI: move "env" definitions into ci/lib.sh ci/run-test-slice.sh: replace shelling out with "echo" CI: pre-select test slice in Windows & VS tests CI: only invoke ci/lib.sh as "steps" in main.yml CI: narrow down variable definitions in --build and --test CI: add more variables to MAKEFLAGS, except under vs-build CI: set CC in MAKEFLAGS directly, don't add it to the environment CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case CI: don't use "set -x" in "ci/lib.sh" output .github/workflows/main.yml | 100 +++++--- Makefile | 31 ++- ci/check-directional-formatting.bash | 2 +- ci/check-unignored-build-artifacts.sh | 20 ++ ci/install-dependencies.sh | 53 ++++- ci/install-docker-dependencies.sh | 22 -- ci/lib-ci-type.sh | 10 + ci/lib-tput.sh | 2 + ci/lib.sh | 316 +++++++++++++------------- ci/make-test-artifacts.sh | 12 - ci/mount-fileshare.sh | 25 -- ci/print-test-failures.sh | 16 +- ci/run-build-and-tests.sh | 54 ----- ci/run-docker-build.sh | 66 ------ ci/run-docker.sh | 47 ---- ci/run-static-analysis.sh | 32 --- ci/run-test-slice.sh | 17 -- ci/select-test-slice.sh | 10 + ci/test-documentation.sh | 37 +-- ci/util/extract-trash-dirs.sh | 50 ---- shared.mak | 1 + 21 files changed, 346 insertions(+), 577 deletions(-) create mode 100755 ci/check-unignored-build-artifacts.sh delete mode 100755 ci/install-docker-dependencies.sh create mode 100644 ci/lib-ci-type.sh create mode 100644 ci/lib-tput.sh delete mode 100755 ci/make-test-artifacts.sh delete mode 100755 ci/mount-fileshare.sh delete mode 100755 ci/run-build-and-tests.sh delete mode 100755 ci/run-docker-build.sh delete mode 100755 ci/run-docker.sh delete mode 100755 ci/run-static-analysis.sh delete mode 100755 ci/run-test-slice.sh create mode 100755 ci/select-test-slice.sh delete mode 100755 ci/util/extract-trash-dirs.sh Range-diff against v1: 1: 970849a227f = 1: 4addbd70213 CI: run "set -ex" early in ci/lib.sh 2: eda7fb18064 = 2: b23aa99fd37 CI: make "$jobname" explicit, remove fallback 3: 3ee32815cf3 = 3: eec15a95879 CI: remove more dead Travis CI support 4: c81c1b3f504 = 4: 73c894f1665 CI: remove dead "tree skipping" code 5: 4738a22a36d = 5: b5e6d538554 CI: remove unused Azure ci/* code 6: 59e4f41e86c = 6: a4b9febbdca CI: don't have "git grep" invoke a pager in tree content check 7: 836ef20fdcc ! 7: 5d46d5b34c9 CI: have "static-analysis" run a "make ci-static-analysis" target @@ .github/workflows/main.yml: jobs: if: needs.ci-config.outputs.enabled == 'yes' ## Makefile ## -@@ Makefile: ifndef V - QUIET_HDR = @echo ' ' HDR $(<:hcc=h); - QUIET_RC = @echo ' ' RC $@; - QUIET_SPATCH = @echo ' ' SPATCH $<; -+ QUIET_CHECK = @echo ' ' CHECK $@; - QUIET_SUBDIR0 = +@subdir= - QUIET_SUBDIR1 = ;$(NO_SUBDIR) echo ' ' SUBDIR $$subdir; \ - $(MAKE) $(PRINT_DIR) -C $$subdir @@ Makefile: coccicheck: $(addsuffix .patch,$(filter-out %.pending.cocci,$(wildcard contrib/c # See contrib/coccinelle/README coccicheck-pending: $(addsuffix .patch,$(wildcard contrib/coccinelle/*.pending.cocci)) @@ ci/run-static-analysis.sh (deleted) - -make hdr-check || -exit 1 + + ## shared.mak ## +@@ shared.mak: ifndef V + QUIET_HDR = @echo ' ' HDR $(<:hcc=h); + QUIET_RC = @echo ' ' RC $@; + QUIET_SPATCH = @echo ' ' SPATCH $<; ++ QUIET_CHECK = @echo ' ' CHECK $@; + + ## Used in "Documentation/Makefile" + QUIET_ASCIIDOC = @echo ' ' ASCIIDOC $@; 8: 95cd496868e = 8: 81e06f13d84 CI: have "static-analysis" run "check-builtins", not "documentation" 9: a1d0796259e = 9: 3be91c26d44 CI: move p4 and git-lfs variables to ci/install-dependencies.sh 10: b6a61a786c5 = 10: 9dc148341ba CI: consistently use "export" in ci/lib.sh 11: f2fcee5d6e4 = 11: e9c7ba492e8 CI: export variables via a wrapper 12: dfd823f2e7d = 12: 86d5a48d59a CI: remove "run-build-and-tests.sh", run "make [test]" directly 13: 46459fff296 = 13: 649ad1ae249 CI: check ignored unignored build artifacts in "win[+VS] build" too 14: aecd3ebaafe = 14: b1b5b083389 CI: invoke "make artifacts-tar" directly in windows-build 15: 4f1564af70f = 15: dfa91ac8938 CI: split up and reduce "ci/test-documentation.sh" 16: 868613a5b06 = 16: ba4ed216769 CI: combine ci/install{,-docker}-dependencies.sh 17: 5d854e8ff36 = 17: b5e89a33340 CI: move "env" definitions into ci/lib.sh 18: a6106525b7f = 18: 571f4d0f441 ci/run-test-slice.sh: replace shelling out with "echo" 19: e9c6c4dd293 = 19: 2edea06ee4d CI: pre-select test slice in Windows & VS tests 20: 40d86e8c1dc = 20: ef9daa6882f CI: only invoke ci/lib.sh as "steps" in main.yml 21: abf9c504740 = 21: 44e3ace5fbe CI: narrow down variable definitions in --build and --test 22: 9f4c2798a82 = 22: 5f56b922e08 CI: add more variables to MAKEFLAGS, except under vs-build 23: 8a8b7ecf16b ! 23: b45b7cec94e CI: stop over-setting the $CC variable @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com> ## Commit message ## - CI: stop over-setting the $CC variable + CI: set CC in MAKEFLAGS directly, don't add it to the environment - As detailed in 2c8921db2b8 (travis-ci: build with the right compiler, - 2019-01-17) the reason we started using $CC in $MAKEFLAGS as opposed - to setting it in the environment was due to Travis CI clobbering $CC - in the environment. + Rather than pass a "$CC" and "$CC_PACKAGE" in the environment to be + picked up in ci/lib.sh let's instead have ci/lib.sh itself add it + directly to MAKEFLAGS. - We don't need to set it unconditionally to accomplish that, but rather - just have it set for those jobs that need them. E.g. the "win+VS - build" job confusingly has CC=gcc set, even though it builds with - MSVC. + Setting CC=gcc by default made for confusing trace output, and since a + preceding change to carry it and others over across "steps" in the + GitHub CI it's been even more misleading. E.g. the "win+VS build" job + confusingly has CC=gcc set, even though it builds with MSVC. + + Let's instead reply on the Makefile default of CC=cc, and only + override it for those jobs where it's needed. This does mean that + we'll need to set it for the "pedantic" job, which previously relied + on the default CC=gcc in case "clang" become the default on that + platform. This partially reverts my 707d2f2fe86 (CI: use "$runs_on_pool", not "$jobname" to select packages & config, 2021-11-23), i.e. we're now @@ ci/lib.sh: linux-TEST-vars) setenv --test GIT_TEST_DEFAULT_HASH sha256 ;; pedantic) ++ CC=gcc + # Don't run the tests; we only care about whether Git can be + # built. + setenv --build DEVOPTS pedantic @@ ci/lib.sh: linux-musl) MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8" ;; 24: d7b472b4a52 = 24: 06bf8d9f61b CI: set PYTHON_PATH setting for osx-{clang,gcc} into "$jobname" case 25: 08a9776c259 = 25: 6dc96ba8b82 CI: don't use "set -x" in "ci/lib.sh" output