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@xxxxxxxxx/ > 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.
