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

[Dan Jacques <dnj@google.com>]

On Mon, 20 Nov 2017 22:00:10, Ævar Arnfjörð Bjarmason replied:

> [...]

Thanks for responding. I'll readily confess that PERL and the PERL
ecosystem are not areas I'm very familiar with, so I'm really grateful
for your feedback here.

> You need to bust the perl/PM.stamp cache as a function of your
> RUNTIME_PREFIX_PERL variable (or NO_PERL_MAKEMAKER).

Good catch, I'll add that to the next version of the patch.

> 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 ?

Sort of. I actually had a version set hacked MakeMaker's $INSTALLSITELIB
(same general result) and it worked.

The executable path resolution logic that I built make assumptions about
the layout of the installation, and I ended up favoring NO_PERL_MAKEMAKER
because I could rely on its explicit and straightforward installation
logic.

I am not sure that MakeMaker adds value in a RUNTIME_PREFIX
installation, since RUNTIME_PREFIX's motivation is fundamentally portable
and my impression of MakeMaker is that much of the value that it adds is
handling system-specific PERL installation details. Given that
NO_PERL_MAKEMAKER is supported as a Git installation option, I opted to
take advantage of the explicit installation rather than rely on
MakeMaker as an intermediary.

My other motivation is that if I integrate $RELPERLPATH into the MakeMaker
installation, I'd still have to implement that behavior when
NO_PERL_MAKEMAMER is enabled so that it is compatible with
RUNTIME_PREFIX_PERL.

I'd welcome any insight on whether this is the correct way to proceed.
If we decode to go forward with NO_PERL_MAKEMAKER, I'm happy to add some
better documentation in the Makefile to detail the rationale for
forcefully enabling it.

> 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 /.

The regex-based approach was motivated by a perceived value to
conciseness. Since a larger header code block seems to be acceptable, I
could emit "$(gitexecdir)" as a constant in the header and operate on it
at runtime. This would avoid having to calculate the correct sequence of
"../" to walk up from "$(gitexecdir)" directly in the Makefile.

> 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:

Sounds good! I wasn't sure whether I should adhere to the one-line header
that was being used before, but I am all for whitespace if that is
acceptable.

This seems a bit much to emit from a Makefile - what do you think about
adding template files for each header in the "perl/" directory and
preprocessing them via "sed" in the Makefile?

> 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?

$GITPERLLIB is (as far as I can tell) used for testing, so that PERL scripts
invoked by tests can find the Git support libraries. I think it is still
necessary in both RUNTIME_PREFIX_PERL and default worlds because tests run
Git out of the repository root, so relative resolution logic based on
installation paths will not work.

Thanks again!
-Dan