Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets
- Date: Tue, 29 Jan 2019 11:46:54 -0500
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH 1/1] Makefile: add prove and coverage-prove targets
On Tue, Jan 29, 2019 at 11:35:41AM -0500, Derrick Stolee wrote:
> > 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).
> >> +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.
> > 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.