[Mac/GN] Treat create_bundle as a leaf for `gn desc runtime_deps`.

[Mac/GN] Treat create_bundle as a leaf for `gn desc runtime_deps`.

BUG=611414
R=brettw@chromium.org,dpranke@chromium.org,sdefresne@chromium.org

Committed: https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39
Cr-Commit-Position: refs/heads/master@{#394840}

[Robert Sesek, 2016-05-13 21:42:14]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2016-05-13 22:16:14]

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

https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
tools/gn/runtime_deps.cc:111:
target->bundle_data().GetBundleRootDirOutput(target->settings());
Do you know if this gets reflected as "out/Release/Google Chrome.app/" in the
output? The trailing slash is important for isolates ...

[Robert Sesek, 2016-05-13 22:52:27]

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

https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
tools/gn/runtime_deps.cc:111:
target->bundle_data().GetBundleRootDirOutput(target->settings());
On 2016/05/13 22:16:14, Dirk Pranke wrote:
> Do you know if this gets reflected as "out/Release/Google Chrome.app/" in the
> output? The trailing slash is important for isolates ...

No, it doesn't end with a slash because BundleData::GetBundleRootDirOutput
returns a SourceFile and so it cannot. I could add one manually here?

[Dirk Pranke, 2016-05-13 23:37:38]

On 2016/05/13 22:52:27, Robert Sesek wrote:
> https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.cc
> File tools/gn/runtime_deps.cc (right):
> 
>
https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
> tools/gn/runtime_deps.cc:111:
> target->bundle_data().GetBundleRootDirOutput(target->settings());
> On 2016/05/13 22:16:14, Dirk Pranke wrote:
> > Do you know if this gets reflected as "out/Release/Google Chrome.app/" in
the
> > output? The trailing slash is important for isolates ...
> 
> No, it doesn't end with a slash because BundleData::GetBundleRootDirOutput
> returns a SourceFile and so it cannot. I could add one manually here?

Yeah, that's what I figured. I think you need to add the slash and hopefully
that doesn't mess up anything else.

[Dirk Pranke, 2016-05-14 02:54:22]

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

https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
tools/gn/runtime_deps.cc:111:
target->bundle_data().GetBundleRootDirOutput(target->settings());
On 2016/05/13 22:52:26, Robert Sesek wrote:
> On 2016/05/13 22:16:14, Dirk Pranke wrote:
> > Do you know if this gets reflected as "out/Release/Google Chrome.app/" in
the
> > output? The trailing slash is important for isolates ...
> 
> No, it doesn't end with a slash because BundleData::GetBundleRootDirOutput
> returns a SourceFile and so it cannot. I could add one manually here?

Yup, I think it will need to be either a SourceDir or an OutputFile to work
properly. Perhaps we
should modify BundleData to also have a GetBundleRootDirOutputAsDir() or
something .

sdefresne@ probably has thoughts here, too.

[sdefresne, 2016-05-17 08:43:29]

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

https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
tools/gn/runtime_deps.cc:111:
target->bundle_data().GetBundleRootDirOutput(target->settings());
On 2016/05/14 02:54:22, Dirk Pranke wrote:
> On 2016/05/13 22:52:26, Robert Sesek wrote:
> > On 2016/05/13 22:16:14, Dirk Pranke wrote:
> > > Do you know if this gets reflected as "out/Release/Google Chrome.app/" in
> the
> > > output? The trailing slash is important for isolates ...
> > 
> > No, it doesn't end with a slash because BundleData::GetBundleRootDirOutput
> > returns a SourceFile and so it cannot. I could add one manually here?
> 
> Yup, I think it will need to be either a SourceDir or an OutputFile to work
> properly. Perhaps we
> should modify BundleData to also have a GetBundleRootDirOutputAsDir() or
> something .
> 
> sdefresne@ probably has thoughts here, too. 

I'd personally add a GetBundleRootDirOutputAsDir() with an explanatory comment.
It will in my opinion cleaner than adding a trailing slash here.

[Robert Sesek, 2016-05-17 18:32:04]

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

https://codereview.chromium.org/1982463002/diff/20001/tools/gn/runtime_deps.c...
tools/gn/runtime_deps.cc:111:
target->bundle_data().GetBundleRootDirOutput(target->settings());
On 2016/05/17 08:43:28, sdefresne wrote:
> On 2016/05/14 02:54:22, Dirk Pranke wrote:
> > On 2016/05/13 22:52:26, Robert Sesek wrote:
> > > On 2016/05/13 22:16:14, Dirk Pranke wrote:
> > > > Do you know if this gets reflected as "out/Release/Google Chrome.app/"
in
> > the
> > > > output? The trailing slash is important for isolates ...
> > > 
> > > No, it doesn't end with a slash because BundleData::GetBundleRootDirOutput
> > > returns a SourceFile and so it cannot. I could add one manually here?
> > 
> > Yup, I think it will need to be either a SourceDir or an OutputFile to work
> > properly. Perhaps we
> > should modify BundleData to also have a GetBundleRootDirOutputAsDir() or
> > something .
> > 
> > sdefresne@ probably has thoughts here, too. 
> 
> I'd personally add a GetBundleRootDirOutputAsDir() with an explanatory
comment.
> It will in my opinion cleaner than adding a trailing slash here.

That's a good idea. Done.

[sdefresne, 2016-05-17 18:45:36]

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc
File tools/gn/bundle_data.cc (right):

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:135: return
SourceDir(GetBundleRootDirOutput(settings).value());
I think you need to add the trailing "/" when calling SourceDir() otherwise
you'll get a DCHECK in debug (but no error in release).

[Robert Sesek, 2016-05-17 18:49:34]

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc
File tools/gn/bundle_data.cc (right):

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:135: return
SourceDir(GetBundleRootDirOutput(settings).value());
On 2016/05/17 18:45:36, sdefresne wrote:
> I think you need to add the trailing "/" when calling SourceDir() otherwise
> you'll get a DCHECK in debug (but no error in release).

SourceFile DCHECKs if there is a slash, but SourceDir adds one if it doesn't
exist:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gn/source_di...

[sdefresne, 2016-05-18 08:08:41]

lgtm

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc
File tools/gn/bundle_data.cc (right):

https://codereview.chromium.org/1982463002/diff/40001/tools/gn/bundle_data.cc...
tools/gn/bundle_data.cc:135: return
SourceDir(GetBundleRootDirOutput(settings).value());
On 2016/05/17 18:49:34, Robert Sesek wrote:
> On 2016/05/17 18:45:36, sdefresne wrote:
> > I think you need to add the trailing "/" when calling SourceDir() otherwise
> > you'll get a DCHECK in debug (but no error in release).
> 
> SourceFile DCHECKs if there is a slash, but SourceDir adds one if it doesn't
> exist:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/gn/source_di...

Ah good.

[Robert Sesek, 2016-05-19 14:56:15]

dpranke: Thoughts, or should we wait for brettw?

[Dirk Pranke, 2016-05-19 18:43:23]

lgtm. I don't think we need to wait for brettw@ for this.

[Dirk Pranke, 2016-05-19 18:43:35]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-05-19 18:44:19]

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

[commit-bot: I haz the power, 2016-05-19 19:43:43]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-05-19 19:46:18]

Description was changed from

==========
[Mac/GN] Treat create_bundle as a leaf for `gn desc runtime_deps`.

BUG=611414
R=brettw@chromium.org,dpranke@chromium.org,sdefresne@chromium.org
==========

to

==========
[Mac/GN] Treat create_bundle as a leaf for `gn desc runtime_deps`.

BUG=611414
R=brettw@chromium.org,dpranke@chromium.org,sdefresne@chromium.org

Committed: https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39
Cr-Commit-Position: refs/heads/master@{#394840}
==========

[commit-bot: I haz the power, 2016-05-19 19:46:19]

Patchset 2 (id:??) landed as
https://crrev.com/903e2b85c71675f83767ac9b1687087382b49f39
Cr-Commit-Position: refs/heads/master@{#394840}