Make RelativePath use abspath rather than realpath for the 'path' variable.

Make RelativePath use abspath rather than realpath for the 'path' variable.
This allows gyp to work correctly in symlink-heavy environments. Basically,
this is because gyp paths need to be in a consistent tree, so we need to
compute a path to the target within the *stated* tree, even if it is not
the real underlying path to the target. The 'relative_path' variable does
need to be resolved using realpath, since gyp or the underlying build system
might cd to it before looking for the 'path' target.

Also make Ninja's host configuration consult environment variables analogous
to those it previously had for the target configuration, and replace os.getenv
with os.environ.get for consistency.

R=thakis@chromium.org

Committed: https://chromium.googlesource.com/external/gyp/+/3a0fcb40557e3ba02273609e49801fdb714f9ee2

Committed: https://chromium.googlesource.com/external/gyp/+/121d89dfcd4f6ebe1c89524b3f9ca11ddd437e77

[lindleyf, 2015-07-13 17:18:51]

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

[Nico, 2015-07-13 17:29:24]

not lgtm, projects generated by gyp should be relocatable by default

if it helps you, this could be some opt-in optional thing. in that case, please
add a test.

[lindleyf, 2015-07-13 17:33:21]

On 2015/07/13 17:29:24, Nico wrote:
> not lgtm, projects generated by gyp should be relocatable by default
> 
> if it helps you, this could be some opt-in optional thing. in that case,
please
> add a test.

Can you define 'relocatable' in this case? As far as I can tell, this makes
cases that didn't work before start working, without breaking any existing
cases.

[Nico, 2015-07-13 17:38:48]

i think i misunderstood what this does. can you add a test that demonstrates
where this difference is important?

[lindleyf, 2015-07-13 17:41:30]

On 2015/07/13 17:38:48, Nico wrote:
> i think i misunderstood what this does. can you add a test that demonstrates
> where this difference is important?

I should be able to add a test, assuming the underlying repository understands
checked-in symlinks and/or I can find a way to generate one on the fly....

[lindleyf, 2015-07-13 22:50:34]

On 2015/07/13 17:41:30, lindleyf wrote:
> On 2015/07/13 17:38:48, Nico wrote:
> > i think i misunderstood what this does. can you add a test that demonstrates
> > where this difference is important?
> 
> I should be able to add a test, assuming the underlying repository understands
> checked-in symlinks and/or I can find a way to generate one on the fly....

After thinking about this for a while, I'm not entirely sure how to proceed. The
problem manifests most obviously (I can't say for sure this is the only time)
when the realpath to a gyp file is *outside* the tree rooted at --depth. In
particular, the build_to_base variable in ninja.py becomes bogus.

This would be simple enough to test for except that tests are expected to be run
from the root directory of the repo already, so simulating a file outside that
tree is a bit difficult.

[lindleyf, 2015-08-19 01:00:22]

On 2015/07/13 22:50:34, lindleyf wrote:
> On 2015/07/13 17:41:30, lindleyf wrote:
> > On 2015/07/13 17:38:48, Nico wrote:
> > > i think i misunderstood what this does. can you add a test that
demonstrates
> > > where this difference is important?
> > 
> > I should be able to add a test, assuming the underlying repository
understands
> > checked-in symlinks and/or I can find a way to generate one on the fly....
> 
> After thinking about this for a while, I'm not entirely sure how to proceed.
The
> problem manifests most obviously (I can't say for sure this is the only time)
> when the realpath to a gyp file is *outside* the tree rooted at --depth. In
> particular, the build_to_base variable in ninja.py becomes bogus.
> 
> This would be simple enough to test for except that tests are expected to be
run
> from the root directory of the repo already, so simulating a file outside that
> tree is a bit difficult.

I've looked into this and I believe I understand the root of the problem.

In ninja.py, line 2270 sets:
build_file = gyp.common.RelativePath(build_file, options.toplevel_dir)

Then on line 2280, it sets:
output_file = os.path.join(obj, base_path, name + '.ninja')

The problem is that if build_file is a symlink to a file that isn't under
toplevel_dir, and RelativePath() calls realpath instead of abspath, then the
updated build_file will start with '../'. And *that* means that sometime after
output_file is computed, it will be normalized, cutting off some of the desired
directories in 'obj'.

That's bad enough----you no longer are ensured partitioning of the generated
files based on config----but it's worse than that, because the paths written
into the file assume it's in a different place than it is. (I haven't tracked
all of that back yet.)

I haven't found a way to write a test that uses a toplevel_dir that isn't the
root of the gyp repo. Instead, I've written a test that just verifies that a
call to RelativePath with a similarly symlinked argument doesn't return a path
starting with '..'. It's not ideal but it's the best I can come up with for now.

Also, the testing infrastructure was using sys.path.append(<path to pylib>) in
several places; I changed this to sys.path.insert(0, <path to pylib>) because it
was importing the installed version of gyp rather than the version from the repo
updated with my changes.

[lindleyf, 2015-08-24 16:18:13]

Ping?

[lindleyf, 2015-08-26 01:54:00]

I improved the test to actually run gyp. I accidentally figured out the key in
the previous approach: use Python's tempfile to create a file outside the git
repo. Using this, I was able to improve the test to actually be end-to-end. I've
also confirmed that without the key change from realpath->abspath in common.py,
the new test fails.

My new test is designed to pass if any of the tempfile or symlink operations
raise OSError, indicating it isn't supported on the current platform. It will
also pass if we can't read anything from a still-open temporary file, since the
python docs say this may be the case on some Windows versions.

[Nico, 2015-08-31 17:35:23]

lgtm assuming all tests pass for you (in particular, the tests that were added
in https://codereview.chromium.org/11659002/)

Some minor comments below.

Very sorry about the delay :-(

https://codereview.chromium.org/1236933002/diff/40001/gyp_main.py
File gyp_main.py (right):

https://codereview.chromium.org/1236933002/diff/40001/gyp_main.py#newcode13
gyp_main.py:13: import gyp
This looks like a good change, but maybe it should be in a separate CL?

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

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:3: # Copyright (c) 2012 Google Inc. All rights
reserved.
2015

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:34: gyp_file = tempfile.NamedTemporaryFile()
Use

with tempfile.NamedTemporaryFile() as gyp_file, tempfile.NamedTemporaryFile() as
c_file:

That way the files are cleaned up at the end of the block

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:39: gyp_file.flush()
the with block closes the file which should flush it. do you need this line?

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:42: c_file.flush()
same question

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:69: test.pass_test()  # os.symlink may not be
supported on all platforms.
Instead of this, put the test body into a

if sys.platform != 'win32':

[lindleyf, 2015-08-31 18:14:34]

On 2015/08/31 17:35:23, Nico wrote:
> lgtm assuming all tests pass for you (in particular, the tests that were added
> in https://codereview.chromium.org/11659002/)
> 
> Some minor comments below.
> 
> Very sorry about the delay :-(
> 
> https://codereview.chromium.org/1236933002/diff/40001/gyp_main.py
> File gyp_main.py (right):
> 
> https://codereview.chromium.org/1236933002/diff/40001/gyp_main.py#newcode13
> gyp_main.py:13: import gyp
> This looks like a good change, but maybe it should be in a separate CL?
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> File test/symlinks/gyptest-symlinks.py (right):
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:3: # Copyright (c) 2012 Google Inc. All
rights
> reserved.
> 2015
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:34: gyp_file = tempfile.NamedTemporaryFile()
> Use
> 
> with tempfile.NamedTemporaryFile() as gyp_file, tempfile.NamedTemporaryFile()
as
> c_file:
> 
> That way the files are cleaned up at the end of the block
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:39: gyp_file.flush()
> the with block closes the file which should flush it. do you need this line?
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:42: c_file.flush()
> same question
> 
>
https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
> test/symlinks/gyptest-symlinks.py:69: test.pass_test()  # os.symlink may not
be
> supported on all platforms.
> Instead of this, put the test body into a
> 
> if sys.platform != 'win32':

Comments addressed.

[lindleyf, 2015-08-31 18:14:58]

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

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:3: # Copyright (c) 2012 Google Inc. All rights
reserved.
On 2015/08/31 17:35:23, Nico wrote:
> 2015

Done.

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:39: gyp_file.flush()
On 2015/08/31 17:35:23, Nico wrote:
> the with block closes the file which should flush it. do you need this line?

Yes. The temporary file will be deleted when it is closed, but we want to read
it first. So we have to flush() explicitly. On some Windows versions, the python
docs say we can't read from an open temporary file....since I can't test this
directly I hedged against it with the getsize call below.

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:42: c_file.flush()
On 2015/08/31 17:35:23, Nico wrote:
> same question

Acknowledged.

[Nico, 2015-08-31 18:33:36]

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

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:69: test.pass_test()  # os.symlink may not be
supported on all platforms.
On 2015/08/31 17:35:23, Nico wrote:
> Instead of this, put the test body into a
> 
> if sys.platform != 'win32':

this wasn't addressed

[lindleyf, 2015-08-31 19:02:41]

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

https://codereview.chromium.org/1236933002/diff/40001/test/symlinks/gyptest-s...
test/symlinks/gyptest-symlinks.py:69: test.pass_test()  # os.symlink may not be
supported on all platforms.
On 2015/08/31 18:33:35, Nico wrote:
> On 2015/08/31 17:35:23, Nico wrote:
> > Instead of this, put the test body into a
> > 
> > if sys.platform != 'win32':
> 
> this wasn't addressed

Missed that, done. Note that I'm not entirely confident Windows is the only
problem point, so I'd prefer not to remove the safety net entirely.

[Nico, 2015-08-31 19:11:32]

bots will find other platforms where this might not work, but only if the
try/catch isn't there.

[lindleyf, 2015-08-31 20:03:22]

Removed except OSError block.

[lindleyf, 2015-08-31 20:17:35]

Committed patchset #8 (id:140001) manually as
3a0fcb40557e3ba02273609e49801fdb714f9ee2 (presubmit successful).

[Nico, 2015-09-01 17:00:53]

I filed https://code.google.com/p/chromium/issues/detail?id=527089 for us
silently dropping commits from people without commit bits. I'll land this for
you.

[Nico, 2015-09-01 17:39:35]

Turns out the test doesn't pass with the xcode generator as-is; it gets confused
that the gyp file is in the out dir. This makes things pass with all generators
on OS X for me:

diff --git a/test/symlinks/gyptest-symlinks.py
b/test/symlinks/gyptest-symlinks.py
index 8fca864..ed6ab74 100755
--- a/test/symlinks/gyptest-symlinks.py
+++ b/test/symlinks/gyptest-symlinks.py
@@ -61,9 +61,9 @@ if sys.platform != 'win32':
       os.symlink(c_file.name, symlink_c)
 
       # Run gyp on the symlinked files.
-      test.run_gyp(symlink_gyp)
-      test.build(symlink_gyp)
-      test.run_built_executable('symlink_hello', stdout="Hello, world!\n")
+      test.run_gyp(symlink_gyp, chdir=outdir)
+      test.build(symlink_gyp, chdir=outdir)
+      test.run_built_executable('symlink_hello', stdout="Hello, world!\n",
+                                chdir=outdir)

Is it ok if I land this with that change?

[lindleyf, 2015-09-01 17:46:51]

On 2015/09/01 17:39:35, Nico wrote:
> Turns out the test doesn't pass with the xcode generator as-is; it gets
confused
> that the gyp file is in the out dir. This makes things pass with all
generators
> on OS X for me:
> 
> diff --git a/test/symlinks/gyptest-symlinks.py
> b/test/symlinks/gyptest-symlinks.py
> index 8fca864..ed6ab74 100755
> --- a/test/symlinks/gyptest-symlinks.py
> +++ b/test/symlinks/gyptest-symlinks.py
> @@ -61,9 +61,9 @@ if sys.platform != 'win32':
>        os.symlink(c_file.name, symlink_c)
>  
>        # Run gyp on the symlinked files.
> -      test.run_gyp(symlink_gyp)
> -      test.build(symlink_gyp)
> -      test.run_built_executable('symlink_hello', stdout="Hello, world!\n")
> +      test.run_gyp(symlink_gyp, chdir=outdir)
> +      test.build(symlink_gyp, chdir=outdir)
> +      test.run_built_executable('symlink_hello', stdout="Hello, world!\n",
> +                                chdir=outdir)
> 
> Is it ok if I land this with that change?

Yeah, that looks fine. The thing we're testing shouldn't care about chdir.

[Nico, 2015-09-01 17:49:00]

Committed patchset #8 (id:140001) manually as
121d89dfcd4f6ebe1c89524b3f9ca11ddd437e77 (presubmit successful).

[kjellander_chromium, 2015-09-02 09:58:24]

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

[kjellander_chromium, 2015-09-02 09:58:25]

This breaks how WebRTC checkouts are built.
Since we use a lot of the Chromium toolchain when building WebRTC, we use
symlinks to emulate being in a Chromium checkout (we checkout chromium into our
src/chromium/src dir)

Example: 
build -> chromium/src/build
third_party/libvpx -> chromium/src/third_party/libvpx

and so on. This has been working great for us so far and we only need to roll a
single revision (chromium_revision in
https://chromium.googlesource.com/external/webrtc/+/master/DEPS) but with this
CL, the relative paths gets screwed up and GYP can no longer generate projects.

Can I ask for a revert? I'll be happy to review a new CL to ensure it doesn't
break our setup.

[lindleyf, 2015-09-02 16:49:59]

On 2015/09/02 09:58:25, kjellander (chromium) wrote:
> This breaks how WebRTC checkouts are built.
> Since we use a lot of the Chromium toolchain when building WebRTC, we use
> symlinks to emulate being in a Chromium checkout (we checkout chromium into
our
> src/chromium/src dir)
> 
> Example: 
> build -> chromium/src/build
> third_party/libvpx -> chromium/src/third_party/libvpx
> 
> and so on. This has been working great for us so far and we only need to roll
a
> single revision (chromium_revision in
> https://chromium.googlesource.com/external/webrtc/+/master/DEPS) but with this
> CL, the relative paths gets screwed up and GYP can no longer generate
projects.
> 
> Can I ask for a revert? I'll be happy to review a new CL to ensure it doesn't
> break our setup.

Apparently this has some problems with several IOS projects too, so I'm going to
be looking at it today to see if I can pin down the problem. Since this change
ought to be making gyp more symlink-friendly, not less, I suspect it's as simple
as needing to change a few more instances of realpath->abspath for consistency.
If you could give me instructions on how to repro your error, it would help.

[bradn, 2015-09-02 16:51:19]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2015-09-02 16:51:20]

This is also causing a problem for an alternate v8 configuration using symlinks.

[Nico, 2015-09-02 16:53:11]

let's revert and reland once things are figured out then

[titzer, 2015-09-03 08:01:02]

On 2015/09/02 16:53:11, Nico (vacation Thu Sep 2) wrote:
> let's revert and reland once things are figured out then

This also broke the wasm v8-native-prototype build, which uses symlinks.

[kjellander_chromium, 2015-09-04 07:38:48]

On 2015/09/03 08:01:02, titzer wrote:
> On 2015/09/02 16:53:11, Nico (vacation Thu Sep 2) wrote:
> > let's revert and reland once things are figured out then
> 
> This also broke the wasm v8-native-prototype build, which uses symlinks.

I still don't see any revert, lindleyf can you take care of this?

[lindleyf, 2015-09-04 07:55:48]

On 2015/09/04 07:38:48, kjellander (chromium) wrote:
> On 2015/09/03 08:01:02, titzer wrote:
> > On 2015/09/02 16:53:11, Nico (vacation Thu Sep 2) wrote:
> > > let's revert and reland once things are figured out then
> > 
> > This also broke the wasm v8-native-prototype build, which uses symlinks.
> 
> I still don't see any revert, lindleyf can you take care of this?

I don't think I have permission to do a revert myself. thakis@?

[bradn, 2015-09-04 19:49:29]

Sent out a revert here:
https://codereview.chromium.org/1315373003/