[PATCH] Makefile: add a prerequisite to the coverage-report target

[PATCH] Makefile: add a prerequisite to the coverage-report target

[Elia Pinto]

Directly invoking make coverage-report as a target results in an error because
its prerequisites are missing,

The patch adds the necessary prerequisite.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e8aba291d7..d9e8dd331e 100644
--- a/Makefile
+++ b/Makefile
@@ -3409,7 +3409,7 @@ coverage-prove: coverage-clean-results coverage-compile
 		DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
 		-j1 test
 
-coverage-report:
+coverage-report: coverage-test
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
-- 
2.35.1

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

[Junio C Hamano]

Elia Pinto <gitter.spiros@gmail.com> writes:

> @@ -3409,7 +3409,7 @@ coverage-prove: coverage-clean-results coverage-compile
>  		DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
>  		-j1 test
>  
> -coverage-report:
> +coverage-report: coverage-test
>  	$(QUIET_GCOV)for dir in $(object_dirs); do \
>  		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
>  	done

I am not sure if this alone makes it a good change.

With the current set-up, you can run coverage-test or coverage-prove
once, view coverage-report, shift your attention to elsewhere to
address issues you saw in the report, and then decide to view the
report again to remind yourself what you saw and what motivated you
to work on your changes.  Most likely this sequence would be
followed by another run of coverage-test followed by coverage-report
to compare what you saw before you made these changes with the
covernage report after your changes.

If this were conditional, i.e. "ah, I see you haven't run any
coverage test yet, so let me run it for you before showing the
result" combined with "ok, I see you did run coverage test, so let
me just show the result without running tests anew", then it would
be a strict improvement from the status quo.

But with the patch as posted, wouldn't we unconditonally wipe the
earlier result out?  If so that may negatively affect established
workflow of some people, I am afraid.

Thanks.

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

[Elia Pinto]

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

 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e8aba291d7..eacdffd748 100644
--- a/Makefile
+++ b/Makefile
@@ -3378,6 +3378,8 @@ check-builtins::
 .PHONY: coverage-untested-functions cover_db cover_db_html
 .PHONY: coverage-clean-results
 
+coverage-test.file?=coverage-test.file
+
 coverage:
 	$(MAKE) coverage-test
 	$(MAKE) coverage-untested-functions
@@ -3389,6 +3391,7 @@ coverage-clean-results:
 	$(RM) coverage-untested-functions
 	$(RM) -r cover_db/
 	$(RM) -r cover_db_html/
+	$(RM) -f $(coverage-test.file)
 
 coverage-clean: coverage-clean-results
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
@@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
 
+$(coverage-test.file):
+	@make coverage-test
+	touch $(coverage-test.file)
+
 coverage-prove: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
 		-j1 test
 
-coverage-report:
+coverage-report: $(coverage-test.file)
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
-- 
2.35.1

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

[Elia Pinto]

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

 Makefile | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e8aba291d7..eacdffd748 100644
--- a/Makefile
+++ b/Makefile
@@ -3378,6 +3378,8 @@ check-builtins::
 .PHONY: coverage-untested-functions cover_db cover_db_html
 .PHONY: coverage-clean-results
 
+coverage-test.file?=coverage-test.file
+
 coverage:
 	$(MAKE) coverage-test
 	$(MAKE) coverage-untested-functions
@@ -3389,6 +3391,7 @@ coverage-clean-results:
 	$(RM) coverage-untested-functions
 	$(RM) -r cover_db/
 	$(RM) -r cover_db_html/
+	$(RM) -f $(coverage-test.file)
 
 coverage-clean: coverage-clean-results
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
@@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
 
+$(coverage-test.file):
+	@make coverage-test
+	touch $(coverage-test.file)
+
 coverage-prove: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
 		-j1 test
 
-coverage-report:
+coverage-report: $(coverage-test.file)
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done
-- 
2.35.1

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

[Junio C Hamano]

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?



>  Makefile | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index e8aba291d7..eacdffd748 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3378,6 +3378,8 @@ check-builtins::
>  .PHONY: coverage-untested-functions cover_db cover_db_html
>  .PHONY: coverage-clean-results
>  
> +coverage-test.file?=coverage-test.file
> +
>  coverage:
>  	$(MAKE) coverage-test
>  	$(MAKE) coverage-untested-functions
> @@ -3389,6 +3391,7 @@ coverage-clean-results:
>  	$(RM) coverage-untested-functions
>  	$(RM) -r cover_db/
>  	$(RM) -r cover_db_html/
> +	$(RM) -f $(coverage-test.file)
>  
>  coverage-clean: coverage-clean-results
>  	$(RM) $(addsuffix *.gcno,$(object_dirs))
> @@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile
>  	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
>  		DEFAULT_TEST_TARGET=test -j1 test
>  
> +$(coverage-test.file):
> +	@make coverage-test
> +	touch $(coverage-test.file)
> +
>  coverage-prove: coverage-clean-results coverage-compile
>  	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
>  		DEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS="$(GIT_PROVE_OPTS) -j1" \
>  		-j1 test
>  
> -coverage-report:
> +coverage-report: $(coverage-test.file)
>  	$(QUIET_GCOV)for dir in $(object_dirs); do \
>  		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
>  	done

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

[Ævar Arnfjörð Bjarmason]

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

> 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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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 can see how this might lead to "Ah, *.coverage file exists so we
run report to show that existing result", but it is not reasonable
to say "we didn't touch t0001 so we do not have to rerun the script
under coverage-test" because whatever git subcommand we use may have
be updated (we _could_ describe the dependency fully so we only
re-run t0001 if any of t0001-init.sh, "git init", "git config", and
"git" is newer than the existing t0001.coverage; I do not know if
that is sensible, though).  And ...

> 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.

... I don't think overhead of gcov matters all that much.  Overhead
of "Having to" rerun tests primarily comes from running the tests,
with or without gcov enabled, so...

Or are you suggesting that we'd enable gcov in all our test runs by
default?

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

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> 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 can see how this might lead to "Ah, *.coverage file exists so we
> run report to show that existing result", but it is not reasonable
> to say "we didn't touch t0001 so we do not have to rerun the script
> under coverage-test" because whatever git subcommand we use may have
> be updated (we _could_ describe the dependency fully so we only
> re-run t0001 if any of t0001-init.sh, "git init", "git config", and
> "git" is newer than the existing t0001.coverage; I do not know if
> that is sensible, though).  And ...

I've done some experimenting with having "make -C t t0001-init.sh" only
run if the underlying code changed, which we can do by scraping the
trace2 output, generated*.d files, and e.g. making t0001-init.sh depend
on whatever builtin/init-db.c and the rest depend on.

But that's not what I'm talking about here, I'm just saying that we'd do
a normal "make test" where we write the gcov tests per-test into
t/test-results/t0001 and join them at the end of the run.

So the equivalent of a FORCE run, just like now.

>> 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.
>
> ... I don't think overhead of gcov matters all that much.  Overhead
> of "Having to" rerun tests primarily comes from running the tests,
> with or without gcov enabled, so...

No, on a multi-core machine the inability to run with -jN is the main
factor in making this run slow. E.g. on my 8 core box the tests run in
2-3 minutes with -j8, with -j1 it's 20-25 minutes.

(-j1 numbers from wetware memory, I didn't want to wait for that slower
run while writing this, which is pretty much the point...).

So I'm wondering if the desire to keep the old coverage report around is
synonymous with the current implementation running so slowly.

We could also make that work, e.g. with order-only dependencies, but if
this doesn't run in ~30m but instead in ~3m with -jN v.s. -j1 perhaps we
could just treat it like we do "make test" itself.

> Or are you suggesting that we'd enable gcov in all our test runs by
> default?

No, just that when you do want to generate this report that we can make
it happen much faster than we currently do.

The GCC manual discusses all the ways to easily make gcov work with
parallelism, these rules just didn't use those methods.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But that's not what I'm talking about here, I'm just saying that we'd do
> a normal "make test" where we write the gcov tests per-test into
> t/test-results/t0001 and join them at the end of the run.

OK, instead of per directory .gcov, we allow separate recording area
per test, ... 

> No, on a multi-core machine the inability to run with -jN is the main
> factor in making this run slow. E.g. on my 8 core box the tests run in
> 2-3 minutes with -j8, with -j1 it's 20-25 minutes.

... and that would make it easier to run the same binary from
different tests in parallel, which makes sense.

I missed that you were talking about running tests in parallel when
you brought up the "running tests can be made cheaper".

> So I'm wondering if the desire to keep the old coverage report around is
> synonymous with the current implementation running so slowly.

Possibly.