git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Dan Jacques <dnj@google.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 1/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems
Date: Mon, 20 Nov 2017 22:50:12 +0100	[thread overview]
Message-ID: <87k1ykwrfv.fsf@evledraar.booking.com> (raw)
In-Reply-To: <87lgj0wtr9.fsf@evledraar.booking.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?

  reply	other threads:[~2017-11-20 21:50 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-19 17:31 [PATCH v2 0/1] exec_cmd: RUNTIME_PREFIX on some POSIX systems Dan Jacques
2017-11-19 17:31 ` [PATCH 1/1] " Dan Jacques
2017-11-20  1:01   ` Junio C Hamano
2017-11-20 21:00   ` Ævar Arnfjörð Bjarmason
2017-11-20 21:50     ` Ævar Arnfjörð Bjarmason [this message]
2017-11-21  2:41       ` Re " Dan Jacques
2017-11-22 12:09         ` Ævar Arnfjörð Bjarmason
2017-11-22 14:27           ` Dan Jacques
2017-11-20 21:58     ` Re(2): " Dan Jacques
2017-11-20 22:46       ` Ævar Arnfjörð Bjarmason
  -- strict thread matches above, loose matches on Subject: below --
2017-11-16 17:05 [PATCH 0/1] RUNTIME_PREFIX on " Dan Jacques
2017-11-16 17:05 ` [PATCH 1/1] exec_cmd: RUNTIME_PREFIX on some " Dan Jacques
2017-11-17  5:08   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k1ykwrfv.fsf@evledraar.booking.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=dnj@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).