On Tue, Jan 02 2018, Jonathan Nieder jotted:
> Subject: perl: treat PERLLIB_EXTRA as an extra path again
>
> PERLLIB_EXTRA was introduced in v1.9-rc0~88^2 (2013-11-15) as a way
> for packagers to add additional directories such as the location of
> Subversion's perl bindings to Git's perl path. Since 20d2a30f
> (Makefile: replace perl/Makefile.PL with simple make rules,
> 2012-12-10) setting that variable breaks perl-based commands instead:
>
> $ PATH=$HOME/opt/git/bin:$PATH
> $ make install prefix=$HOME/opt/git PERLLIB_EXTRA=anextralibdir
> [...]
> $ head -2 $HOME/opt/git/libexec/git-core/git-add--interactive
> #!/usr/bin/perl
> use lib (split(/:/, $ENV{GITPERLLIB} || ":helloiamanextrainstlibdir" ||
> "/usr/local/google/home/jrn/opt/git/share/perl5"));
> $ git add -p
> Empty compile time value given to use lib at
> /home/jrn/opt/git/libexec/git-core/git-add--interactive line 2.
>
> Removing the spurious ":" at the beginning of ":$PERLLIB_EXTRA" avoids
> the "Empty compile time value" error but with that tweak the problem
> still remains: PERLLIB_EXTRA ends up replacing instead of
> supplementing the perllibdir that would be passed to 'use lib' if
> PERLLIB_EXTRA were not set.
>
> The intent was to simplify, as the commit message to 20d2a30f
> explains:
>
> | The scripts themselves will 'use lib' the target directory, but if
> | INSTLIBDIR is set it overrides it. It doesn't have to be this way,
> | it could be set in addition to INSTLIBDIR, but my reading of
> | [v1.9-rc0~88^2] is that this is the desired behavior.
>
> Restore the previous code structure to make PERLLIB_EXTRA work again.
>
> Reproducing this problem requires an invocation of "make install"
> instead of running bin-wrappers/git in place, since the latter sets
> the GITPERLLIB environment variable, avoiding trouble.
>
> Reported-by: Jonathan Koren <jdko...@google.com>
> Signed-off-by: Jonathan Nieder <jrnie...@gmail.com>
> ---
> Jonathan Nieder wrote:
>> Hi,
>>
>> Ævar Arnfjörð Bjarmason wrote:
>>
>> > +++ b/Makefile
>> [...]
>> > -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
>> > -$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES
>> > GIT-VERSION-FILE
>> > +PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
>> > +$(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
>> > $(QUIET_GEN)$(RM) $@ $@+ && \
>> > - INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory
>> > instlibdir` && \
>> > INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>> > INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
>> > sed -e '1{' \
>> > -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>> > -e ' h' \
>> > - -e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} ||
>> > "'"$$INSTLIBDIR"'"));=' \
>> > + -e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} ||
>> > "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
>>
>> This appears to have broken a build with INSTLIBDIR set.
>
> Here it is in patch form.
>
> Makefile | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 5c73cd208a..409e8f6ec9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1951,12 +1951,13 @@ $(SCRIPT_PERL_GEN):
> PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
> $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-VERSION-FILE
> $(QUIET_GEN)$(RM) $@ $@+ && \
> + INSTLIBDIR='$(perllibdir_SQ)' && \
> INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
> INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
> sed -e '1{' \
> -e ' s|#!.*perl|#!$(PERL_PATH_SQ)|' \
> -e ' h' \
> - -e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} ||
> "'"$$INSTLIBDIR"'" || "'"$(perllibdir_SQ)"'"));=' \
> + -e ' s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} ||
> "'"$$INSTLIBDIR"'"));=' \
> -e ' H' \
> -e ' x' \
> -e '}' \
This obviously makes perfect sense if the intent is to add this lib dir
instead of it being a replacement (as is clear from this being an issue
you're noting).
With the benefit of hindsight in re-reading the commit + this report I
can see that it *should* be that way, but I assumed it was the other way
around when I wrote this up.
Thanks!
