| Message ID | patch-1.4-ed2005a2fbf-20210505T121857Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y | expand |
On Wed, May 05, 2021 at 02:21:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > Since 07d90eadb50 (Makefile: add Perl runtime prefix support, > 2018-04-10) we have been declaring PERL_DEFINES right after assigning > to it, with the effect that the first PERL_DEFINES was ignored. > > That bug didn't matter in practice since the first line had all the > same variables as the second, so we'd correctly re-generate > everything. It just made for confusing reading. > > Let's remove that first assignment, and while we're at it split these > across lines to make them more maintainable. This and the other three patches all look sensible to me. I did have one question: > diff --git a/Makefile b/Makefile > index 93664d67146..1d4c02e59d9 100644 > --- a/Makefile > +++ b/Makefile > @@ -2270,9 +2270,10 @@ perl_localedir_SQ = $(localedir_SQ) > > ifndef NO_PERL > PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl > -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) > - > -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) > +PERL_DEFINES := > +PERL_DEFINES += $(PERL_PATH_SQ) > +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) > +PERL_DEFINES += $(perllibdir_SQ) > PERL_DEFINES += $(RUNTIME_PREFIX) I don't think we generally use simply-expanded variables in our Makefile unless there's a reason. Do we actually need it here? Obviously not new in your patch, but just a curiosity I noticed while reading it. -Peff
Jeff King <peff@peff.net> writes: > I did have one question: > >> diff --git a/Makefile b/Makefile >> index 93664d67146..1d4c02e59d9 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -2270,9 +2270,10 @@ perl_localedir_SQ = $(localedir_SQ) >> >> ifndef NO_PERL >> PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl >> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) >> - >> -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) >> +PERL_DEFINES := >> +PERL_DEFINES += $(PERL_PATH_SQ) >> +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) >> +PERL_DEFINES += $(perllibdir_SQ) >> PERL_DEFINES += $(RUNTIME_PREFIX) > > I don't think we generally use simply-expanded variables in our Makefile > unless there's a reason. Do we actually need it here? Obviously not new > in your patch, but just a curiosity I noticed while reading it. Splitting the appending into multiple lines does make sense, and is in line with what 07d90ead (Makefile: add Perl runtime prefix support, 2018-04-10) introduced the "first create a space separated list and then redefine that same variable with spaces replaced with colons" strategy to reach the final value (i.e. colon separated tokens that lets us notice when build options changed) for. As to the simply-expanded vs recursively-expanded variable, there is aneed to use former, which comes from what the original commit 07d90ead did outside the context of this patch, which is: PERL_DEFINES := $(subst $(space),:,$(PERL_DEFINES)) GIT-PERL-DEFINES: FORCE @FLAGS='$(PERL_DEFINES)'; \ if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \ echo >&2 " * new perl-specific parameters"; \ echo "$$FLAGS" >$@; \ fi That is, up to this point PERL_DEFINES accumulate various build-time settings with += (i.e. space separated tokens), and at this point finally it is turned into a colon separated tokens, which cannot be written with a recursively expanded variable. But I tend to agree that you do not have to := clear the list in this patch.
On Wed, May 05 2021, Jeff King wrote: > On Wed, May 05, 2021 at 02:21:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> Since 07d90eadb50 (Makefile: add Perl runtime prefix support, >> 2018-04-10) we have been declaring PERL_DEFINES right after assigning >> to it, with the effect that the first PERL_DEFINES was ignored. >> >> That bug didn't matter in practice since the first line had all the >> same variables as the second, so we'd correctly re-generate >> everything. It just made for confusing reading. >> >> Let's remove that first assignment, and while we're at it split these >> across lines to make them more maintainable. > > This and the other three patches all look sensible to me. > > I did have one question: > >> diff --git a/Makefile b/Makefile >> index 93664d67146..1d4c02e59d9 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -2270,9 +2270,10 @@ perl_localedir_SQ = $(localedir_SQ) >> >> ifndef NO_PERL >> PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl >> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) >> - >> -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) >> +PERL_DEFINES := >> +PERL_DEFINES += $(PERL_PATH_SQ) >> +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) >> +PERL_DEFINES += $(perllibdir_SQ) >> PERL_DEFINES += $(RUNTIME_PREFIX) > > I don't think we generally use simply-expanded variables in our Makefile > unless there's a reason. Do we actually need it here? Obviously not new > in your patch, but just a curiosity I noticed while reading it. I didn't notice it at the time. I suppose it could be changed to not do expansion, but per-se unrelated to the more narrorw bugfix in this patch.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >>> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) >>> - >>> -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) >>> +PERL_DEFINES := >>> +PERL_DEFINES += $(PERL_PATH_SQ) >>> +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) >>> +PERL_DEFINES += $(perllibdir_SQ) >>> PERL_DEFINES += $(RUNTIME_PREFIX) >> >> I don't think we generally use simply-expanded variables in our Makefile >> unless there's a reason. Do we actually need it here? Obviously not new >> in your patch, but just a curiosity I noticed while reading it. > > I didn't notice it at the time. I suppose it could be changed to not do > expansion, but per-se unrelated to the more narrorw bugfix in this > patch. Actually, strictly speaking there was *no* bug because assigning three items with := made sure the previous recursively expanded one to be ineffective. In other words, there was a valid reason to use ":=" there in the original version. Now your patch removed the recursively expanded one that was immediately invalidated, there no longer is a reason to use := there. So "unrelated to the more narrow bugfix" is a rather lame excuse to do only half a task. If we remove that extra one (which is a good thing), then we should correct := into = because the original used := only because there was the unwanted extra one, no?
On Thu, May 06 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>>> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) >>>> - >>>> -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) >>>> +PERL_DEFINES := >>>> +PERL_DEFINES += $(PERL_PATH_SQ) >>>> +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) >>>> +PERL_DEFINES += $(perllibdir_SQ) >>>> PERL_DEFINES += $(RUNTIME_PREFIX) >>> >>> I don't think we generally use simply-expanded variables in our Makefile >>> unless there's a reason. Do we actually need it here? Obviously not new >>> in your patch, but just a curiosity I noticed while reading it. >> >> I didn't notice it at the time. I suppose it could be changed to not do >> expansion, but per-se unrelated to the more narrorw bugfix in this >> patch. > > Actually, strictly speaking there was *no* bug because assigning > three items with := made sure the previous recursively expanded one > to be ineffective. In other words, there was a valid reason to use > ":=" there in the original version. Yes, there wasn't any bug with the the eventual value being incorrect. I.e. both of these are equivalent in a Makefile: FOO = abc FOO := def FOO += ghi And: FOO = abc FOO = def FOO += ghi Both will yield "def ghi". They're just different in a case like: X = Y FOO = abc FOO := $(X) X = Z FOO += ghi Where using := will echo "Y ghi", and using = will echo "Z ghi". As a practical matter the distinction doesn't matter in this case. > Now your patch removed the recursively expanded one that was > immediately invalidated, there no longer is a reason to use := > there. So "unrelated to the more narrow bugfix" is a rather lame > excuse to do only half a task. If we remove that extra one (which > is a good thing), then we should correct := into = because the > original used := only because there was the unwanted extra one, no? I don't see how removing the stray line changes the reason to use ":=" or "=" there. I agree it should be removed, it's just unrelated to removing the stay line. Looking at 07d90eadb50 it's clear that it's just some copy/pasting error. Maybe the confusion is that I'm using "bug" closer to a meaning of "a thing nobody intended to be in the program", not just "a behavior-changing issue observable from the outside". In any case. I can just submit a patch on top of this in a v2. I continue to find it hard to discover the line between superfluous while-we're-at-it fixes in your mind v.s. "we should fix this while we're at it" though :) But regarding the "half a task" it seems to me that these are different issues; I don't think that's a point worth arguing in this case specifically (let's just fix it, and I will), but perhaps I'm missing something subtle with regards to Makefile semantics per my examples above so it really is all one issue, and I'd like to understand how they're entwined.
On Thu, May 06, 2021 at 10:05:20AM +0900, Junio C Hamano wrote: > As to the simply-expanded vs recursively-expanded variable, there is > aneed to use former, which comes from what the original commit > 07d90ead did outside the context of this patch, which is: > > PERL_DEFINES := $(subst $(space),:,$(PERL_DEFINES)) > GIT-PERL-DEFINES: FORCE > @FLAGS='$(PERL_DEFINES)'; \ > if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \ > echo >&2 " * new perl-specific parameters"; \ > echo "$$FLAGS" >$@; \ > fi > > That is, up to this point PERL_DEFINES accumulate various build-time > settings with += (i.e. space separated tokens), and at this point > finally it is turned into a colon separated tokens, which cannot be > written with a recursively expanded variable. > > But I tend to agree that you do not have to := clear the list in > this patch. OK, that matches my understanding. Thanks for laying out. In general, I would say that the later use that you quoted above would do better to use a second variable (because then there is no question of when PERL_DEFINES is space-separated and when it is colon-separated). But that is not that big a deal, and certainly very orthogonal to Ævar's patch. I'd also question whether the colon transformation is even necessary. The point of the file is to change when the values change. Being sloppy with delimiters means we _could_ miss a change, but in practice I doubt it is very likely (and it is not like colons cannot appear in values, either, so they are not foolproof). But again, not really important for this patch. -Peff P.S. I also wondered briefly if make would preserve spaces even for empty variables (since without that, we might miss delimiters and confuse one of the variables for another). I.e., we know that: FOO = $(one):$(two):$(three) will have two colons even if some of the variables are empty. But does it preserve them even for "$(one) $(two) $(three)", or more importantly, when using "+=" (which _would_ be relevant to this patch)? The answer is yes, they are all fine. You can demonstrate it with the Makefile below, running "make one=foo three=bar", "make two=foo", etc. -- >8 -- empty := space := $(empty) $(empty) COLONS = $(one):$(two):$(three) SPACES_SINGLE = $(one) $(two) $(three) SPACES_SINGLE := $(subst $(space),:,$(SPACES_SINGLE)) SPACES_PLUS = SPACES_PLUS += $(one) SPACES_PLUS += $(two) SPACES_PLUS += $(three) SPACES_PLUS := $(subst $(space),:,$(SPACES_PLUS)) all: @echo "COLONS=$(COLONS)" @echo "SPACES_SINGLE=$(SPACES_SINGLE)" @echo "SPACES_PLUS=$(SPACES_PLUS)"
On Thu, May 06, 2021 at 11:04:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Actually, strictly speaking there was *no* bug because assigning > > three items with := made sure the previous recursively expanded one > > to be ineffective. In other words, there was a valid reason to use > > ":=" there in the original version. > > Yes, there wasn't any bug with the the eventual value being > incorrect. I.e. both of these are equivalent in a Makefile: > > FOO = abc > FOO := def > FOO += ghi > > And: > > FOO = abc > FOO = def > FOO += ghi > > Both will yield "def ghi". They're just different in a case like: > > X = Y > FOO = abc > FOO := $(X) > X = Z > FOO += ghi > > Where using := will echo "Y ghi", and using = will echo "Z ghi". As a > practical matter the distinction doesn't matter in this case. Yeah, I don't think the ":=" was impacting the bug or no bug (not to mention that even if we duplicated those entries in the variable, it _still_ wouldn't be a bug, since the whole point of the variable is just to notice when the content changes). > > Now your patch removed the recursively expanded one that was > > immediately invalidated, there no longer is a reason to use := > > there. So "unrelated to the more narrow bugfix" is a rather lame > > excuse to do only half a task. If we remove that extra one (which > > is a good thing), then we should correct := into = because the > > original used := only because there was the unwanted extra one, no? > > I don't see how removing the stray line changes the reason to use ":=" > or "=" there. I agree it should be removed, it's just unrelated to > removing the stay line. Looking at 07d90eadb50 it's clear that it's just > some copy/pasting error. Yeah, I'd agree it is truly orthogonal. I don't mind seeing it cleaned up in addition (or am even actively happy to see it cleaned up :) ), but IMHO it would not need to hold up the series. -Peff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Maybe the confusion is that I'm using "bug" closer to a meaning of "a > thing nobody intended to be in the program", not just "a > behavior-changing issue observable from the outside". OK, thanks for explaining where my confusion came from.
diff --git a/Makefile b/Makefile index 93664d67146..1d4c02e59d9 100644 --- a/Makefile +++ b/Makefile @@ -2270,9 +2270,10 @@ perl_localedir_SQ = $(localedir_SQ) ifndef NO_PERL PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ) - -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ) +PERL_DEFINES := +PERL_DEFINES += $(PERL_PATH_SQ) +PERL_DEFINES += $(PERLLIB_EXTRA_SQ) +PERL_DEFINES += $(perllibdir_SQ) PERL_DEFINES += $(RUNTIME_PREFIX) # Support Perl runtime prefix. In this mode, a different header is installed
Since 07d90eadb50 (Makefile: add Perl runtime prefix support, 2018-04-10) we have been declaring PERL_DEFINES right after assigning to it, with the effect that the first PERL_DEFINES was ignored. That bug didn't matter in practice since the first line had all the same variables as the second, so we'd correctly re-generate everything. It just made for confusing reading. Let's remove that first assignment, and while we're at it split these across lines to make them more maintainable. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Makefile | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)