[GN] Add support to rebase_path to resolve paths above the source root.

[GN] Add support to rebase_path to resolve paths above the source root.

Currently, rebase_path clips any path which references the filesystem
above or outside of the source root. Extend this functionality to allow
rebase_path to properly resolve paths outside the source root when the
absolute path to the source root is known. When the source root is
unknown, the behavior will continue to clip.

BUG=554241
TEST=gn_unittests

Committed: https://crrev.com/910ac443ab1827eae049ae052af867d432ad7aac
Cr-Commit-Position: refs/heads/master@{#363080}

[slan, 2015-11-18 18:03:01]

slan@chromium.org changed reviewers:
+ alokp@chromium.org, brettw@chromium.org, dpranke@chromium.org

[slan, 2015-11-18 18:03:01]

All,

Here is the first pass at adding this functionality. It simply involves
optionally passing information about the source root to NormalizePath().

One initial concern: I performed a quick-and-dirty perf test of the gn binary
with my change. It adds about 5% increase in time (with a Release build, no
optimizations). I suppose this could be due to the couple of copies that I am
adding, or the flags might not be tuned right. I will look into improving this
today.

PTAL

[brettw, 2015-11-20 22:16:40]

On 2015/11/18 18:03:01, slan wrote:
> All,
> 
> Here is the first pass at adding this functionality. It simply involves
> optionally passing information about the source root to NormalizePath().
> 
> One initial concern: I performed a quick-and-dirty perf test of the gn binary
> with my change. It adds about 5% increase in time (with a Release build, no
> optimizations). I suppose this could be due to the couple of copies that I am
> adding, or the flags might not be tuned right. I will look into improving this
> today.
> 
> PTAL

Yeah, I think this is too heavyweight. You're forcing the common case down a
always prepend and then strip at the end scheme.

Instead, I think a change along these lines should affect behavior only if the
path actually does this behavior. I think this means it uses ".." to go above
"//". There is a condition in the function that handles this case, can you
insert the new logic there?

[slan, 2015-11-22 00:59:12]

On 2015/11/20 22:16:40, brettw wrote:
> On 2015/11/18 18:03:01, slan wrote:
> > All,
> > 
> > Here is the first pass at adding this functionality. It simply involves
> > optionally passing information about the source root to NormalizePath().
> > 
> > One initial concern: I performed a quick-and-dirty perf test of the gn
binary
> > with my change. It adds about 5% increase in time (with a Release build, no
> > optimizations). I suppose this could be due to the couple of copies that I
am
> > adding, or the flags might not be tuned right. I will look into improving
this
> > today.
> > 
> > PTAL
> 
> Yeah, I think this is too heavyweight. You're forcing the common case down a
> always prepend and then strip at the end scheme.
> 
> Instead, I think a change along these lines should affect behavior only if the
> path actually does this behavior. I think this means it uses ".." to go above
> "//". There is a condition in the function that handles this case, can you
> insert the new logic there?

Agreed. The latest patch only modifies the path if needed. There is no
measurable performance decrease, as expected. PTAL.

[slan, 2015-11-30 14:49:47]

Friendly ping for feedback.

[brettw, 2015-12-01 01:21:22]

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
tools/gn/filesystem_utils.cc:439: const size_t len = source_root.ends_with("/")
I think we can know whether the source_root ends in a slash. I think we can do a
DCHECK and then hard code this (I think SourceDir where you call this always
expects a path a certain way).

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
File tools/gn/filesystem_utils_unittest.cc (right):

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
tools/gn/filesystem_utils_unittest.cc:291: }
Can you add a test to SourceDir that checks this is getting called correctly? It
doesn't have to handle the edge cases, those are tested here, just that the new
code that hooks up the source dir does the right thing.

Here, we should add some tests for source root of "/", and some Windows cases.

I actually think your new code doesn't work on Windows. I think the source_root
comes from BuildSetting.root_path_utf8() which is in the system native format.
You can see the RebasePath implementation has some special handling for this.
The way this is used, you will want to produce output that looks like "/C:/foo"
in this case (GN makes Windows paths start with a slash in SourceDir/SourceFile
for uniformity).

This is weird because NormalizePath previously wasn't aware of any of these
special Windows rules because it can handle both absolute and relative input and
it all kind of worked before. I may have to think about the best way to handle
this, it's possible the root path we pass in should already be GN-formatted.

Note: It looks like NormalizePath doesn't check that ".." doesn't back up over
"C:" and will make things that start looking like Unix paths with no driver
letter at that point. But I think this is OK and you can do the same thing with
the new code.

This is a bit weird. You changed RebasePath tests and it produces paths like
"C:/foo", but that's due to the special case of having an empty "new_base" which
will produce something that looks like a native path with no initial slash. This
is designed so that if the GN script is producing absolute paths to send to an
external program, that they make sense to that program.

[brettw, 2015-12-01 17:46:43]

I'm guessing it's best to have NormalizePath fix this when it prepends the
source root on Windows to also prepend a slash, then document in the header what
format the source root should be in.

[slan, 2015-12-01 22:15:03]

Brett, thanks for the quick and thorough review. My latest PS accounts for many
of the Windows cases you mentioned. I have not done much development for Windows
in general, so thanks for helping me find edge cases.

Note - I am not set up to test Windows locally. We will have to wait for the
trybot to verify. I will make sure to dryrun.

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
tools/gn/filesystem_utils.cc:439: const size_t len = source_root.ends_with("/")
On 2015/12/01 01:21:22, brettw wrote:
> I think we can know whether the source_root ends in a slash. I think we can do
a
> DCHECK and then hard code this (I think SourceDir where you call this always
> expects a path a certain way).

Done.

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
File tools/gn/filesystem_utils_unittest.cc (right):

https://codereview.chromium.org/1455203002/diff/80001/tools/gn/filesystem_uti...
tools/gn/filesystem_utils_unittest.cc:291: }
On 2015/12/01 01:21:22, brettw wrote:
> Can you add a test to SourceDir that checks this is getting called correctly?
It
> doesn't have to handle the edge cases, those are tested here, just that the
new
> code that hooks up the source dir does the right thing.

Done.

> 
> Here, we should add some tests for source root of "/", and some Windows cases.

Done and done. 

> 
> I actually think your new code doesn't work on Windows. I think the
source_root
> comes from BuildSetting.root_path_utf8() which is in the system native format.
> You can see the RebasePath implementation has some special handling for this.
> The way this is used, you will want to produce output that looks like
"/C:/foo"
> in this case (GN makes Windows paths start with a slash in
SourceDir/SourceFile
> for uniformity).

The code now accepts "/C:/foo" or "C:/foo" for the source_root, always returning
the former. Comments updated to reflect this.

> 
> This is weird because NormalizePath previously wasn't aware of any of these
> special Windows rules because it can handle both absolute and relative input
and
> it all kind of worked before. I may have to think about the best way to handle
> this, it's possible the root path we pass in should already be GN-formatted.
> 
> Note: It looks like NormalizePath doesn't check that ".." doesn't back up over
> "C:" and will make things that start looking like Unix paths with no driver
> letter at that point. But I think this is OK and you can do the same thing
with
> the new code.

Right, once it backs up over the root, all bets are off IMO.

> 
> This is a bit weird. You changed RebasePath tests and it produces paths like
> "C:/foo", but that's due to the special case of having an empty "new_base"
which
> will produce something that looks like a native path with no initial slash.
This
> is designed so that if the GN script is producing absolute paths to send to an
> external program, that they make sense to that program.

Not sure what you mean. The RebasePath tests continue to return absolute paths
in the same manner as before this CL. Would you like that behavior to change?

[brettw, 2015-12-03 19:22:04]

lgtm

[slan, 2015-12-03 19:23:18]

The CQ bit was checked by slan@chromium.org to run a CQ dry run

[slan, 2015-12-03 19:24:11]

Dry run to test Windows. Will try to submit to CQ this afternoon, pending no
more feedback.

[commit-bot: I haz the power, 2015-12-03 19:24:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455203002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455203002/100001

[commit-bot: I haz the power, 2015-12-03 20:45:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-03 20:45:09]

Dry run: This issue passed the CQ dry run.

[slan, 2015-12-03 23:29:25]

The CQ bit was checked by slan@chromium.org

[commit-bot: I haz the power, 2015-12-03 23:31:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1455203002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1455203002/100001

[commit-bot: I haz the power, 2015-12-04 01:40:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-04 01:42:32]

Description was changed from

==========
[GN] Add support to rebase_path to resolve paths above the source root.

Currently, rebase_path clips any path which references the filesystem
above or outside of the source root. Extend this functionality to allow
rebase_path to properly resolve paths outside the source root when the
absolute path to the source root is known. When the source root is
unknown, the behavior will continue to clip.

BUG=554241
TEST=gn_unittests
==========

to

==========
[GN] Add support to rebase_path to resolve paths above the source root.

Currently, rebase_path clips any path which references the filesystem
above or outside of the source root. Extend this functionality to allow
rebase_path to properly resolve paths outside the source root when the
absolute path to the source root is known. When the source root is
unknown, the behavior will continue to clip.

BUG=554241
TEST=gn_unittests

Committed: https://crrev.com/910ac443ab1827eae049ae052af867d432ad7aac
Cr-Commit-Position: refs/heads/master@{#363080}
==========

[commit-bot: I haz the power, 2015-12-04 01:42:33]

Patchset 6 (id:??) landed as
https://crrev.com/910ac443ab1827eae049ae052af867d432ad7aac
Cr-Commit-Position: refs/heads/master@{#363080}