On November 20, 2021 4:28:30 AM GMT+01:00, "Ævar Arnfjörð Bjarmason" <avarab@xxxxxxxxx> wrote: > > CI: remove Travis CI support This is a patch I am in favor of, and would prefer in its own patch "series": separation of concern, and consideration in avoiding reviewer fatigue, are two aspects I would like to see you optimize for a bit more. > CI: use shorter names that fit in UX tooltips > CI: rename the "Linux32" job to lower-case "linux32" > CI: use "$runs_on_pool", not "$jobname" to select packages & config These strike me as simply shuffling things around and ramping up commit count in git/git, without actually addressing any of the problems our GitHub workflow _definitely_ has. One quite obvious problem is that any failing run is very cumbersome to diagnose, and you kind of have to know where you're looking. A troublesome and off-putting experience for newcomers (and even some oldtimers). You have to expand the print-failures step logs and search for "not ok" to get even close to the relevant part of the failing test case's details. Yes, addressing this would be much harder and take more effort than just going ahead and renaming things. It would also be much more useful. > CI: don't run "make test" twice in one job I am in favor of the idea. As is obvious from the fact that I already proposed this years ago. The commit message, however, is mum about that. And about the reasons why my proposal was shot down. And why those reasons should somehow no longer apply (and I would strongly suggest to aim for providing convincing evidence over mere opinions, to back up the patch). As has been mentioned before, this lack of diligence is disappointing. Reviewers should not be forced to look up previous related discussions on the Git mailing list. I would do that for a first-time contributor, but you are a long-term contributor who clearly has the ability, the knowledge, and the time, to accompany patches with such vital information. > CI: run "documentation" via run-build-and-test.sh This patch has a commit message that explains what the patch does, and describes a little bit of related commit history. It does not talk about any convincing reasons why the change should be desirable. This is troubling, in particular since it counteracts the major benefit of the preceding patch: to reduce the jobs' runtime. Also, while the preceding patch makes each job's focus more obvious, so that it no longer requires careful study of the entire test log merely to find out which `GIT_TEST_*` settings are set, _this_ patch crams the check-docs into the same job as the pretty unrelated test suite run. In other words, it combines even _more unrelated_ things into the same job. So what does this patch series try to accomplish? To shorten the jobs' runtime? Or to extend it? To have a clearer separation of concerns of the individual jobs? Or to muddy the waters by once again doing several things in the same job? It is quite confusing and strikes me as a patch series that could have used quite a bit more time and polishing and mailing list research and rearranging and splitting up and patch-dropping before unleashing it to the reviewers on the Git mailing list. It does not help that v2 seems to have been rushed out, and tripled in size, no less. If I had the task of wearing out reviewers, that is exactly the type of thing I would do. If I would accept the task. To end on a positive note: I suggest to split off the Travis CI patch. It should be relatively uncontroversial. Further, I suggest to find the previous patch that split `linux-gcc` on the mailing list, summarize what the conclusion was, and either adjust the equivalent patch in this series accordingly in order to contribute it stand-alone, or drop it. Then, you could use the considerable time you seem to have at your disposal on working towards teaching our workflow to present diagnosable information about build/test failures in a much more immediately consumable manner than it does right now. Ciao, Johannes Hi,
