[v2] ci(linux-asan-ubsan): let's save some time

Commit Message

Johannes Schindelin Aug. 29, 2023, 8:47 p.m. UTC

From: Johannes Schindelin <[e-mail]>

Every once in a while, the `git-p4` tests flake for reasons outside of
our control. It typically fails with "Connection refused" e.g. here:
https://github.com/git/git/actions/runs/5969707156/job/16196057724

	[...]
	+ git p4 clone --dest=/home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git //depot
	  Initialized empty Git repository in /home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git/.git/
	  Perforce client error:
		Connect to server failed; check $P4PORT.
		TCP connect to localhost:9807 failed.
		connect: 127.0.0.1:9807: Connection refused
	  failure accessing depot: could not run p4
	  Importing from //depot into /home/runner/work/git/git/t/trash directory.t9807-git-p4-submit/git
	 [...]

This happens in other jobs, too, but in the `linux-asan-ubsan` job it
hurts the most because that job often takes over a full hour to run,
therefore re-running a failed `linux-asan-ubsan` job is _very_ costly.

The purpose of the `linux-asan-ubsan` job is to exercise the C code of
Git, anyway, and any part of Git's source code that the `git-p4` tests
run and that would benefit from the attention of ASAN/UBSAN are run
better in other tests anyway, as debugging C code run via Python scripts
can get a bit hairy.

In fact, it is not even just `git-p4` that is the problem (even if it
flakes often enough to be problematic in the CI builds), but really the
part about Python scripts. So let's just skip any Python parts of the
tests from being run in that job.

For good measure, also skip the Subversion tests because debugging C
code run via Perl scripts is as much fun as debugging C code run via
Python scripts. And it will reduce the time this very expensive job
takes, which is a big benefit.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
    ci(linux-asan-ubsan): let's save some time
    
    I often look at failed CI runs, and the linux-asan-ubsan job comes up
    frequently, and painfully (because it takes such a long time that
    re-running is often less desirable than getting the CI runs to pass).
    
    This commit is an attempt to reduce the pain and suffering stemming from
    this particular job, simply by deciding that the benefit of running the
    Python/Subversion-related tests in that job is far outweighed by its
    cost.
    
    This commit not only reduces the number of git-p4 flakes in
    linux-asan-ubsan to 0, it also seems to shave off about 10 minutes
    runtime, comparing
    https://github.com/gitgitgadget/git/actions/runs/5929602548/job/16077585391
    to
    https://github.com/gitgitgadget/git/actions/runs/6010305446/job/16301473243?pr=1578
    (which is not quite scientific due to the lack of a controlled
    environment, but it's the best we got for now). Together, those benefits
    form a strong incentive for me to get this merged.
    
    This patch is based on maint.
    
    Changes since v1:
    
     * Made the rationale clearer (it is not Python that flakes, but
       Perforce).
     * Touched up the commit message.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1578%2Fdscho%2Fskip-p4-from-asan-runs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1578/dscho/skip-p4-from-asan-runs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1578

Range-diff vs v1:

 1:  ac5b82ea934 ! 1:  1927a97667b ci(linux-asan-ubsan): let's save some time
     @@ Commit message
          better in other tests anyway, as debugging C code run via Python scripts
          can get a bit hairy.
      
     -    In fact, it is not even `git-p4` that is the problem (even if it flakes
     -    often enough to be problematic in the CI builds), but really the part
     -    about Python scripts. So let's just skip any Python parts of the tests
     -    from being run in that job.
     +    In fact, it is not even just `git-p4` that is the problem (even if it
     +    flakes often enough to be problematic in the CI builds), but really the
     +    part about Python scripts. So let's just skip any Python parts of the
     +    tests from being run in that job.
      
          For good measure, also skip the Subversion tests because debugging C
          code run via Perl scripts is as much fun as debugging C code run via
     @@ ci/lib.sh: linux-leaks)
       	;;
       linux-asan-ubsan)
       	export SANITIZE=address,undefined
     -+	export NO_SVN_TESTS=LetsSaveSomeTimeBack
     -+	MAKEFLAGS="$MAKEFLAGS NO_PYTHON=YepItFlakesTooOften"
     ++	export NO_SVN_TESTS=LetsSaveSomeTime
     ++	MAKEFLAGS="$MAKEFLAGS NO_PYTHON=YepBecauseP4FlakesTooOften"
       	;;
       esac
       


 ci/lib.sh | 2 ++
 1 file changed, 2 insertions(+)


base-commit: 43c8a30d150ecede9709c1f2527c8fba92c65f40

Comments

Junio C Hamano Aug. 29, 2023, 9:51 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> ...
> In fact, it is not even just `git-p4` that is the problem (even if it
> flakes often enough to be problematic in the CI builds), but really the
> part about Python scripts. So let's just skip any Python parts of the
> tests from being run in that job.
>
> For good measure, also skip the Subversion tests because debugging C
> code run via Perl scripts is as much fun as debugging C code run via
> Python scripts. And it will reduce the time this very expensive job
> takes, which is a big benefit.
>
> Signed-off-by: Johannes Schindelin <[e-mail]>
> ---
> ...
>     Changes since v1:
>     
>      * Made the rationale clearer (it is not Python that flakes, but
>        Perforce).
>      * Touched up the commit message.

Thanks.  Merged to 'next'.

Jeff King Aug. 30, 2023, 12:28 a.m. UTC | #2

On Tue, Aug 29, 2023 at 08:47:28PM +0000, Johannes Schindelin via GitGitGadget wrote:

>     This commit is an attempt to reduce the pain and suffering stemming from
>     this particular job, simply by deciding that the benefit of running the
>     Python/Subversion-related tests in that job is far outweighed by its
>     cost.

FWIW, I am in favor of this patch. I've had the same thought many times
but was worried I was being too dismissive of p4/svn (which I personally
do not care at all about). Omitting them from the sanitizer job (but
still running the basic svn/p4 tests elsewhere) seems like a good
compromise.

-Peff

Junio C Hamano Aug. 30, 2023, 6:19 a.m. UTC | #3

Jeff King <[e-mail]> writes:

> On Tue, Aug 29, 2023 at 08:47:28PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>>     This commit is an attempt to reduce the pain and suffering stemming from
>>     this particular job, simply by deciding that the benefit of running the
>>     Python/Subversion-related tests in that job is far outweighed by its
>>     cost.
>
> FWIW, I am in favor of this patch. I've had the same thought many times
> but was worried I was being too dismissive of p4/svn (which I personally
> do not care at all about). Omitting them from the sanitizer job (but
> still running the basic svn/p4 tests elsewhere) seems like a good
> compromise.

Exactly my feeling.  Thanks, both.

Patch

diff --git a/ci/lib.sh b/ci/lib.sh
index 369d462f130..6fbb5bade12 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -280,6 +280,8 @@  linux-leaks)
 	;;
 linux-asan-ubsan)
 	export SANITIZE=address,undefined
+	export NO_SVN_TESTS=LetsSaveSomeTime
+	MAKEFLAGS="$MAKEFLAGS NO_PYTHON=YepBecauseP4FlakesTooOften"
 	;;
 esac