Re: [PATCH v2 0/6] CI: Remove Travis CI, shorten names for GH tooltips, split jobs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Nov 20 2021, Johannes Schindelin wrote:

> On November 20, 2021 4:28:30 AM GMT+01:00, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> 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.

It's included due to a comment on the v1 from Jeff King. I could have
gone either way, but I'm not going to change it around now.

I think it's fair enough to keep up the quality of series's, but I also
don't think it would help to go back & forth with including or not
including this travis removal.

If I don't include it I've got to patch "dead" code for the rest of the
series, if I re-roll and have two sets of patches depending on each
other that's its own reviewer fatigue etc.

For some counter-advice it would be nice to see you optimize for
responding to reviews, cf.:
https://lore.kernel.org/git/211118.86zgq14jp1.gmgdl@evledraar.gmail.com/
:)

>>  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.

FWIW I do have patches for that, but getting anything in takes time etc.

To do that properly needed to parse TAP, which I've got patches for in
pure-C (not Perl or whatever) locally.

One of the reasons I didn't submit that UX improvement yet is because I
know it'll run afoul of one of your hobby horses.

I.e. I'll either need to fix in-tree dead code relating to the test
suite's XML generation blindly (it's not used, so how am I going to test
it?), or argue with you about it being worth to remove it to move the
test suite UX forward without having to spending time on it.

Another is that someone (I think SZEDER, hopefully I'm not
misattributing there) pointed out that they liked to have the current
print-failure firehose of "set -x" output for both failures and
successes, and would object to e.g. something that just peeled out the
specific failure output.

I don't think that opinion is wrong (if I even understood it
correctl). I disagree, but I see how it's clearly a matter of
preference. Some would like that, some don't.

But whatever anyone's preference on that I think it clearly shows that
what you're suggesting here isn't as easy as you make it
out.

I.e. something to the effect of "instead of renaming things work on
<thing I consider a clear UX improvement>".

Even if I agree that it would be an improvement in this case others
won't, and getting that past review is a matter of arguing about that,
addressing feedback on that topic etc.

The reason I submitted this is because I thought "I'd like to rename
this thing, not because of bikeshedding, but because I literally cannot
see the relevant information" wouldn't be in any way contentious, but
here we are :)

If you'd like to test some of that referenced UX work out it's
avar/support-test-verbose-under-prove-2 in my git.git clone (the CI
changes aren't there yet, but they're made easy by the changes).

>>  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.

I think in terms of over-explaining something in commit messages &
including references to past commit I'm doing better than most in terms
of commit messages to this project.

But you've got to stop somewhere, exhaustive explanations of past
caveats etc. are also its own fatigue.

In this case I think the explanations I've provided as they stand
suffice. Curious archaeologists can always dig in the archives for more.

>>  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.

I just ejected this in v3, yeah it's a bit of a mixed bag with the
runtime of the installation.

The benefit, which I thought was a bit too obvious to even point out, is
that you can plainly see which half of asciidoc/asciidoctor fails, or
both. So it's clear if it's a compatibility issue or doc ource issue.

> 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.

The "check-docs" run was already there in the pre-image, so nothing
changed there.

I agree that it would be beneficial to e.g. split out all this ci/
script wrapping into "make" targets at the top-level, but that's a
separate future improvement.

> [...]