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

[Jeff King <peff@peff.net>]

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