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.
