Sent from my iPhone...
> On Jan 30, 2019, at 04:51, Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote:
>
> Hi Jeremy,
>
> On Tue, 29 Jan 2019, Jeremy Huddleston Sequoia wrote:
>
>>> On Jan 29, 2019, at 2:47 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>>
>>> Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx> writes:
>>>
>>>> Signed-off-by: Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx>
>>>> ---
>>>> 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?"
>>>> }
>>
>>
