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

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

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