Re: [PATCH] CI: add SANITIZE=[address|undefined] jobs

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

On Wed, Jul 27 2022, Jeff King wrote:

> On Tue, Jul 26, 2022 at 01:09:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Per [2] the GitHub fork of git.git runs with these in CI, so it's
>> already useful to some forks of this repository.
>
> Yeah, it has been helpful there and I think this is worth doing as part
> of our CI. It's a lot of CPU versus running the test suite once on
> Linux, but probably not compared to the overall cost of our current CI.
>
> For the GitHub fork, the code-coverage issues you noticed were easy: we
> only built one variant, so we could just test with those knobs. ;) But I
> tend to agree with your approach here to just test on one platform,
> which covers _most_ of the code. It's certainly better than the status
> quo, and it strikes a nice balance of CPU versus coverage.

*nod*

> One thing I'd say...
>
>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> index cd1f52692a5..4f59a7aa44c 100644
>> --- a/.github/workflows/main.yml
>> +++ b/.github/workflows/main.yml
>> @@ -251,6 +251,12 @@ jobs:
>>            - jobname: linux-leaks
>>              cc: gcc
>>              pool: ubuntu-latest
>> +          - jobname: SANITIZE=address
>> +            cc: gcc
>> +            pool: ubuntu-latest
>> +          - jobname: SANITIZE=undefined
>> +            cc: gcc
>> +            pool: ubuntu-latest
>
> There's really no reason to split the "address" and "undefined" builds
> into two jobs. We expect them to pass, and if one fails, having the
> results split is not likely to give any extra information. So I think
> one job with SANITIZE=address,undefined is fine, and reclaims some of
> the extra CPU we're spending.

I'll do that in a re-roll, pending a resolution of the naming discussion
at:
https://lore.kernel.org/git/220728.86sfmljhyx.gmgdl@evledraar.gmail.com/

But note that it *does* give you extra information to split them up
currently, i.e. the "test_expect_failure" that you get with "undefined"
isn't conflated with the non-changes that SANITIZE=address flags (sans
outstanding recent breakage) in the test report.

But just having that "TODO" test sitting there will suck less than
potentially having CI run much longer, or taking up resources from
concurrent CI runs, so I'll do this.

We also leave a lot of CI performance on the table by e.g. doing "chain
lint" in every single test run (except Windows), there *are* platform
edge-cases there like with SANITIZE=address, but I wonder if we should
just declare it good enough to do it in 1-2 jobs.

Ditto TEST_NO_MALLOC_CHECK=1 & --no-bin-wrappers, but we can think about
all of those some other time....

Thanks for reviewing this.