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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2022, 18:29 UTC
Message-ID: <xmqqv8vfiidm.fsf@gitster.g>
In-Reply-To: <20220409043033.1288946-1-gitter.spiros@gmail.com>

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

Show 15 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 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?

Show 42 quoted lines

>  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