Re: [PATCH] ci: add address and undefined sanitizer tasks

From: Jeff King <peff@peff.net>
Date: Oct 11, 2022, 00:00 UTC
Message-ID: <Y0SyH34tdXQK2qhA@coredump.intra.peff.net>
In-Reply-To: <xmqqtu4bz2fw.fsf@gitster.g>

On Mon, Oct 10, 2022 at 04:25:23PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > ---
> >  * I've been running my local post-integration-pre-pushout tests of
> >    'seen' with these two sanitizer tests, which has saved me from a
> >    few potential embarrassments early.  As it takes a lot extra time
> >    to run these locally, I am aiming to burden contributors who run
> >    their due diligence "before sending the patch" checks using the
> >    GitHub Actions CI ;-).
> >
> >    The way the patch adds jobs to CI just imitates how -leaks one is
> >    defined.
> 
> One downside is that the usual CI cycle for a branch that takes a
> bit shorter than 40 minutes seems to take between 50 to 60 minutes
> (the primary culprit seems to be the address sanitizer).
> 
> Arguably, that 10 or 20 minutes are time saved from human
> developers' time, so it might not be too bad, but I'll keep
> it out of 'next' for now.

You can make it a bit faster by running both at once as SANITIZE=address,undefined.

Since we expect both to pass cleanly, there's really no other complication; the user will either see errors (and correct them) or they won't. The signal of "passed with asan, but not ubsan" (or vice versa) is not that useful in practice.

In the long run, I hope that "leaks" can run in the same way, but we're not there yet since there's a lot of selective test-running. In theory the regular linux test run is not necessary with this job, but I think the signal for "broke with/without sanitizers" is a little higher (and anyway, we have to run the regular suite on a zillion other platforms, too).

-Peff