On Mon, Nov 20 2017, Dan Jacques jotted: > 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. Right, it needs some if/else, or we could simply always add lib/ as well to @INC (via "use lib") and it would work in both modes unconditionally. > 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. My impression is (see `git log --reverse -p -GNO_PERL_MAKEMAKER`) that almost nobody uses NO_PERL_MAKEMAKER, so it would be better to make it work with the more supported mode since it seems easy. But I've now forgotten all the details of why we're even using MakeMaker in the first place, if I ever actually knew them. >> 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. Yeah I think nobody's going to have any problem with a large header block. >> 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? Sure, that sounds great. >> 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. Ah, thanks. Makes sense, I missed that.
