gn: Support build directories outside the source tree.

gn: Support build directories outside the source tree.

The main use-case is to support using a fast SSD or ramdisk for the
build directory, but a slower persistent disk for the source code.

The general idea behind this change is modifying

 - RebaseSourceAbsolutePath()
 - SourceDir::ResolveRelativeFile()
 - SourceDir::ResolveRelativeDir()
 - GetOutputDirForSourceDirAsOutputFile()
 - PathOutput

to work with paths reaching out of the source directory.

Thanks to jam@ for the Windows-fixes.

BUG=343728
TEST=New unit tests + Unit tests pass.
TEST=`gn gen /ssd/out/Debug && ninja -C /ssd/out/Debug` work as expected.

Committed: https://crrev.com/6f9c6456a4358ec6ccb7b1c9d4e64a796633faae
Cr-Commit-Position: refs/heads/master@{#303719}

[zeuthen, 2014-10-06 21:33:15]

zeuthen@chromium.org changed reviewers:
+ brettw@chromium.org

[zeuthen, 2014-10-06 21:34:06]

Hey Brett,

Here's a CL to make gn support build directories outside the source tree. Please
let me know what you think. Thanks, David

[zeuthen, 2014-10-06 21:43:09]

Btw, I should mention that the general idea here involves making the following
functions/methods

 - RebaseSourceAbsolutePath()
 - SourceDir::ResolveRelativeFile()
 - SourceDir::ResolveRelativeDir()
 - GetOutputDirForSourceDirAsOutputFile()

work with paths reaching out of the source directory. To do this, the absolute
path of the source directory needs to be passed and a big chunk of the CL is
about passing this down to appropriate call-sites.

(I'll update the CL description later to include this.)

[brettw, 2014-10-07 17:51:29]

It's not clear to me why you needed to change the label resolution. You plumbed
through the current source dir so that absolution paths that reference files
inside the source tree get canonicalized to the source-dir-relative format. But
why do we need this? I would have expected that only changes in output file
processing would be necessary for this change.

I did a test and this patch regresses performance from 758ms to 818ms on my
computer. I presume this extra overhead for each label is the culprit.

[brettw, 2014-10-07 17:51:36]

https://codereview.chromium.org/630223002/diff/1/tools/gn/filesystem_utils.cc
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/1/tools/gn/filesystem_utils.cc...
tools/gn/filesystem_utils.cc:579: ret.pop_back();
I get a compiler error that this function doesn't exist.

[zeuthen, 2014-10-07 18:14:38]

https://codereview.chromium.org/630223002/diff/1/tools/gn/filesystem_utils.cc
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/1/tools/gn/filesystem_utils.cc...
tools/gn/filesystem_utils.cc:579: ret.pop_back();
On 2014/10/07 17:51:36, brettw wrote:
> I get a compiler error that this function doesn't exist.

It's a C++-11-ism, sorry. I'll change it.

[zeuthen, 2014-10-07 18:17:29]

On 2014/10/07 17:51:29, brettw wrote:
> It's not clear to me why you needed to change the label resolution. You
plumbed
> through the current source dir so that absolution paths that reference files
> inside the source tree get canonicalized to the source-dir-relative format.
But
> why do we need this? I would have expected that only changes in output file
> processing would be necessary for this change.

It's not unlikely we don't need to change anything for labels. By way of
explanation, I only did changes to Label because it ended up using
RebaseSourceAbsolutePath() - I'll try to delete it and see if things work. (I
also had a lot of other things in my first working CL that I since deleted).

> I did a test and this patch regresses performance from 758ms to 818ms on my
> computer. I presume this extra overhead for each label is the culprit.

I see.

[zeuthen, 2014-10-07 21:18:27]

Patch set 2 has the requested changes - please take another look! Thanks!

[brettw, 2014-10-07 23:55:06]

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.cc:562: input_full =
source_root.Append(input.substr(2)).value();
I tracked the performance regression to this function, which is called a lot (I
locally substitutied the old implementation). I notice there are kind of a lot
of string allocations and copies (just this line is three). MakeRelativePath at
the bottom is also kind of slow. We should optimize this.

Also, this won't work at all on Windows. The FilePath temporary will make
backslashes which I think will confuse things. And you can't append the
wchar_t's and chars the way you're doing on this line.

I think it might be better to keep the source root either a StringPiece or an
absolute SourceDir.

https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc
File tools/gn/source_dir.cc (right):

https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc#n...
tools/gn/source_dir.cc:73: if (!source_root.empty()) {
Are these blocks necessary? Are you trying to handle the case where you're doing
a relative path to something outside the source root? Do we care about that
case? It seems like a separate issue than the one you are solving.

There are also a lot of string copies here (this function is called a lot) and
it won't compile on Windows.

Same for the one in the Dir case below.

[zeuthen, 2014-10-08 15:21:06]

Hey, thanks for the review - replies inline!

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.cc:562: input_full =
source_root.Append(input.substr(2)).value();
Fair enough, I'll try to make this go faster / fewer copies etc. (Just wrapping
my head around the code and getting it to do the right thing was a lot of
work... so I didn't pay much attention to performance.)

https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc
File tools/gn/source_dir.cc (right):

https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc#n...
tools/gn/source_dir.cc:73: if (!source_root.empty()) {
I tried commenting this out and without it, the build fails with

 zeuthen@luna:/ssd/chromium/src/tools/gn$ ../../out/Debug/gn gen
/ssd/midlertidigt/chrome_build/out/Debug/
 ERROR at //tools/grit/grit_rule.gni:254:18: File is not inside output
directory.
   grit_outputs = get_path_info(
                  ^-------------
 The given file should be in the output directory. Normally you would specify
 "$target_out_dir/foo" or "$target_gen_dir/foo". I interpreted this as
 "//midlertidigt/chrome_build/out/Debug/gen/content/grit/content_resources.h".
 See //content/BUILD.gn:66:1: whence it was called.
 grit("resources") {

I think there's a way to optimize this so we don't have to call Resolve() -
basically, we're good until we call NormalizePath(): the culprit is that we're
trying to normalize a path of the form
//../../../ssd/midlertidigt/chrome_build/out/Debug which we'll erroneously
normalize to //midlertidigt/chrome_build/out/Debug/ ... so perhaps all we need
is to teach NormalizePath() about being smart and only do expensive stuff if we
end up going outside the source root.

[brettw, 2014-10-08 19:28:38]

>
https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc#n...
> tools/gn/source_dir.cc:73: if (!source_root.empty()) {
> I tried commenting this out and without it, the build fails with
> 
>  zeuthen@luna:/ssd/chromium/src/tools/gn$ ../../out/Debug/gn gen
> /ssd/midlertidigt/chrome_build/out/Debug/
>  ERROR at //tools/grit/grit_rule.gni:254:18: File is not inside output
> directory.
>    grit_outputs = get_path_info(
>                   ^-------------
>  The given file should be in the output directory. Normally you would specify
>  "$target_out_dir/foo" or "$target_gen_dir/foo". I interpreted this as
>  "//midlertidigt/chrome_build/out/Debug/gen/content/grit/content_resources.h".
>  See //content/BUILD.gn:66:1: whence it was called.
>  grit("resources") {
> 
> I think there's a way to optimize this so we don't have to call Resolve() -
> basically, we're good until we call NormalizePath(): the culprit is that we're
> trying to normalize a path of the form
> //../../../ssd/midlertidigt/chrome_build/out/Debug

I *think* we should try to avoid generating this path in the first place. I
believe this is probably being generated by the call in grit_rule.gni:

grit_outputs = get_path_info(
      rebase_path(invoker.outputs, ".", output_dir),
      "abspath")

But this is really the wrong output for this function. When you ask for
"abspath" for an absolute input, it should be a NOP. If the file is not in the
source tree (i.e. it doesn't have a strict prefix of the source root),
MakePathRelativeIfPossible should probably just keep it absolute (my guess is
that it's trying too hard in this case). Does this make sense if you look at it
in the debugger?

[zeuthen, 2014-10-08 21:05:42]

On 2014/10/08 19:28:38, brettw wrote:
> >
>
https://codereview.chromium.org/630223002/diff/20001/tools/gn/source_dir.cc#n...
> > tools/gn/source_dir.cc:73: if (!source_root.empty()) {
> > I tried commenting this out and without it, the build fails with
> > 
> >  zeuthen@luna:/ssd/chromium/src/tools/gn$ ../../out/Debug/gn gen
> > /ssd/midlertidigt/chrome_build/out/Debug/
> >  ERROR at //tools/grit/grit_rule.gni:254:18: File is not inside output
> > directory.
> >    grit_outputs = get_path_info(
> >                   ^-------------
> >  The given file should be in the output directory. Normally you would
specify
> >  "$target_out_dir/foo" or "$target_gen_dir/foo". I interpreted this as
> > 
"//midlertidigt/chrome_build/out/Debug/gen/content/grit/content_resources.h".
> >  See //content/BUILD.gn:66:1: whence it was called.
> >  grit("resources") {
> > 
> > I think there's a way to optimize this so we don't have to call Resolve() -
> > basically, we're good until we call NormalizePath(): the culprit is that
we're
> > trying to normalize a path of the form
> > //../../../ssd/midlertidigt/chrome_build/out/Debug
> 
> I *think* we should try to avoid generating this path in the first place. I
> believe this is probably being generated by the call in grit_rule.gni:
> 
> grit_outputs = get_path_info(
>       rebase_path(invoker.outputs, ".", output_dir),
>       "abspath")
> 
> But this is really the wrong output for this function. When you ask for
> "abspath" for an absolute input, it should be a NOP. If the file is not in the
> source tree (i.e. it doesn't have a strict prefix of the source root),
> MakePathRelativeIfPossible should probably just keep it absolute (my guess is
> that it's trying too hard in this case). Does this make sense if you look at
it
> in the debugger?

I'll take a look!

[zeuthen, 2014-10-09 21:49:32]

On 2014/10/08 21:05:42, zeuthen wrote:
> > But this is really the wrong output for this function. When you ask for
> > "abspath" for an absolute input, it should be a NOP. If the file is not in
the
> > source tree (i.e. it doesn't have a strict prefix of the source root),
> > MakePathRelativeIfPossible should probably just keep it absolute (my guess
is
> > that it's trying too hard in this case). Does this make sense if you look at
> it
> > in the debugger?
> 
> I'll take a look!

I just checked - with the Chrome build, MakePathRelativeIfPossible() is never
passed any paths with "../" in them and thus its prefix check (on Linux, all I'm
testing against right now) works just fine. I'll dig a bit deeper.

[zeuthen, 2014-11-04 19:49:16]

Sorry for the delay, been traveling etc. the past few weeks. New CL coming up.

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/20001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.cc:562: input_full =
source_root.Append(input.substr(2)).value();
Upcoming CL makes this part more efficient.

[zeuthen, 2014-11-04 19:53:24]

Main delta from PS2->PS3 is that RebaseSourceAbsolutePath() is more optimized
plus we're using base::StringPiece for passing the source_root around.

[brettw, 2014-11-05 19:59:49]

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.cc:552: std::string RebaseSourceAbsolutePath(const
std::string& input,
Wait, doesn't this function handle all types of paths now? Can we just call it
RebasePath?

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_utils.h
File tools/gn/filesystem_utils.h (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.h:130: // If supplied, the |source_root| parameter
must be the absolute path
"must be" -> "is the". Can you explain here what happens if this is supplied?
The current comment gives no reason why you would bother specifying this.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/function_rebase...
File tools/gn/function_rebase_path.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/function_rebase...
tools/gn/function_rebase_path.cc:90: from_dir.ResolveRelativeDir(string_value,
Can you add a quick test for these file and dir cases? I realize this should
basically be covered by the other unit test you wrote, but there are a lot of
tests for this rebase function, and it would be nice to verify that everything
is hooked up properly.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/item.cc
File tools/gn/item.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/item.cc#newcode6
tools/gn/item.cc:6: #include "tools/gn/item.h"
This should be first with a blank line after it since it corresponds to the
current .cc file.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/label_pattern_u...
File tools/gn/label_pattern_unittest.cc (left):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/label_pattern_u...
tools/gn/label_pattern_unittest.cc:65: 
Revert this file.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/path_output.cc
File tools/gn/path_output.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/path_output.cc#...
tools/gn/path_output.cc:19: if (!EndsWithSlash(inverse_current_dir_))
I would hope that the above operation is always the same with regard to the
output ending in a slash or not. Can we remove this and do an assertion instead?
Or is there a complicated case it can't tell?

[brettw, 2014-11-05 20:00:12]

BTW I benchmarked this and it's exactly the same speed. Nice!

[zeuthen, 2014-11-07 19:24:12]

Hey, thanks for the review. New CL coming up.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
File tools/gn/filesystem_utils.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.cc:552: std::string RebaseSourceAbsolutePath(const
std::string& input,
On 2014/11/05 19:59:49, brettw wrote:
> Wait, doesn't this function handle all types of paths now? Can we just call it
> RebasePath?

That's a good point. I'll rename it.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_utils.h
File tools/gn/filesystem_utils.h (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/filesystem_util...
tools/gn/filesystem_utils.h:130: // If supplied, the |source_root| parameter
must be the absolute path
On 2014/11/05 19:59:49, brettw wrote:
> "must be" -> "is the". Can you explain here what happens if this is supplied?
> The current comment gives no reason why you would bother specifying this.

Done.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/function_rebase...
File tools/gn/function_rebase_path.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/function_rebase...
tools/gn/function_rebase_path.cc:90: from_dir.ResolveRelativeDir(string_value,
On 2014/11/05 19:59:49, brettw wrote:
> Can you add a quick test for these file and dir cases? I realize this should
> basically be covered by the other unit test you wrote, but there are a lot of
> tests for this rebase function, and it would be nice to verify that everything
> is hooked up properly.

OK, sure thing. Done.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/item.cc
File tools/gn/item.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/item.cc#newcode6
tools/gn/item.cc:6: #include "tools/gn/item.h"
On 2014/11/05 19:59:49, brettw wrote:
> This should be first with a blank line after it since it corresponds to the
> current .cc file.

Done.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/label_pattern_u...
File tools/gn/label_pattern_unittest.cc (left):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/label_pattern_u...
tools/gn/label_pattern_unittest.cc:65: 
On 2014/11/05 19:59:49, brettw wrote:
> Revert this file.

Done.

https://codereview.chromium.org/630223002/diff/50001/tools/gn/path_output.cc
File tools/gn/path_output.cc (right):

https://codereview.chromium.org/630223002/diff/50001/tools/gn/path_output.cc#...
tools/gn/path_output.cc:19: if (!EndsWithSlash(inverse_current_dir_))
On 2014/11/05 19:59:49, brettw wrote:
> I would hope that the above operation is always the same with regard to the
> output ending in a slash or not. Can we remove this and do an assertion
instead?
> Or is there a complicated case it can't tell?

I tried adding a DHCECK() and it fails is in the PathOutput.BasicInRoot test
case:

  SourceDir build_dir("//");
  base::StringPiece source_root("/source/root");
  PathOutput writer(build_dir, source_root, ESCAPE_NONE);

where inverse_current_dir_ ends up being a ".".

[brettw, 2014-11-07 20:33:50]

LGTM, thanks!

[zeuthen, 2014-11-07 20:39:31]

The CQ bit was checked by zeuthen@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:41:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/630223002/70001

[commit-bot: I haz the power, 2014-11-07 21:19:15]

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

[commit-bot: I haz the power, 2014-11-07 21:19:16]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[zeuthen, 2014-11-07 22:14:32]

I guess I don't have trybot access (been looking at
http://www.chromium.org/developers/testing/try-server-usage )... any chance one
of you guys can kick off a Windows trybot against the latest patch set? Thanks!

[brettw, 2014-11-07 22:15:48]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-11-07 22:16:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/630223002/90001

[zeuthen, 2014-11-07 22:35:55]

More failures on Windows. I'm going to try and find a Windows box to compile and
test the CL on.

[commit-bot: I haz the power, 2014-11-07 23:04:11]

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

[commit-bot: I haz the power, 2014-11-07 23:04:14]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jam, 2014-11-11 03:05:25]

On 2014/11/07 22:35:55, zeuthen wrote:
> More failures on Windows. I'm going to try and find a Windows box to compile
and
> test the CL on.

Here's the win build fix: https://codereview.chromium.org/711113004

Brett pointed me at this cl, since once it lands I can do something I need (copy
a file from an absolute dir)

[zeuthen, 2014-11-11 17:06:03]

The CQ bit was checked by zeuthen@chromium.org

[commit-bot: I haz the power, 2014-11-11 17:07:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/630223002/110001

[zeuthen, 2014-11-11 17:36:50]

On 2014/11/11 17:07:24, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/630223002/110001

Failed on win8_chromium_rel compiling the unit tests - I'm on it! (Just
installed Win7 yesterday in a VM.)

[commit-bot: I haz the power, 2014-11-11 17:44:12]

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

[commit-bot: I haz the power, 2014-11-11 17:44:13]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[zeuthen, 2014-11-11 20:07:35]

The CQ bit was checked by zeuthen@chromium.org

[commit-bot: I haz the power, 2014-11-11 20:09:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/630223002/130001

[commit-bot: I haz the power, 2014-11-11 21:01:28]

Committed patchset #7 (id:130001)

[commit-bot: I haz the power, 2014-11-11 21:02:09]

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