| Message ID | 20190129193818.8645-3-jeremyhu@apple.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Differences between git-2.20.1 and Apple Git-116 | expand |
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?" > }
> 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?" >> }
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?" > >> } >
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?" >>>> } >>
/usr/local/versioner/perl/versions is also not provided with macOS or Xcode AFAIK Carlo
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?" > >> } > >
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?" >>>> } >> >>
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.
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?" }
Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com> --- t/test-lib.sh | 3 +++ 1 file changed, 3 insertions(+)