git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y
@ 2021-05-05 12:21 Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
                   ` (4 more replies)
  0 siblings, 5 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-05 12:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

These patches are relatively trivial fixes for bugs noticed when I was
working on recent send-email patches. We don't re-build perl/build/*
assets when variables that affect them are changed, and needlessly use
our non-mock gettext Perl i18n framework under NO_GETTEXT=Y if the
system happens to have Locale::Messages installed.

Ævar Arnfjörð Bjarmason (4):
  Makefile: don't re-define PERL_DEFINES
  Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes
  Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change
  perl: use mock i18n functions under NO_GETTEXT=Y

 Makefile         | 13 +++++++++----
 perl/Git/I18N.pm | 10 ++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.31.1.838.g7ac6e98bb53


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
@ 2021-05-05 12:21 ` Ævar Arnfjörð Bjarmason
  2021-05-05 14:08   ` Jeff King
  2021-05-05 12:21 ` [PATCH 2/4] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-05 12:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

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(-)

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
-- 
2.31.1.838.g7ac6e98bb53


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/4] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes
  2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
@ 2021-05-05 12:21 ` Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 3/4] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-05 12:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

Change the logic to generate perl/build/* to regenerate those files if
GIT-PERL-DEFINES changes. This ensures that e.g. changing localedir
will result in correctly re-generated files.

I don't think that ever worked. The brokenness pre-dates my
20d2a30f8ff (Makefile: replace perl/Makefile.PL with simple make
rules, 2017-12-10).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 1d4c02e59d9..a15f39e40ff 100644
--- a/Makefile
+++ b/Makefile
@@ -2676,7 +2676,7 @@ endif
 NO_PERL_CPAN_FALLBACKS_SQ = $(subst ','\'',$(NO_PERL_CPAN_FALLBACKS))
 endif
 
-perl/build/lib/%.pm: perl/%.pm
+perl/build/lib/%.pm: perl/%.pm GIT-PERL-DEFINES
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
 	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
-- 
2.31.1.838.g7ac6e98bb53


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/4] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change
  2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 2/4] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
@ 2021-05-05 12:21 ` Ævar Arnfjörð Bjarmason
  2021-05-05 12:21 ` [PATCH 4/4] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
  4 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-05 12:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

Regenerate the *.pm files in perl/build/* if the
NO_PERL_CPAN_FALLBACKS flag added to the *.pm files in
1aca69c0195 (perl Git::LoadCPAN: emit better errors under
NO_PERL_CPAN_FALLBACKS, 2018-03-03) is changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index a15f39e40ff..574b25512e7 100644
--- a/Makefile
+++ b/Makefile
@@ -2275,6 +2275,7 @@ PERL_DEFINES += $(PERL_PATH_SQ)
 PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
 PERL_DEFINES += $(perllibdir_SQ)
 PERL_DEFINES += $(RUNTIME_PREFIX)
+PERL_DEFINES += $(NO_PERL_CPAN_FALLBACKS)
 
 # Support Perl runtime prefix. In this mode, a different header is installed
 # into Perl scripts.
-- 
2.31.1.838.g7ac6e98bb53


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/4] perl: use mock i18n functions under NO_GETTEXT=Y
  2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2021-05-05 12:21 ` [PATCH 3/4] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
@ 2021-05-05 12:21 ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
  4 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-05 12:21 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason

Change the logic of the i18n functions I added in 5e9637c6297 (i18n:
add infrastructure for translating Git with gettext, 2011-11-18) to
use pass-through functions when NO_GETTEXT is defined.

This speeds up the compilation time of commands that use this library
when NO_GETTEXT=Y is in effect. Loading it and POSIX.pm is around 20ms
on my machine, whereas it takes 2ms to just instantiate perl itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile         |  3 +++
 perl/Git/I18N.pm | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/Makefile b/Makefile
index 574b25512e7..b8d6b313056 100644
--- a/Makefile
+++ b/Makefile
@@ -1986,6 +1986,7 @@ ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
 ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
+NO_GETTEXT_SQ = $(subst ','\'',$(NO_GETTEXT))
 bindir_SQ = $(subst ','\'',$(bindir))
 bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_SQ = $(subst ','\'',$(mandir))
@@ -2276,6 +2277,7 @@ PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
 PERL_DEFINES += $(perllibdir_SQ)
 PERL_DEFINES += $(RUNTIME_PREFIX)
 PERL_DEFINES += $(NO_PERL_CPAN_FALLBACKS)
+PERL_DEFINES += $(NO_GETTEXT)
 
 # Support Perl runtime prefix. In this mode, a different header is installed
 # into Perl scripts.
@@ -2680,6 +2682,7 @@ endif
 perl/build/lib/%.pm: perl/%.pm GIT-PERL-DEFINES
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
 	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
+	    -e 's|@@NO_GETTEXT@@|$(NO_GETTEXT_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
 	< $< > $@
 
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index 2037f387c89..895e759c57a 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -16,9 +16,19 @@ BEGIN
 our @EXPORT = qw(__ __n N__);
 our @EXPORT_OK = @EXPORT;
 
+# See Git::LoadCPAN's NO_PERL_CPAN_FALLBACKS_STR for a description of
+# this "'@@' [...] '@@'" pattern.
+use constant NO_GETTEXT_STR => '@@' . 'NO_GETTEXT' . '@@';
+use constant NO_GETTEXT => (
+	q[@@NO_GETTEXT@@] ne ''
+	and
+	q[@@NO_GETTEXT@@] ne NO_GETTEXT_STR
+);
+
 sub __bootstrap_locale_messages {
 	our $TEXTDOMAIN = 'git';
 	our $TEXTDOMAINDIR ||= $ENV{GIT_TEXTDOMAINDIR} || '@@LOCALEDIR@@';
+	die "NO_GETTEXT=" . NO_GETTEXT_STR if NO_GETTEXT;
 
 	require POSIX;
 	POSIX->import(qw(setlocale));
-- 
2.31.1.838.g7ac6e98bb53


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
@ 2021-05-05 14:08   ` Jeff King
  2021-05-06  1:05     ` Junio C Hamano
  2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 26+ messages in thread
From: Jeff King @ 2021-05-05 14:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-05 14:08   ` Jeff King
@ 2021-05-06  1:05     ` Junio C Hamano
  2021-05-06 16:55       ` Jeff King
  2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2021-05-06  1:05 UTC (permalink / raw)
  To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-05 14:08   ` Jeff King
  2021-05-06  1:05     ` Junio C Hamano
@ 2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
  2021-05-06  8:42       ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-06  6:23 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano


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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
@ 2021-05-06  8:42       ` Junio C Hamano
  2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2021-05-06  8:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git

Æ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?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-06  8:42       ` Junio C Hamano
@ 2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason
  2021-05-06 16:59           ` Jeff King
  2021-05-07  8:42           ` Junio C Hamano
  0 siblings, 2 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-06  9:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git


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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-06  1:05     ` Junio C Hamano
@ 2021-05-06 16:55       ` Jeff King
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2021-05-06 16:55 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Ævar Arnfjörð Bjarmason, git

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)"

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason
@ 2021-05-06 16:59           ` Jeff King
  2021-05-07  8:42           ` Junio C Hamano
  1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2021-05-06 16:59 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
  2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason
  2021-05-06 16:59           ` Jeff King
@ 2021-05-07  8:42           ` Junio C Hamano
  1 sibling, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2021-05-07  8:42 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Jeff King, git

Æ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.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y
  2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2021-05-05 12:21 ` [PATCH 4/4] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50 ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
                     ` (6 more replies)
  4 siblings, 7 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

The summary, from v1:

    These patches are relatively trivial fixes for bugs noticed when I was
    working on recent send-email patches. We don't re-build perl/build/*
    assets when variables that affect them are changed, and needlessly use
    our non-mock gettext Perl i18n framework under NO_GETTEXT=Y if the
    system happens to have Locale::Messages installed.

Changes since v1:

The only change to the end-state is the trivial change-on-top of:

    -PERL_DEFINES :=
    +PERL_DEFINES =

I.e. the PERL_DEFINES is now also refactored away from a
simply-expanded variable. The re-arrangement and split-up of patches
in this v2 just makes for a more incremental way to get there, per the
discussion on v1.

Ævar Arnfjörð Bjarmason (6):
  Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes
  Makefile: don't re-define PERL_DEFINES
  Makefile: make PERL_DEFINES recursively expanded
  Makefile: split up the deceleration of PERL_DEFINES
  Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change
  perl: use mock i18n functions under NO_GETTEXT=Y

 Makefile         | 13 +++++++++----
 perl/Git/I18N.pm | 10 ++++++++++
 2 files changed, 19 insertions(+), 4 deletions(-)

Range-diff against v1:
2:  49339028db4 = 1:  8b899364916 Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes
-:  ----------- > 2:  c15887bbc93 Makefile: don't re-define PERL_DEFINES
-:  ----------- > 3:  7919ae0e546 Makefile: make PERL_DEFINES recursively expanded
1:  ed2005a2fbf ! 4:  2cdefbe920c Makefile: don't re-define PERL_DEFINES
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    Makefile: don't re-define PERL_DEFINES
    +    Makefile: split up the deceleration of PERL_DEFINES
     
    -    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.
    +    Split the declaration of PERL_DEFINES across multiple line, making
    +    this easier to read.
     
    -    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.
    +    In 07d90eadb50 (Makefile: add Perl runtime prefix support, 2018-04-10)
    +    when PERL_DEFINES was added only the RUNTIME_PREFIX was put on its own
    +    line the rest weren't formatted like that for consistency. Let's do
    +    that to make this consistent with most of the rest of this Makefile.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ Makefile: 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) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ)
    ++PERL_DEFINES =
     +PERL_DEFINES += $(PERL_PATH_SQ)
     +PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
     +PERL_DEFINES += $(perllibdir_SQ)
3:  06e25bc1db3 = 5:  1171b73256e Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change
4:  97247cb72a5 = 6:  7a5e7cf39a5 perl: use mock i18n functions under NO_GETTEXT=Y
-- 
2.31.1.838.g924d365b763


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 2/6] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
                     ` (5 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Change the logic to generate perl/build/* to regenerate those files if
GIT-PERL-DEFINES changes. This ensures that e.g. changing localedir
will result in correctly re-generated files.

I don't think that ever worked. The brokenness pre-dates my
20d2a30f8ff (Makefile: replace perl/Makefile.PL with simple make
rules, 2017-12-10).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 93664d67146..ad618cea33f 100644
--- a/Makefile
+++ b/Makefile
@@ -2675,7 +2675,7 @@ endif
 NO_PERL_CPAN_FALLBACKS_SQ = $(subst ','\'',$(NO_PERL_CPAN_FALLBACKS))
 endif
 
-perl/build/lib/%.pm: perl/%.pm
+perl/build/lib/%.pm: perl/%.pm GIT-PERL-DEFINES
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
 	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2 2/6] Makefile: don't re-define PERL_DEFINES
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 3/6] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

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.

This 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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Makefile b/Makefile
index ad618cea33f..ea387b431e1 100644
--- a/Makefile
+++ b/Makefile
@@ -2270,8 +2270,6 @@ 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 += $(RUNTIME_PREFIX)
 
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2 3/6] Makefile: make PERL_DEFINES recursively expanded
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 2/6] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 4/6] Makefile: split up the deceleration of PERL_DEFINES Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Since 07d90eadb50 (Makefile: add Perl runtime prefix support,
2018-04-10) PERL_DEFINES has been a simply-expanded variable, let's
make it recursively expanded instead.

This change doesn't matter for the correctness of the logic. Whether
we used simply-expanded or recursively expanded didn't change what we
wrote out in GIT-PERL-DEFINES, but being consistent with other rules
makes this easier to understand.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index ea387b431e1..3ed6828de67 100644
--- a/Makefile
+++ b/Makefile
@@ -2270,7 +2270,7 @@ 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 += $(RUNTIME_PREFIX)
 
 # Support Perl runtime prefix. In this mode, a different header is installed
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2 4/6] Makefile: split up the deceleration of PERL_DEFINES
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-05-10 10:50   ` [PATCH v2 3/6] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 5/6] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Split the declaration of PERL_DEFINES across multiple line, making
this easier to read.

In 07d90eadb50 (Makefile: add Perl runtime prefix support, 2018-04-10)
when PERL_DEFINES was added only the RUNTIME_PREFIX was put on its own
line the rest weren't formatted like that for consistency. Let's do
that to make this consistent with most of the rest of this Makefile.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 3ed6828de67..4f68f5e1dba 100644
--- a/Makefile
+++ b/Makefile
@@ -2270,7 +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_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
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2 5/6] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2021-05-10 10:50   ` [PATCH v2 4/6] Makefile: split up the deceleration of PERL_DEFINES Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 10:50   ` [PATCH v2 6/6] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
  2021-05-10 18:17   ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Junio C Hamano
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Regenerate the *.pm files in perl/build/* if the
NO_PERL_CPAN_FALLBACKS flag added to the *.pm files in
1aca69c0195 (perl Git::LoadCPAN: emit better errors under
NO_PERL_CPAN_FALLBACKS, 2018-03-03) is changed.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 4f68f5e1dba..aaa972c56aa 100644
--- a/Makefile
+++ b/Makefile
@@ -2275,6 +2275,7 @@ PERL_DEFINES += $(PERL_PATH_SQ)
 PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
 PERL_DEFINES += $(perllibdir_SQ)
 PERL_DEFINES += $(RUNTIME_PREFIX)
+PERL_DEFINES += $(NO_PERL_CPAN_FALLBACKS)
 
 # Support Perl runtime prefix. In this mode, a different header is installed
 # into Perl scripts.
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v2 6/6] perl: use mock i18n functions under NO_GETTEXT=Y
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  2021-05-10 10:50   ` [PATCH v2 5/6] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
@ 2021-05-10 10:50   ` Ævar Arnfjörð Bjarmason
  2021-05-10 18:17   ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Junio C Hamano
  6 siblings, 0 replies; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-10 10:50 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Change the logic of the i18n functions I added in 5e9637c6297 (i18n:
add infrastructure for translating Git with gettext, 2011-11-18) to
use pass-through functions when NO_GETTEXT is defined.

This speeds up the compilation time of commands that use this library
when NO_GETTEXT=Y is in effect. Loading it and POSIX.pm is around 20ms
on my machine, whereas it takes 2ms to just instantiate perl itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile         |  3 +++
 perl/Git/I18N.pm | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/Makefile b/Makefile
index aaa972c56aa..0705fc2d3fb 100644
--- a/Makefile
+++ b/Makefile
@@ -1986,6 +1986,7 @@ ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
 ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
 DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
+NO_GETTEXT_SQ = $(subst ','\'',$(NO_GETTEXT))
 bindir_SQ = $(subst ','\'',$(bindir))
 bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
 mandir_SQ = $(subst ','\'',$(mandir))
@@ -2276,6 +2277,7 @@ PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
 PERL_DEFINES += $(perllibdir_SQ)
 PERL_DEFINES += $(RUNTIME_PREFIX)
 PERL_DEFINES += $(NO_PERL_CPAN_FALLBACKS)
+PERL_DEFINES += $(NO_GETTEXT)
 
 # Support Perl runtime prefix. In this mode, a different header is installed
 # into Perl scripts.
@@ -2680,6 +2682,7 @@ endif
 perl/build/lib/%.pm: perl/%.pm GIT-PERL-DEFINES
 	$(QUIET_GEN)mkdir -p $(dir $@) && \
 	sed -e 's|@@LOCALEDIR@@|$(perl_localedir_SQ)|g' \
+	    -e 's|@@NO_GETTEXT@@|$(NO_GETTEXT_SQ)|g' \
 	    -e 's|@@NO_PERL_CPAN_FALLBACKS@@|$(NO_PERL_CPAN_FALLBACKS_SQ)|g' \
 	< $< > $@
 
diff --git a/perl/Git/I18N.pm b/perl/Git/I18N.pm
index 2037f387c89..895e759c57a 100644
--- a/perl/Git/I18N.pm
+++ b/perl/Git/I18N.pm
@@ -16,9 +16,19 @@ BEGIN
 our @EXPORT = qw(__ __n N__);
 our @EXPORT_OK = @EXPORT;
 
+# See Git::LoadCPAN's NO_PERL_CPAN_FALLBACKS_STR for a description of
+# this "'@@' [...] '@@'" pattern.
+use constant NO_GETTEXT_STR => '@@' . 'NO_GETTEXT' . '@@';
+use constant NO_GETTEXT => (
+	q[@@NO_GETTEXT@@] ne ''
+	and
+	q[@@NO_GETTEXT@@] ne NO_GETTEXT_STR
+);
+
 sub __bootstrap_locale_messages {
 	our $TEXTDOMAIN = 'git';
 	our $TEXTDOMAINDIR ||= $ENV{GIT_TEXTDOMAINDIR} || '@@LOCALEDIR@@';
+	die "NO_GETTEXT=" . NO_GETTEXT_STR if NO_GETTEXT;
 
 	require POSIX;
 	POSIX->import(qw(setlocale));
-- 
2.31.1.838.g924d365b763


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y
  2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
                     ` (5 preceding siblings ...)
  2021-05-10 10:50   ` [PATCH v2 6/6] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
@ 2021-05-10 18:17   ` Junio C Hamano
  2021-05-11  6:56     ` Ævar Arnfjörð Bjarmason
  6 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2021-05-10 18:17 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The summary, from v1:
>
>     These patches are relatively trivial fixes for bugs noticed when I was
>     working on recent send-email patches. We don't re-build perl/build/*
>     assets when variables that affect them are changed, and needlessly use
>     our non-mock gettext Perl i18n framework under NO_GETTEXT=Y if the
>     system happens to have Locale::Messages installed.
>
> Changes since v1:

Hmph, didn't reviewers declare that the previous round good enough?
I thought I merged it 'next' already.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y
  2021-05-10 18:17   ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Junio C Hamano
@ 2021-05-11  6:56     ` Ævar Arnfjörð Bjarmason
  2021-05-11  7:05       ` Junio C Hamano
  0 siblings, 1 reply; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-11  6:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King


On Tue, May 11 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> The summary, from v1:
>>
>>     These patches are relatively trivial fixes for bugs noticed when I was
>>     working on recent send-email patches. We don't re-build perl/build/*
>>     assets when variables that affect them are changed, and needlessly use
>>     our non-mock gettext Perl i18n framework under NO_GETTEXT=Y if the
>>     system happens to have Locale::Messages installed.
>>
>> Changes since v1:
>
> Hmph, didn't reviewers declare that the previous round good enough?
> I thought I merged it 'next' already.

I sent this re-roll before I noticed that. Nevermind & sorry for the
noise.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y
  2021-05-11  6:56     ` Ævar Arnfjörð Bjarmason
@ 2021-05-11  7:05       ` Junio C Hamano
  2021-05-12  9:49         ` [PATCH] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2021-05-11  7:05 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, May 11 2021, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> The summary, from v1:
>>>
>>>     These patches are relatively trivial fixes for bugs noticed when I was
>>>     working on recent send-email patches. We don't re-build perl/build/*
>>>     assets when variables that affect them are changed, and needlessly use
>>>     our non-mock gettext Perl i18n framework under NO_GETTEXT=Y if the
>>>     system happens to have Locale::Messages installed.
>>>
>>> Changes since v1:
>>
>> Hmph, didn't reviewers declare that the previous round good enough?
>> I thought I merged it 'next' already.
>
> I sent this re-roll before I noticed that. Nevermind & sorry for the
> noise.

Mails cross all the time.  A single-liner follow-up incremental
would be good, perhaps?

Thanks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH] Makefile: make PERL_DEFINES recursively expanded
  2021-05-11  7:05       ` Junio C Hamano
@ 2021-05-12  9:49         ` Ævar Arnfjörð Bjarmason
  2021-05-12 19:53           ` Jeff King
  0 siblings, 1 reply; 26+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-05-12  9:49 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason

Since 07d90eadb50 (Makefile: add Perl runtime prefix support,
2018-04-10) PERL_DEFINES has been a simply-expanded variable, let's
make it recursively expanded instead.

This change doesn't matter for the correctness of the logic. Whether
we used simply-expanded or recursively expanded didn't change what we
wrote out in GIT-PERL-DEFINES, but being consistent with other rules
makes this easier to understand.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

This is a trivial improvement-on-top for my ab/perl-makefile-cleanup
already in "next". I'd sent a v2 of that in [1] not seeing it was
merged down already, this incremental patch replaces that and results
in the same end-state.

1. https://lore.kernel.org/git/cover-0.6-00000000000-20210510T104937Z-avarab@gmail.com/

 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b8d6b31305..0705fc2d3f 100644
--- a/Makefile
+++ b/Makefile
@@ -2271,7 +2271,7 @@ perl_localedir_SQ = $(localedir_SQ)
 
 ifndef NO_PERL
 PERL_HEADER_TEMPLATE = perl/header_templates/fixed_prefix.template.pl
-PERL_DEFINES :=
+PERL_DEFINES =
 PERL_DEFINES += $(PERL_PATH_SQ)
 PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
 PERL_DEFINES += $(perllibdir_SQ)
-- 
2.31.1.909.g789bb6d90e


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH] Makefile: make PERL_DEFINES recursively expanded
  2021-05-12  9:49         ` [PATCH] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
@ 2021-05-12 19:53           ` Jeff King
  2021-05-12 22:45             ` Junio C Hamano
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2021-05-12 19:53 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: git, Junio C Hamano

On Wed, May 12, 2021 at 11:49:44AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Since 07d90eadb50 (Makefile: add Perl runtime prefix support,
> 2018-04-10) PERL_DEFINES has been a simply-expanded variable, let's
> make it recursively expanded instead.
> 
> This change doesn't matter for the correctness of the logic. Whether
> we used simply-expanded or recursively expanded didn't change what we
> wrote out in GIT-PERL-DEFINES, but being consistent with other rules
> makes this easier to understand.

Thanks for following up on this. I think it's worth doing.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH] Makefile: make PERL_DEFINES recursively expanded
  2021-05-12 19:53           ` Jeff King
@ 2021-05-12 22:45             ` Junio C Hamano
  0 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2021-05-12 22:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git

Jeff King <peff@peff.net> writes:

> On Wed, May 12, 2021 at 11:49:44AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Since 07d90eadb50 (Makefile: add Perl runtime prefix support,
>> 2018-04-10) PERL_DEFINES has been a simply-expanded variable, let's
>> make it recursively expanded instead.
>> 
>> This change doesn't matter for the correctness of the logic. Whether
>> we used simply-expanded or recursively expanded didn't change what we
>> wrote out in GIT-PERL-DEFINES, but being consistent with other rules
>> makes this easier to understand.
>
> Thanks for following up on this. I think it's worth doing.
>
> -Peff

Yup, thanks, both.

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2021-05-12 23:28 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-05 14:08   ` Jeff King
2021-05-06  1:05     ` Junio C Hamano
2021-05-06 16:55       ` Jeff King
2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
2021-05-06  8:42       ` Junio C Hamano
2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason
2021-05-06 16:59           ` Jeff King
2021-05-07  8:42           ` Junio C Hamano
2021-05-05 12:21 ` [PATCH 2/4] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 3/4] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 4/4] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 2/6] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 3/6] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 4/6] Makefile: split up the deceleration of PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 5/6] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 6/6] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-10 18:17   ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Junio C Hamano
2021-05-11  6:56     ` Ævar Arnfjörð Bjarmason
2021-05-11  7:05       ` Junio C Hamano
2021-05-12  9:49         ` [PATCH] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
2021-05-12 19:53           ` Jeff King
2021-05-12 22:45             ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).