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

From: Dan Jacques <dnj@google.com>
Date: Nov 20, 2017, 21:58 UTC
Message-ID: <20171120215805.16994-1-dnj@google.com>
In-Reply-To: <87lgj0wtr9.fsf@evledraar.booking.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.

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 5 quoted lines

> 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