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

From: Jeff King <peff@peff.net>
Date: Jan 29, 2019, 16:46 UTC
Message-ID: <20190129164654.GA6991@sigill.intra.peff.net>
In-Reply-To: <bb17112d-2869-741f-de13-3971a995032e@gmail.com>

On Tue, Jan 29, 2019 at 11:35:41AM -0500, Derrick Stolee wrote:

Show 7 quoted lines

> > For people who don't have "prove" available, I think they could just do
> > "make -k test" to make sure the full suite runs. Should we perhaps be
> > doing that automatically in the sub-make run by coverage-test?
> 
> I wanted to avoid changing the existing behavior, if I could. But, if
> we can reasonably assume that anyone running 'make coverage-test' wants
> to run the full suite even with failures, then that's fine by me.

Another option would be to relay "-k" from the caller. I think it's not enough to just use $(MAKE), but if you use $(MAKE) $(MAKEFLAGS), then running "make -k coverage-test" from your coverage script would (I think) do what you want.

> I see from the make docs that '-k' will still result in an error code
> at the end of the command, so no automation would result in an incorrect
> response to a failed test. Am I correct?

Yeah, that matches my understanding. I don't think you'd have to deal with that failure code manually for coverage-report because it does not depend on coverage-test (but obviously if you did "make coverage-test && make coverage-report", the "&&" needs to become a semicolon).

Show 12 quoted lines

> >> +coverage-prove: coverage-clean-results coverage-compile
> >> +	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
> >> +		DEFAULT_TEST_TARGET=prove -j1 prove
> >> +
> > 
> > You probably don't need to override DEFAULT_TEST_TARGET here, since the
> > "prove" target doesn't look at it. Likewise, "-j1" probably does nothing
> > here, since prove itself is a single target.
> 
> As Szeder mentioned, I can probably just drop the 'prove' target and use
> DEFAULT_TEST_TARGET instead. Or do we think anyone will want to use
> 'make prove' from root?

Yeah, that works. I typically do run prove, and I do run the tests from the root, but DEFAULT_TEST_TARGET already makes it all work for me. So yeah, I think it's fine to leave off the prove target until somebody actually wants it.

Show 6 quoted lines

> > I'm not sure why we want to enforce -j1 for these targets, but if it's
> > important to do so for the prove case, as well, you'd need to add it to
> > GIT_PROVE_OPTS.
> 
> The '-j1' is necessary because the coverage data is collected in a way that
> is not thread-safe. Our compile options also force single-threaded behavior.

Ah, right, I vaguely recall that now.

> I'll specifically override GIT_PROVE_OPTS here to force -j1, but also send
> -j1 to the 'make' command, too.

Makes sense.

-Peff