Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 29, 2019, 17:34 UTC
Message-ID: <20190129173409.GD13764@szeder.dev>
In-Reply-To: <20190129155827.GC13764@szeder.dev>

On Tue, Jan 29, 2019 at 04:58:27PM +0100, SZEDER Gábor wrote:

Show 22 quoted lines

> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
> > @@ -3077,6 +3080,10 @@ coverage-test: coverage-clean-results coverage-compile
> >  	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
> >  		DEFAULT_TEST_TARGET=test -j1 test
> >  
> > +coverage-prove: coverage-clean-results coverage-compile
> > +	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
> > +		DEFAULT_TEST_TARGET=prove -j1 prove
> 
> First I was wondering why do you need a dedicated 'coverage-prove'
> target, instead of letting DEFAULT_TEST_TARGET from the environment or
> from 'config.mak' do its thing.  But then I noticed in the hunk
> context, that, for some reason, the 'coverage-test' target hardcoded
> 'DEFAULT_TEST_TARGET=test -j1'.  Then I was wondering why would it
> want to do that, and stumbled upon commit c14cc77c11:
> 
>     coverage: set DEFAULT_TEST_TARGET to avoid using prove
>     
>     If the user sets DEFAULT_TEST_TARGET=prove in his config.mak, that
>     carries over into the coverage tests.  Which is really bad if he also
>     sets GIT_PROVE_OPTS=-j<..> as that completely breaks the coverage
>     runs.

So this text is really dramatic and implies (to me, at least), that parallelized coverage builds just don't work. I've just run a coverage build with this patch and my usual GIT_PROVE_OPTS containing '-j4', and the tests went well and the generated report looks good, too (I don't know how it's supposed to look, but at least I didn't notice anything obviously wrong with it). However, this might mean nothing, because further digging turned up the follow paragraph in 901c369af5 (Support coverage testing with GCC/gcov, 2009-02-19):

    The tests are run serially (with -j1).  The coverage code should
    theoretically allow concurrent access to its data files, but the
    author saw random test failures.  Obviously this could be
    improved.

And in the related email discussion [1]:

  But even though the docs claim it [-j<N>] should be possible,
  I've been getting "random" test failures when compiled with coverage
  support, that went away with -j1.  So the tests still run with -j1, as
  with the first version of the series.

So it doesn't seem to be that bad after all, because it's not "completely breaks" but "random test failures". Still far from ideal, but the original coverage patch is just about 3 weeks short of a decade old, so maybe things have improved since then, and it'd be worth a try to leave GIT_PROVE_OPTS as is and see what happens.

[1] https://public-inbox.org/git/200902191512.16755.trast@student.ethz.ch/