git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Dan Jacques <dnj@google.com>
Cc: Johannes.Schindelin@gmx.de, git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCH v5 2/3] Makefile: add Perl runtime prefix support
Date: Mon, 08 Jan 2018 21:00:10 +0100	[thread overview]
Message-ID: <878td8azp1.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180108191812.52565-1-dnj@google.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.

  reply	other threads:[~2018-01-08 20:00 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-08  3:02 [PATCH v5 0/3] RUNTIME_PREFIX relocatable Git Dan Jacques
2018-01-08  3:02 ` [PATCH v5 1/3] Makefile: generate Perl header from template file Dan Jacques
2018-01-08  3:02 ` [PATCH v5 2/3] Makefile: add Perl runtime prefix support Dan Jacques
2018-01-08  9:41   ` Ævar Arnfjörð Bjarmason
2018-01-08 19:18     ` Dan Jacques
2018-01-08 20:00       ` Ævar Arnfjörð Bjarmason [this message]
2018-01-08 20:27       ` Johannes Schindelin
2018-01-08 21:54         ` Junio C Hamano
2018-01-08 22:05         ` Dan Jacques
2018-01-08 22:16           ` Ævar Arnfjörð Bjarmason
2018-01-08 22:18             ` Dan Jacques
2018-01-08 20:24     ` Johannes Schindelin
2018-01-08  3:02 ` [PATCH v5 3/3] exec_cmd: RUNTIME_PREFIX on some POSIX systems Dan Jacques

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878td8azp1.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=dnj@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).