Re: [PATCH 1/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 20 2017, Ævar Arnfjörð Bjarmason jotted:

> On Sun, Nov 19 2017, Dan Jacques jotted:
>
>> [...]
>
> Firstly the promise of this is very neat. I'm happy to offer any help I
> can give.
>
>> Enable Git to resolve its own binary location using a variety of
>> OS-specific and generic methods, including:
>>
>> - procfs via "/proc/self/exe" (Linux)
>> - _NSGetExecutablePath (Darwin)
>> - KERN_PROC_PATHNAME sysctl on BSDs.
>> - argv0, if absolute (all, including Windows).
>>
>> This is used to enable RUNTIME_PREFIX support for non-Windows systems,
>> notably Linux and Darwin. When configured with RUNTIME_PREFIX, Git will
>> do a best-effort resolution of its executable path and automatically use
>> this as its "exec_path" for relative helper and data lookups, unless
>> explicitly overridden.
>>
>> Git's PERL tooling now responds to RUNTIME_PREFIX_PERL. When configured,
>> Git's generated PERL scripts resolve the Git library location relative to
>> their runtime paths instead of hard-coding them. Structural changes
>> were made to Makefile to support selective PERL header generation.
>>
>> Small incidental formatting cleanup of "exec_cmd.c".
>
>> +$(SCRIPT_PERL_GEN): % : %.perl perl/perl.mak GIT-PERL-DEFINES GIT-PERL-HEADER 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 '	H' \
>> -	    -e '	x' \
>> +	    -e '	rGIT-PERL-HEADER' \
>> +	    -e '	G' \
>>  	    -e '}' \
>>  	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>>  	    $< >$@+ && \
>> @@ -1986,6 +2028,29 @@ GIT-PERL-DEFINES: FORCE
>>  		echo "$$FLAGS" >$@; \
>>  	    fi
>>
>> +GIT-PERL-HEADER: perl/perl.mak GIT-PERL-DEFINES FORCE
>> +ifndef RUNTIME_PREFIX_PERL
>> +	# Hardcode the runtime path.
>> +	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
>> +	INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>> +	echo \
>> +	  'use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));' \
>> +	  >$@
>> +else
>> +	# Probe the runtime path relative to the PERL script. RUNTIME_PREFIX_PERL
>> +	# automatically sets NO_PERL_MAKEMAKER, causing PERL scripts to be installed
>> +	# to "$(prefix)/lib" (see "perl/Makefile"). This expectation is hard-coded
>> +	# into the generated code below.
>> +	GITEXECDIR='$(gitexecdir_SQ)' && \
>> +	echo \
>> +	  'sub _get_git_lib{'\
>> +	  'use FindBin;'\
>> +	  '(my $$p=$$FindBin::Bin)=~s=/'$${GITEXECDIR}'$$==;'\
>> +		'return File::Spec->catdir($$p,"'"lib"'");' \
>> +	  '};' \
>> +	  'use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB}||_get_git_lib()));'\
>> +	  >$@
>> +endif
>
> If you run run:
>
>     make -j8 prefix=/tmp/git RUNTIME_PREFIX=YesPlease RUNTIME_PREFIX_PERL= CFLAGS="-O0 -g" all install
>
> And then:
>
>     make -j8 prefix=/tmp/git RUNTIME_PREFIX=YesPlease RUNTIME_PREFIX_PERL=YesPlease CFLAGS="-O0 -g" all install
>
> You end up with this:
>
>     $ tree /tmp/git/{lib,share/perl}
>     /tmp/git/lib
>     └── x86_64-linux-gnu
>         └── perl
>             └── 5.26.1
>                 ├── auto
>                 │ └── Git
>                 └── perllocal.pod
>     /tmp/git/share/perl
>     └── 5.26.1
>         [...]
>         └── Git.pm
>
> You need to bust the perl/PM.stamp cache as a function of your
> RUNTIME_PREFIX_PERL variable (or NO_PERL_MAKEMAKER).
>
> Other than that, is this whole NO_PERL_MAKEMAKER workaround just because
> you couldn't figure out what the target RELPERLPATH is in
> $prefix/$RELPERLPATH, which in this case is share/perl/5.26.1 ?
>
> I don't remember offhand how to extract that, but htis is built into
> perl itself, see e.g.:
>
>     $ PERL5LIB= /usr/bin/perl -E 'say for grep { m[share|lib] } @INC'
>     /usr/local/lib/x86_64-linux-gnu/perl/5.26.1
>     /usr/local/share/perl/5.26.1
>     /usr/lib/x86_64-linux-gnu/perl5/5.26
>     /usr/share/perl5
>     /usr/lib/x86_64-linux-gnu/perl/5.26
>     /usr/share/perl/5.26
>     /usr/local/lib/site_perl
>     /usr/lib/x86_64-linux-gnu/perl-base
>
> So it's in Config.pm somewhere IIRC. But your patch doesn't discuss why
> you toggled NO_PERL_MAKEMAKER, is it purely to work around this issue,
> and if we had some aesy way to figure out the target $RELPERLPATH we
> wouldn't need to do that?
>
> Seems a bit of baby & bathwater there :)

So LeonT over at #p5p helped me with this. He believes this'll work
(unless MakeMaker INSTALL_BASE is set, but that should break the Git
install anyway):


    /usr/bin/perl -MConfig -wE 'my ($relsite) = $Config{installsitelib} =~ m[^\Q$Config{siteprefixexp}\E/(.+)]s; say $relsite'
    share/perl/5.26.1

I.e. aside from my spiel below injecting this should work, and would
eliminate the need to toggle NO_PERL_MAKEMAKER (untested):

    BEGIN {
        use lib split /:/,
        (
            $ENV{GITPERLLIB}
            ||
            do {
                require FindBin;
                require File::Spec;
                require Config;
                Config->import;

                my ($relsite) = $Config{installsitelib} =~ m[^\Q$Config{siteprefixexp}\E/(.+)]s
                    or die "PANIC: Ohes noes $Config{siteprefixexp} doesn't match a subset of $Config{installsitelib}";
                File::Spec->catdir($FindBin::Bin, '..', '..', $relsite);
            }
        );
    }


> Aside from that:
>
> 1. The regex match you're doing to munge the dir could be done as a
>    catdir($orig, '..', '..', 'lib'), that doesn't work as discussed
>    above, but *might* be more portable. I say might because I don't know
>    if the path string is always normalized to be unix-like, but if not
>    this won't work e.g on Windows where it'll have \ not /.
>
> 2. You are 'use'-ing FindBin there unconditionally (use is not function
>    local in perl), and implicitly assuming it loads File::Spec.
>
>    Ignoring the NO_PERL_MAKEMAKER caveats above I'd suggest something
>    like this:
>
>        #!/usr/bin/perl
>
>        # BEGIN RUNTIME_PREFIX_PERL=YesPlease generated code.
>        #
>        # This finds our Git::* libraries at relative locations.
>        BEGIN {
>            use lib split /:/,
>            (
>                $ENV{GITPERLLIB}
>                ||
>                do {
>                    require FindBin;
>                    require File::Spec;
>                    File::Spec->catdir($FindBin::Bin, '..', '..', 'lib');
>                }
>            );
>        }
>        # END RUNTIME_PREFIX_PERL=YesPlease generated code.
>
>        # Copyright (C) 2006, Eric Wong <normalperson@yhbt.net>
>        # License: GPL v2 or later
>
>    It's also nice to have some whitespace / comments to note that this
>    is generated code.
>
>  3. I may be squinting at this wrong but it seems to me that between
>     your v1 and v2 reading GITPERLLIB here no longer makes any sense at
>     all. You used to set it in git itself, now it takes priority but
>     nothing sets it, presumably you'd have some external wrapper script
>     that'll set it?
>
>     Now if I compile with RUNTIME_PREFIX=YesPlease I get magic
>     auto-discovery of C program paths, right? But it'll still fallback
>     to the system perl libs (if any) unless
>     RUNTIME_PREFIX_PERL=YesPlease is set. Shouldn't we just make
>     RUNTIME_PREFIX=YesPlease Just Work for everything?