Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 11, 2022, 21:27 UTC
Message-ID: <220411.86fsmji970.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqv8vfiidm.fsf@gitster.g>

On Mon, Apr 11 2022, Junio C Hamano wrote:

Show 34 quoted lines

> Elia Pinto <gitter.spiros@gmail.com> writes:
>
>> Directly invoking make coverage-report as a target results in an error because
>> its prerequisites are missing,
>>
>> This patch adds the compile-test prerequisite, which is run only once each time
>> the compile-report target is invoked. In practice, the developer may decide to
>> review the coverage-report results without necessarily rerunning for this
>> coverage-test, if it has already been run.
>>
>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
>> ---
>> This is the second version of the patch.
>>
>> With respect to the first version, we tried to eliminate the inefficient
>> coverage-test invocation if the target is coverage-report, introducing a more
>> useful invocation order
>
> Thanks.
>
> I do not see why you want to make the name of coverage-test.file
> customizable.  In our makefiles, it seems that these "stamp" files
> are named with the ".made" suffix
>
>    $ git grep -e '\.made'
>
> so using hardcoded "coverage-test.made" (with something that removes
> *.made when "make clean" is run) may make it in line with the rest
> of the build procedure.
>
> Ævar, I had an impression that you had many Makefile patches either
> unsubmitted or work-in-progress in the dependency-tightening area,
> and am wondering if you find the dependencies (or the lack thereof)
> for coverage-report target annoying.  Any good ideas?

I haven't come up with a patch for these coverage targets, but I think it would be much more useful to:

 * Not have the target itself compile git, i.e. work like SANITIZE=leak,
   there's no reason you shouldn't be able to e.g. combine the two
   easily, it's just a complimentary set of flags.

 * We should be able to run this in parallel, see
   e.g. https://stackoverflow.com/questions/14643589/code-coverage-using-gcov-on-parallel-run
   for a trick for how to do that on gcc 9+, on older gcc GCOV_PREFIX*
   can be used.

   I.e. we'd save these in t/test-results/t0001.coverage or whatever,
   and then "join" them at the end.

I wonder if the issue this patch is trying to address would then just go away, i.e. isn't it OK that we'd re-run the tests to get the report then? gcov doesn't add that much runtime overhead.