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

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 29, 2019, 16:37 UTC
Message-ID: <a5ca8985-f877-b250-d186-32bef40092f5@gmail.com>
In-Reply-To: <20190129155827.GC13764@szeder.dev>

On 1/29/2019 10:58 AM, SZEDER Gábor wrote:

Show 7 quoted lines

> On Tue, Jan 29, 2019 at 06:56:08AM -0800, Derrick Stolee via GitGitGadget wrote:
>> +prove: all
>> +	$(MAKE) -C t/ prove
>> +
> 
> You don't need this 'prove' target in the "main" Makefile, because
> 'make test' will run the test suite using DEFAULT_TEST_TARGET anyway.

Thanks!

Show 21 quoted lines

>> +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.
>     
>     Instead of attempting to mess with the GIT_PROVE_OPTS, just force the
>     test target to 'test' so that we run under make, like we intended all
>     along.

Thanks for finding this!

> I'm afraid that this issue would badly affect 'coverage-prove' as well
> (I didn't try).  Or if doesn't (anymore?), then that should be
> mentioned in the commit message, and then perhaps it's time to remove
> that '-j1' from the 'coverage-test' target as well.

I'll fix this by forcing an update to GIT_PROVE_OPTS. It does limit our ability to use GIT_PROVE_OPTS as a pass-through, but at least this new target will have that assumption built in.

Thanks, -Stolee