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

[Dan Jacques]

On Mon, 08 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> > I'll add a "NO_RUNTIME_PREFIX_PERL" flag as per avarab@'s suggestion as a
> > potential mitigation if a problem does end up arising in Windows builds,
> > with a note that NO_RUNTIME_PREFIX_PERL can be deleted if everything seems
> > to be working. What do you think?
>
> To be clear, I meant that if it's determined by you/others that an
> opt-out on Windows is needed I think it makes sense to make it a NO_*
> flag, but if there's a solution where we can just turn it on for
> everything then ideally we'd just have RUNTIME_PREFIX=YesPlease.

Oh that's fair. Okay, we'll go all in and just have RUNTIME_PREFIX.

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 08 2018, Dan Jacques jotted:

> On 2018-01-08 20:27, Johannes Schindelin wrote:
>
>> > 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 the reason is to accommodate Windows, I think it'd make more sense to
>> change the way Git for Windows handles this, and use the same relative
>> paths (if possible, that is, see the GITPERLLIB problems I mentioned
>> elsewhere and which necessitated
>> https://github.com/git-for-windows/git/commit/3b2f716bd8).
>> (...)
>> What do you think? Should we just fold the RUNTIME_PREFIX_PERL handling
>> into RUNTIME_PREFIX and be done with that part?
>> (...)
>> As I mentioned in the mail I just finished and sent (I started it hours
>> ago, but then got busy with other things while the builds were running): I
>> am totally cool with changing this on Windows, too. Should simplify
>> things, right?
>
> No objections here. I see it as adding slightly more risk to this patch's
> potential impact on Windows builds, but if Git-for-Windows is okay with that,
> I'll go ahead and fold RUNTIME_PREFIX_PERL into RUNTIME_PREFIX for
> simplicity's sake.
>
> I'll add a "NO_RUNTIME_PREFIX_PERL" flag as per avarab@'s suggestion as a
> potential mitigation if a problem does end up arising in Windows builds,
> with a note that NO_RUNTIME_PREFIX_PERL can be deleted if everything seems
> to be working. What do you think?

To be clear, I meant that if it's determined by you/others that an
opt-out on Windows is needed I think it makes sense to make it a NO_*
flag, but if there's a solution where we can just turn it on for
everything then ideally we'd just have RUNTIME_PREFIX=YesPlease.

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

[Dan Jacques]

On 2018-01-08 20:27, Johannes Schindelin wrote:

> > 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 the reason is to accommodate Windows, I think it'd make more sense to
> change the way Git for Windows handles this, and use the same relative
> paths (if possible, that is, see the GITPERLLIB problems I mentioned
> elsewhere and which necessitated
> https://github.com/git-for-windows/git/commit/3b2f716bd8).
> (...)
> What do you think? Should we just fold the RUNTIME_PREFIX_PERL handling
> into RUNTIME_PREFIX and be done with that part?
> (...)
> As I mentioned in the mail I just finished and sent (I started it hours
> ago, but then got busy with other things while the builds were running): I
> am totally cool with changing this on Windows, too. Should simplify
> things, right?

No objections here. I see it as adding slightly more risk to this patch's
potential impact on Windows builds, but if Git-for-Windows is okay with that,
I'll go ahead and fold RUNTIME_PREFIX_PERL into RUNTIME_PREFIX for
simplicity's sake.

I'll add a "NO_RUNTIME_PREFIX_PERL" flag as per avarab@'s suggestion as a
potential mitigation if a problem does end up arising in Windows builds,
with a note that NO_RUNTIME_PREFIX_PERL can be deleted if everything seems
to be working. What do you think?

> BTW I managed to run your `runtime-prefix` branch through VSTS builds on
> Windows, macOS and Linux and they all pass the test suite. (Including the
> RUNTIME_PREFIX_PERL=YesPlease setting you added for Travis CI testing.)

Great news, thanks for doing this!

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

[Junio C Hamano]

Johannes Schindelin  writes:

> On Mon, 8 Jan 2018, Dan Jacques wrote:
>
>> 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.
>
> As I mentioned in the mail I just finished and sent (I started it hours
> ago, but then got busy with other things while the builds were running): I
> am totally cool with changing this on Windows, too. Should simplify
> things, right?

Wonderful to see that you two are in agreement.  Will look forward
to see a simplified solution in a later round ;-)

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

[Johannes Schindelin]

Hi,

On Mon, 8 Jan 2018, Dan Jacques wrote:

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

As I mentioned in the mail I just finished and sent (I started it hours
ago, but then got busy with other things while the builds were running): I
am totally cool with changing this on Windows, too. Should simplify
things, right?

Ciao,
Johannes

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

[Johannes Schindelin]

Hi,

On Mon, 8 Jan 2018, Ævar Arnfjörð Bjarmason wrote:

> 
> On Mon, Jan 08 2018, Dan Jacques jotted:
> 
> From 3/3 (not not send 2 e-mails):
> 
> >+# 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 the reason is to accommodate Windows, I think it'd make more sense to
change the way Git for Windows handles this, and use the same relative
paths (if possible, that is, see the GITPERLLIB problems I mentioned
elsewhere and which necessitated
https://github.com/git-for-windows/git/commit/3b2f716bd8).

BTW I managed to run your `runtime-prefix` branch through VSTS builds on
Windows, macOS and Linux and they all pass the test suite. (Including the
RUNTIME_PREFIX_PERL=YesPlease setting you added for Travis CI testing.)

What do you think? Should we just fold the RUNTIME_PREFIX_PERL handling
into RUNTIME_PREFIX and be done with that part?

Ciao,
Johannes

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Dan Jacques]

On Mon, 08 Jan 2018, Ævar Arnfjörð Bjarmason replied:

> Thanks, applied this on top of next and it works for me, i.e. install to
> /tmp/git and move to /tmp/git2 = works for me. Comments below.

Good to hear! I've run this through a few machines at my disposal, but
the more hands on the better.

>> Enabling RUNTIME_PREFIX_PERL overrides the system-specific Perl script
>> installation path generated by MakeMaker to force installation into a
>> platform-neutral location, "/share/perl5".
>
> Not generated by MakeMaker anymore :)

Hah good catch! I'll update the commit message.

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

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

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

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

[Ævar Arnfjörð Bjarmason]

On Mon, Jan 08 2018, Dan Jacques jotted:

Thanks, applied this on top of next and it works for me, i.e. install to
/tmp/git and move to /tmp/git2 = works for me. Comments below.

> Enabling RUNTIME_PREFIX_PERL overrides the system-specific Perl script
> installation path generated by MakeMaker to force installation into a
> platform-neutral location, "/share/perl5".

Not generated by MakeMaker anymore :)

>From 3/3 (not not send 2 e-mails):

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

Or there's another option...

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

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