Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets
- Date: Tue, 29 Jan 2019 11:37:34 -0500
- From: Derrick Stolee <stolee@xxxxxxxxx>
- Subject: Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets
On 1/29/2019 10:58 AM, SZEDER Gábor wrote:
> 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.
>> +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
> 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
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.