Re: [PATCH v5 2/3] Makefile: add Perl runtime prefix support

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jan 08 2018, Dan Jacques jotted:

> On Mon, 08 Jan 2018, Ævar Arnfjörð Bjarmason replied:
>>>+# it. This is intentionally separate from RUNTIME_PREFIX so that notably Windows
>>>+# can hard-code Perl library paths while still enabling RUNTIME_PREFIX
>>>+# resolution.
>>
>> Maybe we covered this in previous submissions, but refresh my memory,
>> why is the *_PERL define still needed? Reading this explanation doesn't
>> make sense to me, but I'm probably missing something.
>>
>> If we have a system where we have some perl library paths on the system
>> we want to use, then they'll still be in @INC after our 'use lib'-ing,
>> so we'll find libraries there.
>>
>> The only reason I can think of for doing this for C and not for Perl
>> would be if you for some reason want to have a git in /tmp/git but then
>> use a different version of the Git.pm from some system install, but I
>> can't imagine why.
>
> The reason is entirely due to the way Git-for-Windows is structured. In
> Git-for-Windows, Git binaries are run directly from Windows, meaning that
> they require RUNTIME_PREFIX resolution. However, Perl scripts are run from
> a MinGW universe, within which filesystem paths are fixed. Therefore,
> Windows Perl scripts don't require a runtime prefix resolution.
>
> This makes sense because they are clearly functional right now without this
> patch enabled :) However, we don't have the luxury of running Perl in a
> separate universe on other OSes, so this patch is necessary for them.
>
> I created a separate option because I wanted to ensure that I don't change
> anything fundamental in Windows, which currently relies on runtime prefix
> resoultion. On all other operating systems, Perl and binary runtime prefix
> resolution is disabled by default, so if this patch set does end up having
> bugs or edge cases in the Perl runtime prefix code, it won't inpact anybody's
> current builds.
>
> I can foresee a future where Windows maintainers decide that
> PERL_RUNTIME_PREFIX is fine for Windows and merge the two options; however,
> I didn't want to force that decision in the initial implementation.

Makes sense, well not really, But that's not your fault, but Windows's.

I do think you're being overly conservative here, the perl change is no
more invasive than the C changes (less so actually), and from anyone
who's not on Windows it makes sense to be able to enable this with just
RUNTIME_PREFIX=YesPlease, and have NO_RUNTIME_PREFIX_PERL=NotNeededHere
for Windows, if someone ends up needing it.

We usually hide stuff you might want in general, but isn't needed on one
special snowflake platform behind NO_*, not the other way around.

Maybe others disagre... 

>> > +	# GIT_EXEC_PATH is supplied by `git` or the test suite. Otherwise, resolve
>> > +	# against the runtime path of this script.
>> > +	require FindBin;
>> > +	require File::Spec;
>> > +	(my $prefix = $ENV{GIT_EXEC_PATH} || $FindBin::Bin) =~ s=${gitexecdir_relative}$==;
>>
>> So why are we falling back on $FindBin::Bin? Just so you can do
>> e.g. /tmp/git2/libexec/git-core/git-svn like you can do
>> /tmp/git2/libexec/git-core/git-status, i.e. will this never be false if
>> invoked via "git"?
>>
>> I don't mind it, just wondering if I'm missing something and we need to
>> use the fallback path in some "normal" codepath.
>
> Yep, exactly. The ability to directly invoke Perl scripts is currently
> functional in non-runtime-prefix builds, so enabling it in runtime-prefix
> builds seemed appropriate. I have found this useful for testing.
>
> However, since GIT_EXEC_PATH is probably going to be the common path,
> I'll scoop the FindBin code (including the "require" statement) into a
> conditional in v6 and use it only when GIT_EXEC_PATH is empty.

Both make sense.

>> > +	return File::Spec->catdir($prefix, $relpath);
>>
>> I think you initially got some version of this from me (or not), so this
>> is probably my fault, but reading this again I think this would be
>> better as just:
>>
>>     return $prefix . '@@PATHSEP@@' . $relpath;
>>
>> I.e. right after this we split on @@PATHSEP@@, and that clearly works
>> (as opposed to using File::Spec->splitpath) since we've used it
>> forever.
>>
>> Better just to use the same idiom on both ends to not leave the reader
>> wondering why we can split paths one way, but need to join them another
>> way.
>
> PATHSEP is the path separator (":"), as opposed to the filesystem separator
> ("/"). We split on PATHSEP below b/c we need to "use lib" as an array, but
> it may be a ":"-delimited string.

Yes, silly me. Nevermind.