git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
Date: Thu, 06 May 2021 10:05:20 +0900	[thread overview]
Message-ID: <xmqq4kfg3c4v.fsf@gitster.g> (raw)
In-Reply-To: <YJKm0dnwHBwQuTi+@coredump.intra.peff.net> (Jeff King's message of "Wed, 5 May 2021 10:08:17 -0400")

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.

  reply	other threads:[~2021-05-06  1:05 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4kfg3c4v.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 1/4] Makefile: don'\''t re-define PERL_DEFINES' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).