threads / patch / 57729

v3Makefile: add a prerequisite to the coverage-report target

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

## tl;dr

3 messages between Apr 14, 2022 and Apr 14, 2022. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Elia Pinto· Apr 14, 2022, 02:25 UTC · lore

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 third revision of the patch. Compared to the second:

- deleted the Makefile coverage-test.file variable, replaced directly by the
  coverage-test.made file
- added in coverage-compile as last command the touch of coverage-test.made, so
  that even if coverage-test is run first, invoking indirectly it by the first coverage-report does not
  call coverage-test and so the test suite gain.

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

Show changes to Makefile +7 −1

diff --git a/Makefile b/Makefile
index f8bccfab5e..d6ae228c74 100644
--- a/Makefile
+++ b/Makefile
@@ -3407,6 +3407,7 @@ coverage-clean-results:
 	$(RM) coverage-untested-functions
 	$(RM) -r cover_db/
 	$(RM) -r cover_db_html/
+	$(RM) coverage-test.made
 
 coverage-clean: coverage-clean-results
 	$(RM) $(addsuffix *.gcno,$(object_dirs))
@@ -3421,13 +3422,18 @@ coverage-compile:
 coverage-test: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
 		DEFAULT_TEST_TARGET=test -j1 test
+	touch coverage-test.made
+
+coverage-test.made:
+	@make coverage-test
+	touch coverage-test.made
 
 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.made
 	$(QUIET_GCOV)for dir in $(object_dirs); do \
 		$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \
 	done

-- 
2.35.1

Junio C Hamano· Apr 14, 2022, 05:06 UTC · re: Elia Pinto · lore

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

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

Show 41 quoted lines

> 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 third revision of the patch. Compared to the second:
>
> - deleted the Makefile coverage-test.file variable, replaced directly by the
>   coverage-test.made file
> - added in coverage-compile as last command the touch of coverage-test.made, so
>   that even if coverage-test is run first, invoking indirectly it by the first coverage-report does not
>   call coverage-test and so the test suite gain.
>
>  Makefile | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index f8bccfab5e..d6ae228c74 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3407,6 +3407,7 @@ coverage-clean-results:
>  	$(RM) coverage-untested-functions
>  	$(RM) -r cover_db/
>  	$(RM) -r cover_db_html/
> +	$(RM) coverage-test.made
>  
>  coverage-clean: coverage-clean-results
>  	$(RM) $(addsuffix *.gcno,$(object_dirs))
> @@ -3421,13 +3422,18 @@ coverage-compile:
>  coverage-test: coverage-clean-results coverage-compile
>  	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
>  		DEFAULT_TEST_TARGET=test -j1 test
> +	touch coverage-test.made
> +
> +coverage-test.made:
> +	@make coverage-test

s/@make/$(MAKE)/, I think.

> +	touch coverage-test.made

Other than that, looking good.

Thanks.

Junio C Hamano· Apr 14, 2022, 05:16 UTC · re: Elia Pinto · lore

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

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

Show 9 quoted lines

> @@ -3421,13 +3422,18 @@ coverage-compile:
>  coverage-test: coverage-clean-results coverage-compile
>  	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \
>  		DEFAULT_TEST_TARGET=test -j1 test
> +	touch coverage-test.made
> +
> +coverage-test.made:
> +	@make coverage-test
> +	touch coverage-test.made

As this target runs "$(MAKE) coverage-test", at the end of which the .made file is touched anyway, I wonder if we need the second line here.

I just tried to run this sequence:

    $ make distclean
    $ make coverage-report
    $ make coverage-report
    $ make coverage-test

without the "touch" in the recipe for the coverage-test.made target, and it behaved as I expected them to, i.e.

    * got an pristine tree
    * built, ran tests and showed report
    * showed report without building or running testing
    * cleared the gcov files, ran tests and showed report

So, perhaps this patch on top?

Thanks.

 Makefile | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Show changes to diff +1 −2

diff --git i/Makefile w/Makefile
index fa8cc86276..72ae7fa886 100644
--- i/Makefile
+++ w/Makefile
@@ -3416,8 +3416,7 @@ coverage-test: coverage-clean-results coverage-compile
 	touch coverage-test.made
 
 coverage-test.made:
-	@make coverage-test
-	touch coverage-test.made
+	$(MAKE) coverage-test
 
 coverage-prove: coverage-clean-results coverage-compile
 	$(MAKE) CFLAGS="$(COVERAGE_CFLAGS)" LDFLAGS="$(COVERAGE_LDFLAGS)" \