{"thread":{"id":"54581","subject":"[PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","startedAt":"2020-11-05T21:03:52Z","lastAt":"2020-11-06T21:43:41Z","messageCount":7,"participants":["Ramsay Jones","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"409223","messageId":"32b7e1dc-8cc3-2236-08b8-4d268bbf8c0b@ramsayjones.plus.com","threadId":"54581","inReplyTo":null,"subject":"[PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-05T21:03:49Z","receivedAt":"2020-11-05T21:03:52Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nThe 'clean' target is still noticeably slow on cygwin, despite the\nsubstantial improvement made by the previous patch. For example, the\nsecond invocation of 'make clean' below:\n\n $ make clean >/dev/null 2>&1\n $ make clean\n ...\n make[1]: Entering directory '/home/ramsay/git/Documentation'\n make[2]: Entering directory '/home/ramsay/git'\n make[2]: 'GIT-VERSION-FILE' is up to date.\n make[2]: Leaving directory '/home/ramsay/git'\n ...\n $\n\nhas been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz,\n8GB RAM, 1TB HDD).\n\nNotice that the 'clean' target is making a nested call to the parent\nMakefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to\nthe previous patch, there would have been _two_ such invocations).\nThis is to ensure that the $(GIT_VERSION) make variable is set, once\nthat file had been included. However, the 'clean' target does not use\nthe $(GIT_VERSION) variable, so this is wasted effort.\n\nIn order to eliminate such wasted effort, use the value of the internal\n$(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the\ntarget is not 'clean'. (This drops the time down to 10.361s, on my laptop,\ngiving an improvement of 16.20%).\n\nSigned-off-by: Ramsay Jones \n---\n Documentation/Makefile | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/Documentation/Makefile b/Documentation/Makefile\nindex 652d57a1b6..5c680024eb 100644\n--- a/Documentation/Makefile\n+++ b/Documentation/Makefile\n@@ -272,7 +272,9 @@ install-html: html\n ../GIT-VERSION-FILE: FORCE\n \t$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE\n \n+ifneq ($(MAKECMDGOALS),clean)\n -include ../GIT-VERSION-FILE\n+endif\n \n #\n # Determine \"include::\" file references in asciidoc files.\n-- \n2.29.0\n"},{"id":"409236","messageId":"xmqqy2jfh3nz.fsf@gitster.c.googlers.com","threadId":"54581","inReplyTo":"32b7e1dc-8cc3-2236-08b8-4d268bbf8c0b@ramsayjones.plus.com","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-05T21:52:48Z","receivedAt":"2020-11-05T21:52:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> The 'clean' target is still noticeably slow on cygwin, despite the\n> substantial improvement made by the previous patch. For example, the\n> second invocation of 'make clean' below:\n>\n> $ make clean >/dev/null 2>&1\n> $ make clean\n> ...\n> make[1]: Entering directory '/home/ramsay/git/Documentation'\n> make[2]: Entering directory '/home/ramsay/git'\n> make[2]: 'GIT-VERSION-FILE' is up to date.\n> make[2]: Leaving directory '/home/ramsay/git'\n> ...\n> $\n>\n> has been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz,\n> 8GB RAM, 1TB HDD).\n>\n> Notice that the 'clean' target is making a nested call to the parent\n> Makefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to\n> the previous patch, there would have been _two_ such invocations).\n> This is to ensure that the $(GIT_VERSION) make variable is set, once\n> that file had been included. However, the 'clean' target does not use\n> the $(GIT_VERSION) variable, so this is wasted effort.\n>\n> In order to eliminate such wasted effort, use the value of the internal\n> $(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the\n> target is not 'clean'. (This drops the time down to 10.361s, on my laptop,\n> giving an improvement of 16.20%).\n\nThis obviously relies on the fact that none of our build products to\nbe cleaned are named using $(GIT_VERSION)---in other words, if our\ncleaning rule contained\n\n\trm -f git-$(GIT_VERSION)-manual.html\n\nthis optimization would not work well.\n\nLuckily, I think we use GIT_VERSION only to engrave version number\nin the resulting document, and it does not affect the name of the\nbuild product, so this change is safe, I think.\n\n> Signed-off-by: Ramsay Jones \n> ---\n> Documentation/Makefile | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 652d57a1b6..5c680024eb 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -272,7 +272,9 @@ install-html: html\n> ../GIT-VERSION-FILE: FORCE\n> \t$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE\n> \n> +ifneq ($(MAKECMDGOALS),clean)\n> -include ../GIT-VERSION-FILE\n> +endif\n> \n> #\n> # Determine \"include::\" file references in asciidoc files.\n"},{"id":"409259","messageId":"2f55ac8d-bcbc-5b4b-a790-4a749b369db0@ramsayjones.plus.com","threadId":"54581","inReplyTo":"xmqqy2jfh3nz.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T01:30:30Z","receivedAt":"2020-11-06T01:30:34Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/11/2020 21:52, Junio C Hamano wrote:\n> Ramsay Jones writes:\n> \n>> The 'clean' target is still noticeably slow on cygwin, despite the\n>> substantial improvement made by the previous patch. For example, the\n>> second invocation of 'make clean' below:\n>>\n>> $ make clean >/dev/null 2>&1\n>> $ make clean\n>> ...\n>> make[1]: Entering directory '/home/ramsay/git/Documentation'\n>> make[2]: Entering directory '/home/ramsay/git'\n>> make[2]: 'GIT-VERSION-FILE' is up to date.\n>> make[2]: Leaving directory '/home/ramsay/git'\n>> ...\n>> $\n>>\n>> has been timed at 12.364s on my laptop (on old core i5-4200M @ 2.50GHz,\n>> 8GB RAM, 1TB HDD).\n>>\n>> Notice that the 'clean' target is making a nested call to the parent\n>> Makefile to ensure that the GIT-VERSION-FILE is up-to-date (prior to\n>> the previous patch, there would have been _two_ such invocations).\n>> This is to ensure that the $(GIT_VERSION) make variable is set, once\n>> that file had been included. However, the 'clean' target does not use\n>> the $(GIT_VERSION) variable, so this is wasted effort.\n>>\n>> In order to eliminate such wasted effort, use the value of the internal\n>> $(MAKECMDGOALS) variable to only '-include ../GIT-VERSION-FILE' when the\n>> target is not 'clean'. (This drops the time down to 10.361s, on my laptop,\n>> giving an improvement of 16.20%).\n> \n> This obviously relies on the fact that none of our build products to\n> be cleaned are named using $(GIT_VERSION)---in other words, if our\n> cleaning rule contained\n> \n> \trm -f git-$(GIT_VERSION)-manual.html\n> \n> this optimization would not work well.\n\nYes, indeed.\n\n> Luckily, I think we use GIT_VERSION only to engrave version number\n> in the resulting document, and it does not affect the name of the\n> build product, so this change is safe, I think.\n\nYes, as I said in the commit message above:\n\n \"However, the 'clean' target does not use\n the $(GIT_VERSION) variable, so this is wasted effort.\"\n\nThanks.\n\nATB,\nRamsay Jones\n"},{"id":"409283","messageId":"20201106181804.GA183267@coredump.intra.peff.net","threadId":"54581","inReplyTo":"32b7e1dc-8cc3-2236-08b8-4d268bbf8c0b@ramsayjones.plus.com","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-06T18:18:04Z","receivedAt":"2020-11-06T18:18:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 05, 2020 at 09:03:49PM +0000, Ramsay Jones wrote:\n\n> diff --git a/Documentation/Makefile b/Documentation/Makefile\n> index 652d57a1b6..5c680024eb 100644\n> --- a/Documentation/Makefile\n> +++ b/Documentation/Makefile\n> @@ -272,7 +272,9 @@ install-html: html\n> ../GIT-VERSION-FILE: FORCE\n> \t$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE\n> \n> +ifneq ($(MAKECMDGOALS),clean)\n> -include ../GIT-VERSION-FILE\n> +endif\n\nCalling out \"clean\" here specially feels somewhat backwards to me, in\nterms of Makefile design. In an ideal world we provide all of the\ndependencies to \"make\", and based on the targets we are building, it\ndecides what needs to be done.\n\nThis works with normal targets, obviously, but also with variables. If\nwe do:\n\n FOO = $(shell do-the-thing)\n\nthen we execute that command only when $(FOO) is needed[1].\n\nBut \"include\" here is tricky. It is loaded regardless of whether the\nvalues it contains are needed or not. I wonder if we could do better by\ngiving make more information about what we're expecting to get from it.\nI.e., if we wrote:\n\n GIT_VERSION = $(shell awk '{print $3}' GIT-VERSION-FILE)\n\nThen \"make clean\", not needing the value of $(GIT_VERSION), wouldn't run\nthat shell snippet at all. Of course there's a catch; we are also\nrelying on the include to trigger the dependency. So it is really more\nlike:\n\n GIT_VERSION = $(shell make GIT-VERSION-FILE && awk '{print $3}' GIT-VERSION-FILE)\n\nI'm not sure how bad that is. Re-invoking make seems like it could get\nexpensive, especially for the common case that we're building actual\nbinaries and we _do_ need the version. But we could probably cut \"make\"\nout of the loop entirely. Generating GIT-VERSION-FILE is already a FORCE\ntarget, so really:\n\n GIT_VERSION = $(shell ./GIT-VERSION-GEN)\n\nwould be equivalent-ish (with some output changes, and possibly we'd\nwant to stash the value in a file for any other scripts to make use of).\n\nThis is all just stuff I've written in my editor and not tried. I won't\nbe surprised if there are some gotchas. But it at least seems like a\nconceptually cleaner path.\n\n-Peff\n\n[1] Variable assignment actually has a slight problem in the opposite\n direction: it wants to run the shell snippet every time the variable\n is referenced. There's a trick to get around that described in\n 0573831950 (Makefile: avoid running curl-config unnecessarily,\n 2020-04-04).\n\n It's built around evals. In fact, I suspect you could build a\n function around eval that actually works similar to include, but\n lazy-loads the file only when one of its stubs is referenced. I.e.,\n something like:\n\n GIT_VERSION = $(eval include GIT-VERSION-FILE)\n\n would probably work (and for other includes, multiple variables\n could mention the same file; as soon as it gets included, it\n overwrites the stubs).\n"},{"id":"409291","messageId":"a654c95f-2367-5805-d4f8-56c466a141d4@ramsayjones.plus.com","threadId":"54581","inReplyTo":"20201106181804.GA183267@coredump.intra.peff.net","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2020-11-06T19:38:06Z","receivedAt":"2020-11-06T19:38:12Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/11/2020 18:18, Jeff King wrote:\n> On Thu, Nov 05, 2020 at 09:03:49PM +0000, Ramsay Jones wrote:\n> \n>> diff --git a/Documentation/Makefile b/Documentation/Makefile\n>> index 652d57a1b6..5c680024eb 100644\n>> --- a/Documentation/Makefile\n>> +++ b/Documentation/Makefile\n>> @@ -272,7 +272,9 @@ install-html: html\n>> ../GIT-VERSION-FILE: FORCE\n>> \t$(QUIET_SUBDIR0)../ $(QUIET_SUBDIR1) GIT-VERSION-FILE\n>> \n>> +ifneq ($(MAKECMDGOALS),clean)\n>> -include ../GIT-VERSION-FILE\n>> +endif\n> \n> Calling out \"clean\" here specially feels somewhat backwards to me, in\n> terms of Makefile design. In an ideal world we provide all of the\n> dependencies to \"make\", and based on the targets we are building, it\n> decides what needs to be done.\n> \n> This works with normal targets, obviously, but also with variables. If\n> we do:\n> \n> FOO = $(shell do-the-thing)\n> \n> then we execute that command only when $(FOO) is needed[1].\n> \n> But \"include\" here is tricky. It is loaded regardless of whether the\n> values it contains are needed or not. I wonder if we could do better by\n> giving make more information about what we're expecting to get from it.\n> I.e., if we wrote:\n> \n> GIT_VERSION = $(shell awk '{print $3}' GIT-VERSION-FILE)\n> \n> Then \"make clean\", not needing the value of $(GIT_VERSION), wouldn't run\n> that shell snippet at all. Of course there's a catch; we are also\n> relying on the include to trigger the dependency. So it is really more\n> like:\n> \n> GIT_VERSION = $(shell make GIT-VERSION-FILE && awk '{print $3}' GIT-VERSION-FILE)\n> \n> I'm not sure how bad that is. Re-invoking make seems like it could get\n> expensive, especially for the common case that we're building actual\n> binaries and we _do_ need the version. But we could probably cut \"make\"\n> out of the loop entirely. Generating GIT-VERSION-FILE is already a FORCE\n> target, so really:\n> \n> GIT_VERSION = $(shell ./GIT-VERSION-GEN)\n> \n> would be equivalent-ish (with some output changes, and possibly we'd\n> want to stash the value in a file for any other scripts to make use of).\n> \n> This is all just stuff I've written in my editor and not tried. I won't\n> be surprised if there are some gotchas. But it at least seems like a\n> conceptually cleaner path.\n> \n> -Peff\n> \n> [1] Variable assignment actually has a slight problem in the opposite\n> direction: it wants to run the shell snippet every time the variable\n> is referenced. There's a trick to get around that described in\n> 0573831950 (Makefile: avoid running curl-config unnecessarily,\n> 2020-04-04).\n> \n> It's built around evals. In fact, I suspect you could build a\n> function around eval that actually works similar to include, but\n> lazy-loads the file only when one of its stubs is referenced. I.e.,\n> something like:\n> \n> GIT_VERSION = $(eval include GIT-VERSION-FILE)\n> \n> would probably work (and for other includes, multiple variables\n> could mention the same file; as soon as it gets included, it\n> overwrites the stubs).\n> \n\nHeh, in another reply in this thread, I mentioned that I had an alternate\npatch I was toying with. If I tell you it was inspired by your commit\n0573831950 (Makefile: avoid running curl-config unnecessarily, 04-04-2020),\nyou would probably not be surprised that it looks similar to what you\noutline here. I had only just started looking at this approach, but it has\nsome wrinkles, so I thought I would look at it after submitting this series\n'because this is an easy win'! ;-)\n\nI hadn't done so yet, but I had planned to modify the GIT-VERSION-GEN script\n(with -v parameter, say) to just output the version to stdout, because it\nwould save a sed invocation. It currently looks something like this:\n\n diff --git a/Makefile b/Makefile\n index 372139f1f2..f166fbe067 100644\n --- a/Makefile\n +++ b/Makefile\n @@ -493,7 +493,11 @@ all::\n \n GIT-VERSION-FILE: FORCE\n @$(SHELL_PATH) ./GIT-VERSION-GEN\n --include GIT-VERSION-FILE\n +\n +ifeq ($(wildcard GIT-VERSION-FILE),)\n +$(shell ./GIT-VERSION-GEN)\n +endif\n +GIT_VERSION = $(eval GIT_VERSION := $$(shell cat GIT-VERSION-FILE | sed -e 's/^GIT_VERSION = //'))$(GIT_VERSION)\n \n # Set our default configuration.\n #\n\nIgnore the 'ifeq' for now (I had several versions, including getting rid\nof the GIT-VERSION-FILE rule, but that caused problems without changing\nthe Documentation/Makefile, and several others ... (including in contrib)).\n\nSo, I haven't worked everything out yet, but I had planned to look at\nthis next.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"409298","messageId":"20201106205618.GA197338@coredump.intra.peff.net","threadId":"54581","inReplyTo":"a654c95f-2367-5805-d4f8-56c466a141d4@ramsayjones.plus.com","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-06T20:56:18Z","receivedAt":"2020-11-06T20:56:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 06, 2020 at 07:38:06PM +0000, Ramsay Jones wrote:\n\n> I hadn't done so yet, but I had planned to modify the GIT-VERSION-GEN script\n> (with -v parameter, say) to just output the version to stdout, because it\n> would save a sed invocation. It currently looks something like this:\n> \n> diff --git a/Makefile b/Makefile\n> index 372139f1f2..f166fbe067 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -493,7 +493,11 @@ all::\n> \n> GIT-VERSION-FILE: FORCE\n> @$(SHELL_PATH) ./GIT-VERSION-GEN\n> --include GIT-VERSION-FILE\n> +\n> +ifeq ($(wildcard GIT-VERSION-FILE),)\n> +$(shell ./GIT-VERSION-GEN)\n> +endif\n> +GIT_VERSION = $(eval GIT_VERSION := $$(shell cat GIT-VERSION-FILE | sed -e 's/^GIT_VERSION = //'))$(GIT_VERSION)\n\nYeah, not too surprising. If we grow a lot of these fill-in-the-value\nstubs, it might be worth abstracting it out into a function (using\n$(call) should be OK, as even Apple's ancient version of GNU make has\nit).\n\nI do think the one that evals an \"include\" might end up being more\nreadable and flexible, though. I'm not sure if the include needs to be\nmore aggressive about using \":=\" though (in a simple test it didn't seem\nto be, and since we'll be filling in a concrete value that's OK to\nevaluate multiple times, I think it would be OK).\n\n> Ignore the 'ifeq' for now (I had several versions, including getting rid\n> of the GIT-VERSION-FILE rule, but that caused problems without changing\n> the Documentation/Makefile, and several others ... (including in contrib)).\n\nYeah, I didn't look at all at what complications we might get from other\nMakefiles. Though if everything is literally running `GIT-VERSION-GEN`\ndirectly then I think they could all use the same code.\n\n> So, I haven't worked everything out yet, but I had planned to look at\n> this next.\n\nSounds good. I mostly wanted to plant the seed in your head. Finding\nthat it was already growing is better still. :)\n\n-Peff\n"},{"id":"409299","messageId":"xmqqsg9mduuy.fsf@gitster.c.googlers.com","threadId":"54581","inReplyTo":"a654c95f-2367-5805-d4f8-56c466a141d4@ramsayjones.plus.com","subject":"Re: [PATCH 2/8] Documentation/Makefile: conditionally include ../GIT-VERSION-FILE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-06T21:43:33Z","receivedAt":"2020-11-06T21:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n>> It's built around evals. In fact, I suspect you could build a\n>> function around eval that actually works similar to include, but\n>> lazy-loads the file only when one of its stubs is referenced. I.e.,\n>> something like:\n>> \n>> GIT_VERSION = $(eval include GIT-VERSION-FILE)\n>> \n>> would probably work (and for other includes, multiple variables\n>> could mention the same file; as soon as it gets included, it\n>> overwrites the stubs).\n>> \n>\n> Heh, in another reply in this thread, I mentioned that I had an alternate\n> patch I was toying with.\n\nYup, I was wondering if you're going to present that variant, which\nI was more curious about.\n\n"}]}