{"thread":{"id":"57701","subject":"[PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","startedAt":"2022-04-09T04:30:43Z","lastAt":"2022-04-12T16:03:05Z","messageCount":6,"participants":["Elia Pinto","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"453363","messageId":"20220409043033.1288946-1-gitter.spiros@gmail.com","threadId":"57701","inReplyTo":null,"subject":"[PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-04-09T04:30:33Z","receivedAt":"2022-04-09T04:30:43Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Directly invoking make coverage-report as a target results in an error because\nits prerequisites are missing,\n\nThis patch adds the compile-test prerequisite, which is run only once each time\nthe compile-report target is invoked. In practice, the developer may decide to\nreview the coverage-report results without necessarily rerunning for this\ncoverage-test, if it has already been run.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\nThis is the second version of the patch.\n\nWith respect to the first version, we tried to eliminate the inefficient\ncoverage-test invocation if the target is coverage-report, introducing a more\nuseful invocation order\n\n Makefile | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex e8aba291d7..eacdffd748 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3378,6 +3378,8 @@ check-builtins::\n .PHONY: coverage-untested-functions cover_db cover_db_html\n .PHONY: coverage-clean-results\n \n+coverage-test.file?=coverage-test.file\n+\n coverage:\n \t$(MAKE) coverage-test\n \t$(MAKE) coverage-untested-functions\n@@ -3389,6 +3391,7 @@ coverage-clean-results:\n \t$(RM) coverage-untested-functions\n \t$(RM) -r cover_db/\n \t$(RM) -r cover_db_html/\n+\t$(RM) -f $(coverage-test.file)\n \n coverage-clean: coverage-clean-results\n \t$(RM) $(addsuffix *.gcno,$(object_dirs))\n@@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=test -j1 test\n \n+$(coverage-test.file):\n+\t@make coverage-test\n+\ttouch $(coverage-test.file)\n+\n coverage-prove: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n \t\t-j1 test\n \n-coverage-report:\n+coverage-report: $(coverage-test.file)\n \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n \tdone\n-- \n2.35.1\n\n"},{"id":"453422","messageId":"xmqqv8vfiidm.fsf@gitster.g","threadId":"57701","inReplyTo":"20220409043033.1288946-1-gitter.spiros@gmail.com","subject":"Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-11T18:29:09Z","receivedAt":"2022-04-11T18:29:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> Directly invoking make coverage-report as a target results in an error because\n> its prerequisites are missing,\n>\n> This patch adds the compile-test prerequisite, which is run only once each time\n> the compile-report target is invoked. In practice, the developer may decide to\n> review the coverage-report results without necessarily rerunning for this\n> coverage-test, if it has already been run.\n>\n> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n> ---\n> This is the second version of the patch.\n>\n> With respect to the first version, we tried to eliminate the inefficient\n> coverage-test invocation if the target is coverage-report, introducing a more\n> useful invocation order\n\nThanks.\n\nI do not see why you want to make the name of coverage-test.file\ncustomizable.  In our makefiles, it seems that these \"stamp\" files\nare named with the \".made\" suffix\n\n   $ git grep -e '\\.made'\n\nso using hardcoded \"coverage-test.made\" (with something that removes\n*.made when \"make clean\" is run) may make it in line with the rest\nof the build procedure.\n\nÆvar, I had an impression that you had many Makefile patches either\nunsubmitted or work-in-progress in the dependency-tightening area,\nand am wondering if you find the dependencies (or the lack thereof)\nfor coverage-report target annoying.  Any good ideas?\n\n\n\n>  Makefile | 9 ++++++++-\n>  1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/Makefile b/Makefile\n> index e8aba291d7..eacdffd748 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3378,6 +3378,8 @@ check-builtins::\n>  .PHONY: coverage-untested-functions cover_db cover_db_html\n>  .PHONY: coverage-clean-results\n>  \n> +coverage-test.file?=coverage-test.file\n> +\n>  coverage:\n>  \t$(MAKE) coverage-test\n>  \t$(MAKE) coverage-untested-functions\n> @@ -3389,6 +3391,7 @@ coverage-clean-results:\n>  \t$(RM) coverage-untested-functions\n>  \t$(RM) -r cover_db/\n>  \t$(RM) -r cover_db_html/\n> +\t$(RM) -f $(coverage-test.file)\n>  \n>  coverage-clean: coverage-clean-results\n>  \t$(RM) $(addsuffix *.gcno,$(object_dirs))\n> @@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile\n>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>  \t\tDEFAULT_TEST_TARGET=test -j1 test\n>  \n> +$(coverage-test.file):\n> +\t@make coverage-test\n> +\ttouch $(coverage-test.file)\n> +\n>  coverage-prove: coverage-clean-results coverage-compile\n>  \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n>  \t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n>  \t\t-j1 test\n>  \n> -coverage-report:\n> +coverage-report: $(coverage-test.file)\n>  \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n>  \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n>  \tdone\n"},{"id":"453432","messageId":"220411.86fsmji970.gmgdl@evledraar.gmail.com","threadId":"57701","inReplyTo":"xmqqv8vfiidm.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-11T21:27:42Z","receivedAt":"2022-04-11T21:47:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 11 2022, Junio C Hamano wrote:\n\n> Elia Pinto <gitter.spiros@gmail.com> writes:\n>\n>> Directly invoking make coverage-report as a target results in an error because\n>> its prerequisites are missing,\n>>\n>> This patch adds the compile-test prerequisite, which is run only once each time\n>> the compile-report target is invoked. In practice, the developer may decide to\n>> review the coverage-report results without necessarily rerunning for this\n>> coverage-test, if it has already been run.\n>>\n>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>\n>> ---\n>> This is the second version of the patch.\n>>\n>> With respect to the first version, we tried to eliminate the inefficient\n>> coverage-test invocation if the target is coverage-report, introducing a more\n>> useful invocation order\n>\n> Thanks.\n>\n> I do not see why you want to make the name of coverage-test.file\n> customizable.  In our makefiles, it seems that these \"stamp\" files\n> are named with the \".made\" suffix\n>\n>    $ git grep -e '\\.made'\n>\n> so using hardcoded \"coverage-test.made\" (with something that removes\n> *.made when \"make clean\" is run) may make it in line with the rest\n> of the build procedure.\n>\n> Ævar, I had an impression that you had many Makefile patches either\n> unsubmitted or work-in-progress in the dependency-tightening area,\n> and am wondering if you find the dependencies (or the lack thereof)\n> for coverage-report target annoying.  Any good ideas?\n\nI haven't come up with a patch for these coverage targets, but I think\nit would be much more useful to:\n\n * Not have the target itself compile git, i.e. work like SANITIZE=leak,\n   there's no reason you shouldn't be able to e.g. combine the two\n   easily, it's just a complimentary set of flags.\n\n * We should be able to run this in parallel, see\n   e.g. https://stackoverflow.com/questions/14643589/code-coverage-using-gcov-on-parallel-run\n   for a trick for how to do that on gcc 9+, on older gcc GCOV_PREFIX*\n   can be used.\n\n   I.e. we'd save these in t/test-results/t0001.coverage or whatever,\n   and then \"join\" them at the end.\n\nI wonder if the issue this patch is trying to address would then just go\naway, i.e. isn't it OK that we'd re-run the tests to get the report\nthen? gcov doesn't add that much runtime overhead.\n"},{"id":"453434","messageId":"xmqqpmlnfcq1.fsf@gitster.g","threadId":"57701","inReplyTo":"220411.86fsmji970.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-11T22:59:34Z","receivedAt":"2022-04-11T22:59:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I haven't come up with a patch for these coverage targets, but I think\n> it would be much more useful to:\n>\n>  * Not have the target itself compile git, i.e. work like SANITIZE=leak,\n>    there's no reason you shouldn't be able to e.g. combine the two\n>    easily, it's just a complimentary set of flags.\n>\n>  * We should be able to run this in parallel, see\n>    e.g. https://stackoverflow.com/questions/14643589/code-coverage-using-gcov-on-parallel-run\n>    for a trick for how to do that on gcc 9+, on older gcc GCOV_PREFIX*\n>    can be used.\n>\n>    I.e. we'd save these in t/test-results/t0001.coverage or whatever,\n>    and then \"join\" them at the end.\n\nI can see how this might lead to \"Ah, *.coverage file exists so we\nrun report to show that existing result\", but it is not reasonable\nto say \"we didn't touch t0001 so we do not have to rerun the script\nunder coverage-test\" because whatever git subcommand we use may have\nbe updated (we _could_ describe the dependency fully so we only\nre-run t0001 if any of t0001-init.sh, \"git init\", \"git config\", and\n\"git\" is newer than the existing t0001.coverage; I do not know if\nthat is sensible, though).  And ...\n\n> I wonder if the issue this patch is trying to address would then just go\n> away, i.e. isn't it OK that we'd re-run the tests to get the report\n> then? gcov doesn't add that much runtime overhead.\n\n... I don't think overhead of gcov matters all that much.  Overhead\nof \"Having to\" rerun tests primarily comes from running the tests,\nwith or without gcov enabled, so...\n\nOr are you suggesting that we'd enable gcov in all our test runs by\ndefault?\n\n"},{"id":"453441","messageId":"220412.8635iiivgm.gmgdl@evledraar.gmail.com","threadId":"57701","inReplyTo":"xmqqpmlnfcq1.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-12T07:51:35Z","receivedAt":"2022-04-12T10:01:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 11 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>> I haven't come up with a patch for these coverage targets, but I think\n>> it would be much more useful to:\n>>\n>>  * Not have the target itself compile git, i.e. work like SANITIZE=leak,\n>>    there's no reason you shouldn't be able to e.g. combine the two\n>>    easily, it's just a complimentary set of flags.\n>>\n>>  * We should be able to run this in parallel, see\n>>    e.g. https://stackoverflow.com/questions/14643589/code-coverage-using-gcov-on-parallel-run\n>>    for a trick for how to do that on gcc 9+, on older gcc GCOV_PREFIX*\n>>    can be used.\n>>\n>>    I.e. we'd save these in t/test-results/t0001.coverage or whatever,\n>>    and then \"join\" them at the end.\n>\n> I can see how this might lead to \"Ah, *.coverage file exists so we\n> run report to show that existing result\", but it is not reasonable\n> to say \"we didn't touch t0001 so we do not have to rerun the script\n> under coverage-test\" because whatever git subcommand we use may have\n> be updated (we _could_ describe the dependency fully so we only\n> re-run t0001 if any of t0001-init.sh, \"git init\", \"git config\", and\n> \"git\" is newer than the existing t0001.coverage; I do not know if\n> that is sensible, though).  And ...\n\nI've done some experimenting with having \"make -C t t0001-init.sh\" only\nrun if the underlying code changed, which we can do by scraping the\ntrace2 output, generated*.d files, and e.g. making t0001-init.sh depend\non whatever builtin/init-db.c and the rest depend on.\n\nBut that's not what I'm talking about here, I'm just saying that we'd do\na normal \"make test\" where we write the gcov tests per-test into\nt/test-results/t0001 and join them at the end of the run.\n\nSo the equivalent of a FORCE run, just like now.\n\n>> I wonder if the issue this patch is trying to address would then just go\n>> away, i.e. isn't it OK that we'd re-run the tests to get the report\n>> then? gcov doesn't add that much runtime overhead.\n>\n> ... I don't think overhead of gcov matters all that much.  Overhead\n> of \"Having to\" rerun tests primarily comes from running the tests,\n> with or without gcov enabled, so...\n\nNo, on a multi-core machine the inability to run with -jN is the main\nfactor in making this run slow. E.g. on my 8 core box the tests run in\n2-3 minutes with -j8, with -j1 it's 20-25 minutes.\n\n(-j1 numbers from wetware memory, I didn't want to wait for that slower\nrun while writing this, which is pretty much the point...).\n\nSo I'm wondering if the desire to keep the old coverage report around is\nsynonymous with the current implementation running so slowly.\n\nWe could also make that work, e.g. with order-only dependencies, but if\nthis doesn't run in ~30m but instead in ~3m with -jN v.s. -j1 perhaps we\ncould just treat it like we do \"make test\" itself.\n\n> Or are you suggesting that we'd enable gcov in all our test runs by\n> default?\n\nNo, just that when you do want to generate this report that we can make\nit happen much faster than we currently do.\n\nThe GCC manual discusses all the ways to easily make gcov work with\nparallelism, these rules just didn't use those methods.\n"},{"id":"453447","messageId":"xmqqh76ycmrz.fsf@gitster.g","threadId":"57701","inReplyTo":"220412.8635iiivgm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-12T16:02:56Z","receivedAt":"2022-04-12T16:03:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But that's not what I'm talking about here, I'm just saying that we'd do\n> a normal \"make test\" where we write the gcov tests per-test into\n> t/test-results/t0001 and join them at the end of the run.\n\nOK, instead of per directory .gcov, we allow separate recording area\nper test, ... \n\n> No, on a multi-core machine the inability to run with -jN is the main\n> factor in making this run slow. E.g. on my 8 core box the tests run in\n> 2-3 minutes with -j8, with -j1 it's 20-25 minutes.\n\n... and that would make it easier to run the same binary from\ndifferent tests in parallel, which makes sense.\n\nI missed that you were talking about running tests in parallel when\nyou brought up the \"running tests can be made cheaper\".\n\n> So I'm wondering if the desire to keep the old coverage report around is\n> synonymous with the current implementation running so slowly.\n\nPossibly.\n"}]}