Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.