Make the ninja generator handle symlinked paths correctly.

Make the ninja generator handle symlinked paths correctly.

Re-spin of https://codereview.chromium.org/1236933002.

Previously, RelativePath() was changed to use abspath instead of realpath unconditionally. This broke the XCode generator in some cases. This re-spin simply allows the caller to optionally specify that they do not want symlinks to be followed, and this is only done in the ninja generator. This approach is not ideal since it is not obvious why this option is needed for ninja but not elsewhere; but it is a much lower-risk change than the previous attempt.

Patch from lindleyf@google.com.

BUG=
R=scottmg@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/7904ce96fd012ded9627172540e91a51025e3c08

[lindleyf, 2015-09-24 20:51:40]

lindleyf@google.com changed reviewers:
+ thakis@chromium.org

[lindleyf, 2015-09-24 20:51:41]

Here's a lower-risk re-spin of the abspath-vs-realpath CL.

[Nico, 2015-09-24 21:13:35]

(I'm away for the next ~2 weeks starting in a few minutes. if you want this
earlier than then, maybe scottmg can take a look at this.)

[lindleyf, 2015-09-24 21:28:53]

On 2015/09/24 20:51:41, lindleyf wrote:
> Here's a lower-risk re-spin of the abspath-vs-realpath CL.

I have tested this on the Make, Ninja, XCode, and XCode-Ninja generators.

[scottmg, 2015-09-24 21:30:50]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2015-09-24 21:30:51]

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/common.py
File pylib/gyp/common.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/common.py#new...
pylib/gyp/common.py:134: def RelativePath(path, relative_to,
follow_path_symlink=True):
could you add some explanation for this arg here? It's not immediately obvious
what it means to me. (something like what's in the head of the test I guess)

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:924: elif self.toolset == 'host':
why is this is in this CL?

[lindleyf, 2015-09-24 21:31:07]

+scottmg since thakis is going on vacation

[lindleyf, 2015-09-24 21:47:14]

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/common.py
File pylib/gyp/common.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/common.py#new...
pylib/gyp/common.py:134: def RelativePath(path, relative_to,
follow_path_symlink=True):
On 2015/09/24 21:30:51, scottmg wrote:
> could you add some explanation for this arg here? It's not immediately obvious
> what it means to me. (something like what's in the head of the test I guess)

I've added a comment here and at the point of use. Let me know if it's clear.

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:924: elif self.toolset == 'host':
On 2015/09/24 21:30:51, scottmg wrote:
> why is this is in this CL?

I could move that to a different CL if you prefer. It's only here because it was
also in the original CL that got rolled back.

[scottmg, 2015-09-24 21:59:09]

The comments help a lot! lgtm with the .host change removed.

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:924: elif self.toolset == 'host':
On 2015/09/24 21:47:14, lindleyf wrote:
> On 2015/09/24 21:30:51, scottmg wrote:
> > why is this is in this CL?
> 
> I could move that to a different CL if you prefer. It's only here because it
was
> also in the original CL that got rolled back.

yes, if it's unrelated, it should be a separate change.

[lindleyf, 2015-09-24 22:10:20]

On 2015/09/24 21:59:09, scottmg wrote:
> The comments help a lot! lgtm with the .host change removed.
> 
>
https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
> File pylib/gyp/generator/ninja.py (right):
> 
>
https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
> pylib/gyp/generator/ninja.py:924: elif self.toolset == 'host':
> On 2015/09/24 21:47:14, lindleyf wrote:
> > On 2015/09/24 21:30:51, scottmg wrote:
> > > why is this is in this CL?
> > 
> > I could move that to a different CL if you prefer. It's only here because it
> was
> > also in the original CL that got rolled back.
> 
> yes, if it's unrelated, it should be a separate change.

I'm going to try to land it, but in the past I haven't been able to because I
don't have permissions. I'll let you know if I need you to land it for me.

[lindleyf, 2015-09-24 22:10:26]

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
File pylib/gyp/generator/ninja.py (right):

https://codereview.chromium.org/1365163002/diff/20001/pylib/gyp/generator/nin...
pylib/gyp/generator/ninja.py:924: elif self.toolset == 'host':
On 2015/09/24 21:59:09, scottmg wrote:
> On 2015/09/24 21:47:14, lindleyf wrote:
> > On 2015/09/24 21:30:51, scottmg wrote:
> > > why is this is in this CL?
> > 
> > I could move that to a different CL if you prefer. It's only here because it
> was
> > also in the original CL that got rolled back.
> 
> yes, if it's unrelated, it should be a separate change.

Okay, reverted. I'll do this separately.

[scottmg, 2015-09-24 22:12:16]

https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
File test/symlinks/gyptest-symlinks.py (right):

https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:28: import gyp.common
oh, what's this for?

[lindleyf, 2015-09-24 22:15:50]

https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
File test/symlinks/gyptest-symlinks.py (right):

https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:28: import gyp.common
On 2015/09/24 22:12:16, scottmg wrote:
> oh, what's this for?

Good catch, I think that's left over from a previous version of this test.
Removing it.

[lindleyf, 2015-09-24 22:22:40]

On 2015/09/24 22:15:50, lindleyf wrote:
>
https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
> File test/symlinks/gyptest-symlinks.py (right):
> 
>
https://codereview.chromium.org/1365163002/diff/60001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:28: import gyp.common
> On 2015/09/24 22:12:16, scottmg wrote:
> > oh, what's this for?
> 
> Good catch, I think that's left over from a previous version of this test.
> Removing it.

Yeah, my attempts at landing this aren't working again. Can you do the land for
me?

[scottmg, 2015-09-24 22:53:28]

(removed blank line at end of gyptest-symlinks.py)

I'll land once bots come back green. Thanks!

[scottmg, 2015-09-24 23:13:26]

(fixed hello.c to pass git cl presubmit too. unfortunately i'm not allowed to
upload to not-my-issue)

[scottmg, 2015-09-24 23:13:56]

Committed patchset #5 (id:80001) manually as
7904ce96fd012ded9627172540e91a51025e3c08 (presubmit successful).

[lindleyf, 2015-09-24 23:14:49]

On 2015/09/24 23:13:26, scottmg wrote:
> (fixed hello.c to pass git cl presubmit too. unfortunately i'm not allowed to
> upload to not-my-issue)

What was the fix? I stared at that error message for a while but couldn't figure
out what it was unhappy about.

[scottmg, 2015-09-24 23:17:23]

On 2015/09/24 23:14:49, lindleyf wrote:
> On 2015/09/24 23:13:26, scottmg wrote:
> > (fixed hello.c to pass git cl presubmit too. unfortunately i'm not allowed
to
> > upload to not-my-issue)
> 
> What was the fix? I stared at that error message for a while but couldn't
figure
> out what it was unhappy about.

Oh, sorry, I should have just commented instead.

The */ has to be on a separate line, apparently.