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

Jeff King [off-list ref] writes:

I did have one question:

quoted

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.