Re: [PATCH 05/25] CI: remove unused Azure ci/* code

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

On Tue, Feb 22 2022, Johannes Schindelin wrote:

> Hi,
>
>
> On Mon, 21 Feb 2022, Ævar Arnfjörð Bjarmason wrote:
>
>> Remove Azure-specific code that's been unused since 6081d3898fe (ci:
>> retire the Azure Pipelines definition, 2020-04-11). As noted in a
>> larger removal of all of the Azure-supporting code in [1] (although
>> that missed some of this) there is more of it in-tree, but let's focus
>> on only the ci/* code for now.
>>
>> This is needed because in subsequent commits this unused code would
>> either need to be changed to accommodate changes in ci/*, or be
>> removed.
>>
>> As we'll see in those subsequent commits the end-state we're heading
>> towards will actually make it easier to add new CI types in the
>> future, even though the only one we're left with now is the GitHub
>> CI. I.e. the "ci/*" framework will be made to do less, not more. We'll
>> be offloading more of what it does to our generic build and test
>> system.
>>
>> While I'm at it (since the line needs to be touched anyway) change an
>> odd 'if test true == "$GITHUB_ACTIONS"' to the more usual style used
>> in other places of 'if test "$GITHUB_ACTIONS" = "true"'.
>>
>> 1. https://lore.kernel.org/git/patch-1.1-eec0a8c3164-20211217T000418Z-avarab@gmail.com/
>
> This has been discussed before, and I already gave my NAK.
>
> It is sad that I have to repeat myself: it is a good thing to have the
> Azure Pipelines definition as a fall-back. In the past, this has served us
> very well especially when we had to run a barrage of security fixes, for a
> slew of backports to previous release trains.
>
> You seem to have fun to just remove this code, under some assumption that
> it is not needed, despite me pointing out that it is needed.

I previously submitted a stand-alone patch to remove it[1] after the
removal of (most of) the Travis CI was merged in f9b889dd67b (Merge
branch 'ab/ci-updates', 2021-12-15).

I submitted that not for "fun", but because I had other CI/test-lib.sh
changes depending on that.

I.e. I needed to either patch code unused in-tree I couldn't test
without reverting your 6081d3898fe (ci: retire the Azure Pipelines
definition, 2020-04-11) (and presumably set up some Azure CI account
etc.), or remove it.

It's still unclear to me how Azure CI is being used as a "fall-back",
can you explain that? I.e. we don't have the azure-pipelines.yml anymore
(and neither does git-for-windows/git). Is this in-tree code being used
by some out-of-tree fork of git.git, or do you mean you'd like to keep
it in case we find a reason to revert your 6081d3898fe?

In this case (and as I think this series makes clear), we can't easily
keep certain Travis + Azure assumptions around *and* simplify how
ci/lib.sh works in the same way. At the tip of this series it's turned
into a very dumb variable setting helper with the CI recipe itself
driving the test.

Whereas Travis and Azure were using a feature where a "job" once running
would attempt to skip the rest of the CI run.

And in your [1] you mentioned the reasons for keeping it around being:
    
    The reason is that there are still some things that Azure Pipelines can do
    that GitHub workflows cannot, for example:
    
    - present the logs of failed tests in an intuitive manner,
    
    - re-run _only_ failed jobs.

It seems to me that in your parallel series you're working on 1/2 of
those, and with/without those changes we could otherwise improve the
presentation of the failed test runs enough with the "grouping"
etc. feature.

And as for 2/2 that any such support would be mostly orthogonal to
keeping Azure code that's currently unused around. I.e. a goal of this
series is also to make the CI less of a special snowflake, because I'm
not only interested in running the CI code on GitHub CI, but also
locally.

Which means that for any future port to Azure, GitLab CI etc. we'd
presumably just have a thin recipe that ran "make" followed by "make
test", and for a re-run of only that job it would just use the CI itself
to do that, without us needing to carry any special-cases in ci/*.

I'm not at all opposed to keeping Azure support in-tree. I specifically
have a problem with unused code holding other improvements
hostage.

I.e. neither I nor anyone else can easily change anything surrounding it
while being assured that we haven't broken that unused code. As noted in
04/25 I think you've also had that issue, i.e. your recent 0e7696c64db
(it seems to me) introduced what would have been a bug if it were
combined with the azure-pipelines.yml code removed in your 6081d3898fe.

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2112201834050.347@tvgsbejvaqbjf.bet/