threads / patch / 57696

patchMakefile: add a prerequisite to the coverage-report target

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

## tl;dr

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

replies: 2people: 2as markdown or json

Elia Pinto· Apr 8, 2022, 10:54 UTC · lore

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(-)

Show changes to Makefile +1 −1

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

Junio C Hamano· Apr 8, 2022, 20:10 UTC · re: Elia Pinto · lore

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

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

Show 9 quoted lines

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

Elia Pinto· Apr 9, 2022, 03:51 UTC · re: Junio C Hamano · lore

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

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(-)

Show changes to Makefile +8 −1

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