| Message ID | 20220409043033.1288946-1-gitter.spiros@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] Makefile: add a prerequisite to the coverage-report target | expand |
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
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.
Æ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?
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.
Æ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.
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
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(-)