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
