[(Apple,Git),02/13] test-lib: Export PERL5LIB for testing git-svn
diff mbox series

Message ID 20190129193818.8645-3-jeremyhu@apple.com
State New
Headers show
Series
  • Differences between git-2.20.1 and Apple Git-116
Related show

Commit Message

Jeremy Sequoia Jan. 29, 2019, 7:38 p.m. UTC
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
---
 t/test-lib.sh | 3 +++
 1 file changed, 3 insertions(+)

Comments

Junio C Hamano Jan. 29, 2019, 10:47 p.m. UTC | #1
Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> ---
>  t/test-lib.sh | 3 +++
>  1 file changed, 3 insertions(+)

This obviously won't be acceptable as-is to my tree.  Shouldn't this
be something to be dealt with in config.mak.uname or something that
is meant to define platform-specific customization?

>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f1faa24b2..4060a53f56 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1017,6 +1017,9 @@ fi
>  
>  GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>  export GITPERLLIB
> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> +export PERL5LIB
>  test -d "$GIT_BUILD_DIR"/templates/blt || {
>  	error "You haven't built things yet, have you?"
>  }
Jeremy Sequoia Jan. 29, 2019, 11:46 p.m. UTC | #2
> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> 
>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>> ---
>> t/test-lib.sh | 3 +++
>> 1 file changed, 3 insertions(+)
> 
> This obviously won't be acceptable as-is to my tree.  Shouldn't this
> be something to be dealt with in config.mak.uname or something that
> is meant to define platform-specific customization?

The issue here is that we're not locating relocatable perl modules during testing.  This is a general problem with testing RUNTIME_PREFIX configurations, and a more general solution to this sledgehammer would be appropriate.  I don't think config.mak.uname really makes sense since it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.

> 
>> 
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 0f1faa24b2..4060a53f56 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1017,6 +1017,9 @@ fi
>> 
>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>> export GITPERLLIB
>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
>> +export PERL5LIB
>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>> 	error "You haven't built things yet, have you?"
>> }
SZEDER Gábor Jan. 29, 2019, 11:59 p.m. UTC | #3
On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
> 
> 
> > On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> >> t/test-lib.sh | 3 +++
> >> 1 file changed, 3 insertions(+)
> > 
> > This obviously won't be acceptable as-is to my tree.  Shouldn't this
> > be something to be dealt with in config.mak.uname or something that
> > is meant to define platform-specific customization?
> 
> The issue here is that we're not locating relocatable perl modules
> during testing.  This is a general problem with testing
> RUNTIME_PREFIX configurations, and a more general solution to this
> sledgehammer would be appropriate.  I don't think config.mak.uname
> really makes sense since it's a general RUNTIME_PREFIX issue and not
> specifically a darwin issue.

But this patch is very darwin-specific ...

> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 0f1faa24b2..4060a53f56 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -1017,6 +1017,9 @@ fi
> >> 
> >> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
> >> export GITPERLLIB
> >> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> >> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION

... because what other platforms could possibly have 'xcode-select'
installed!?  Consequently:

  $ ./t0000-basic.sh 
  grep: /usr/local/versioner/perl/versions: No such file or directory
  ./t0000-basic.sh: 1154: ./test-lib.sh: xcode-select: not found
  ok 1 - verify that the running shell supports "local"
  ok 2 - .git/objects should be empty after git init in an empty repo
  ok 3 - .git/objects should have 3 subdirectories
  ok 4 - success is reported like this
  not ok 5 - pretend we have a fully passing test suite
  <...>
  # failed 29 among 82 test(s)



> >> +export PERL5LIB
> >> test -d "$GIT_BUILD_DIR"/templates/blt || {
> >> 	error "You haven't built things yet, have you?"
> >> }
>
Jeremy Sequoia Jan. 30, 2019, 12:01 a.m. UTC | #4
Sent from my iPhone...

> On Jan 29, 2019, at 15:59, SZEDER Gábor <szeder.dev@gmail.com> wrote:
> 
>> On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
>> 
>> 
>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>>> t/test-lib.sh | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>> 
>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>> be something to be dealt with in config.mak.uname or something that
>>> is meant to define platform-specific customization?
>> 
>> The issue here is that we're not locating relocatable perl modules
>> during testing.  This is a general problem with testing
>> RUNTIME_PREFIX configurations, and a more general solution to this
>> sledgehammer would be appropriate.  I don't think config.mak.uname
>> really makes sense since it's a general RUNTIME_PREFIX issue and not
>> specifically a darwin issue.
> 
> But this patch is very darwin-specific ...
> 
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 0f1faa24b2..4060a53f56 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -1017,6 +1017,9 @@ fi
>>>> 
>>>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>>>> export GITPERLLIB
>>>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>>>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> 
> ... because what other platforms could possibly have 'xcode-select'
> installed!?  Consequently:
> 
>  $ ./t0000-basic.sh 
>  grep: /usr/local/versioner/perl/versions: No such file or directory
>  ./t0000-basic.sh: 1154: ./test-lib.sh: xcode-select: not found
>  ok 1 - verify that the running shell supports "local"
>  ok 2 - .git/objects should be empty after git init in an empty repo
>  ok 3 - .git/objects should have 3 subdirectories
>  ok 4 - success is reported like this
>  not ok 5 - pretend we have a fully passing test suite
>  <...>
>  # failed 29 among 82 test(s)

Yes.  This is one of the patches that I said in the 00 message would certainly not be upstreamable but for which we should find a general solution to the problem if one is available.

> 
> 
> 
>>>> +export PERL5LIB
>>>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>>>>    error "You haven't built things yet, have you?"
>>>> }
>>
Carlo Arenas Jan. 30, 2019, 12:07 a.m. UTC | #5
/usr/local/versioner/perl/versions is also not provided with macOS or
Xcode AFAIK

Carlo
Johannes Schindelin Jan. 30, 2019, 12:51 p.m. UTC | #6
Hi Jeremy,

On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:

> > On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
> > 
> >> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
> >> ---
> >> t/test-lib.sh | 3 +++
> >> 1 file changed, 3 insertions(+)
> > 
> > This obviously won't be acceptable as-is to my tree.  Shouldn't this
> > be something to be dealt with in config.mak.uname or something that
> > is meant to define platform-specific customization?
> 
> The issue here is that we're not locating relocatable perl modules
> during testing.  This is a general problem with testing RUNTIME_PREFIX
> configurations, and a more general solution to this sledgehammer would
> be appropriate.  I don't think config.mak.uname really makes sense since
> it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.

First of all, as others have pointed out, this code is very, very specific
to Darwin (not only xcode-select but also Library/Perl/ are very, very
specific to that platform, I would even argue it is not even
Darwin-specific but instead macOS specific because bare-bones Darwin does
not have Library/Perl/, does it?).

So you *definitely* want to put that code into guards testing for that
platform (I do not think config.mak.uname is the correct place, though, as
it should be accessible to test scripts when run directly, i.e. not
through `make`).

But let's take a huge step back first: why? What is the exact problem this
commit tries to solve? The commit message unfortunately does not really
leave me any wiser.

So I am left with the unfortunate position of having to guess, which is
not really a good use of both of our time. If I allow myself to indulge in
the guessing game, I would guess that whatever `perl` executable is used
in your scenario picks up some unfortunate environment variable that
overrides its internal defaults where to look for Perl modules.

And that simply should not be the case. We are very careful to set
GITPERLLIB in bin-wrappers/, *not* PERL5LIB.

And when we build Git on macOS agents in Travis or Azure Pipelines and
then run the test suite, I fail to see any Perl-related error that looks
like it could be solved by this here patch.

In short: this commit is in dear want of a more substantive commit
message, and most likely in search for a different solution.

Ciao,
Johannes

> 
> > 
> >> 
> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 0f1faa24b2..4060a53f56 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -1017,6 +1017,9 @@ fi
> >> 
> >> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
> >> export GITPERLLIB
> >> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
> >> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
> >> +export PERL5LIB
> >> test -d "$GIT_BUILD_DIR"/templates/blt || {
> >> 	error "You haven't built things yet, have you?"
> >> }
> 
>
Jeremy Sequoia Jan. 30, 2019, 6:45 p.m. UTC | #7
Sent from my iPhone...

> On Jan 30, 2019, at 04:51, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> Hi Jeremy,
> 
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
> 
>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> 
>>> Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:
>>> 
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
>>>> ---
>>>> t/test-lib.sh | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>> 
>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>> be something to be dealt with in config.mak.uname or something that
>>> is meant to define platform-specific customization?
>> 
>> The issue here is that we're not locating relocatable perl modules
>> during testing.  This is a general problem with testing RUNTIME_PREFIX
>> configurations, and a more general solution to this sledgehammer would
>> be appropriate.  I don't think config.mak.uname really makes sense since
>> it's a general RUNTIME_PREFIX issue and not specifically a darwin issue.
> 
> First of all, as others have pointed out, this code is very, very specific
> to Darwin (not only xcode-select but also Library/Perl/ are very, very
> specific to that platform, I would even argue it is not even
> Darwin-specific but instead macOS specific because bare-bones Darwin does
> not have Library/Perl/, does it?).

Yes.  I first pointed that out in my emails to Peff and in my 00 email ;).  Peff requested that I send  all of our changes (even ones I considered not upstreamable) in order to discuss possible generalized solutions that could apply to others as well.

> So you *definitely* want to put that code into guards testing for that
> platform (I do not think config.mak.uname is the correct place, though, as
> it should be accessible to test scripts when run directly, i.e. not
> through `make`).

It isn’t applicable to anyone outside of Apple internal build engineers (or maybe folks like OpenDarwin building from our OSS perl and python drops too) as it is specific to Apple’s build systems.

However a generalized solution would be useful to others.

> But let's take a huge step back first: why? What is the exact problem this
> commit tries to solve? The commit message unfortunately does not really
> leave me any wiser.
> 
> So I am left with the unfortunate position of having to guess, which is
> not really a good use of both of our time. If I allow myself to indulge in
> the guessing game, I would guess that whatever `perl` executable is used
> in your scenario picks up some unfortunate environment variable that
> overrides its internal defaults where to look for Perl modules.

The issue is with RUNTIME_PREFIX.  git’s RUNTINE_PREFIX support assumes that it is the only thing being relocated.  However, with Xcode, svn and its perl modules are relocated as well.  In order to test git-svn, we need to locate those perl modules.  Patch 10 takes care of this when running from the installed location, but we have no svn in the appropriate relative location from the build directory, so we add the explicit path here.

> And that simply should not be the case. We are very careful to set
> GITPERLLIB in bin-wrappers/, *not* PERL5LIB.
> 
> And when we build Git on macOS agents in Travis or Azure Pipelines and
> then run the test suite, I fail to see any Perl-related error that looks
> like it could be solved by this here patch.
> 
> In short: this commit is in dear want of a more substantive commit
> message, and most likely in search for a different solution.

Yes, a number of these patches (like this one) were requested to be sent to the list in order to spark a discussion for another generalized solution and not to be merged into mainline.

Is there a notation that would help to call that out on the commit?  I figured it was pretty obvious that this was one of those.

> 
> Ciao,
> Johannes
> 
>> 
>>> 
>>>> 
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 0f1faa24b2..4060a53f56 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -1017,6 +1017,9 @@ fi
>>>> 
>>>> GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
>>>> export GITPERLLIB
>>>> +PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
>>>> +PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
>>>> +export PERL5LIB
>>>> test -d "$GIT_BUILD_DIR"/templates/blt || {
>>>>    error "You haven't built things yet, have you?"
>>>> }
>> 
>>
Junio C Hamano Jan. 30, 2019, 6:59 p.m. UTC | #8
Jeremy Sequoia <jeremyhu@apple.com> writes:

>> On Jan 29, 2019, at 15:59, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> 
>>> On Tue, Jan 29, 2019 at 03:46:07PM -0800, Jeremy Huddleston Sequoia wrote:
>>> 
>>> 
>>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> 
>>>> This obviously won't be acceptable as-is to my tree.  Shouldn't this
>>>> be something to be dealt with in config.mak.uname or something that
>>>> is meant to define platform-specific customization?
>>> 
>>> The issue here is that we're not locating relocatable perl modules
>>> during testing.  This is a general problem with testing
>>> RUNTIME_PREFIX configurations, and a more general solution to this
>>> sledgehammer would be appropriate.  I don't think config.mak.uname
>>> really makes sense since it's a general RUNTIME_PREFIX issue and not
>>> specifically a darwin issue.
>> 
>> But this patch is very darwin-specific ...
>>  ...
>
> Yes.  This is one of the patches that I said in the 00 message
> would certainly not be upstreamable but for which we should find a
> general solution to the problem if one is available.

Yes, I do appreciate seeing these non-upstreamable ones, as they
serve to illustrate issues that may want to be helped with a bit
more customizability in our tree.  I suspect some of them may
already have enough solution on our side without any need for
further patching (e.g. the "version" one Dscho mentioned for the
06/13), though.

BTW, I'll be mostly offline today, so I'll return to the discussion
tomorrow.

Patch
diff mbox series

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..4060a53f56 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1017,6 +1017,9 @@  fi
 
 GITPERLLIB="$GIT_BUILD_DIR"/perl/build/lib
 export GITPERLLIB
+PERL_VERSION=$(grep DEFAULT /usr/local/versioner/perl/versions | sed 's:^.*= *\([^ ]*\)$:\1:')
+PERL5LIB="$GIT_BUILD_DIR"/perl:"$(xcode-select -p)"/Library/Perl/$PERL_VERSION
+export PERL5LIB
 test -d "$GIT_BUILD_DIR"/templates/blt || {
 	error "You haven't built things yet, have you?"
 }