[Mac/GN] Start running components_unittests, components_browsertests, and unit_tests.

[Mac/GN] Start running components_unittests, components_browsertests, and unit_tests.

BUG=431177
R=dpranke@chromium.org

Committed: https://crrev.com/40f9bc31a00d01256da2f6819e4f3f30518b3261
Cr-Commit-Position: refs/heads/master@{#392362}

[tandrii(chromium), 2016-05-05 10:38:31]

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

[tandrii(chromium), 2016-05-05 10:38:32]

for hung isolate, filed https://codereview.chromium.org/1944943003

[Robert Sesek, 2016-05-05 18:50:10]

Patchset #4 (id:60001) has been deleted

[Robert Sesek, 2016-05-05 20:38:18]

Patchset #4 (id:80001) has been deleted

[Robert Sesek, 2016-05-05 21:22:37]

dpranke: PTAL now

[Dirk Pranke, 2016-05-05 22:59:15]

mostly okay, but with one question ...

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:328: data += [ "$root_out_dir/resources.pak" ]
Really I think these should be data_deps += [ "//chrome:packed_extra_resources"
] instead, to get the dependencies right, but we can fix that later.

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
Is this really deps (i.e., components_browsertests won't link w/o it), or should
it be data_deps, in which case you don't need to also specify data, I don't
think (unless maybe things are confused by this being a directory?)

[Robert Sesek, 2016-05-06 20:52:25]

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:328: data += [ "$root_out_dir/resources.pak" ]
On 2016/05/05 22:59:15, Dirk Pranke wrote:
> Really I think these should be data_deps += [
"//chrome:packed_extra_resources"
> ] instead, to get the dependencies right, but we can fix that later.

Agree that this makes more sense as data_deps[] but that doesn't seem to be
standard practice yet.

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
On 2016/05/05 22:59:15, Dirk Pranke wrote:
> Is this really deps (i.e., components_browsertests won't link w/o it), or
should
> it be data_deps, in which case you don't need to also specify data, I don't
> think (unless maybe things are confused by this being a directory?)

No, it's not really a link deps. But specifying data_deps doesn't result in
Content Shell.app getting built.

[Dirk Pranke, 2016-05-06 20:56:34]

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:328: data += [ "$root_out_dir/resources.pak" ]
On 2016/05/06 20:52:25, Robert Sesek wrote:
> On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > Really I think these should be data_deps += [
> "//chrome:packed_extra_resources"
> > ] instead, to get the dependencies right, but we can fix that later.
> 
> Agree that this makes more sense as data_deps[] but that doesn't seem to be
> standard practice yet.

I've been fixing this where I've noticed it. I agree that it's not endemic yet.

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
On 2016/05/06 20:52:25, Robert Sesek wrote:
> On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > Is this really deps (i.e., components_browsertests won't link w/o it), or
> should
> > it be data_deps, in which case you don't need to also specify data, I don't
> > think (unless maybe things are confused by this being a directory?)
> 
> No, it's not really a link deps. But specifying data_deps doesn't result in
> Content Shell.app getting built.

It should; it would be a rather severe bug if it doesn't.

[Robert Sesek, 2016-05-06 22:11:04]

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:328: data += [ "$root_out_dir/resources.pak" ]
On 2016/05/06 20:56:34, Dirk Pranke wrote:
> On 2016/05/06 20:52:25, Robert Sesek wrote:
> > On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > > Really I think these should be data_deps += [
> > "//chrome:packed_extra_resources"
> > > ] instead, to get the dependencies right, but we can fix that later.
> > 
> > Agree that this makes more sense as data_deps[] but that doesn't seem to be
> > standard practice yet.
> 
> I've been fixing this where I've noticed it. I agree that it's not endemic
yet.

OK, I've updated it here.

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
On 2016/05/06 20:56:34, Dirk Pranke wrote:
> On 2016/05/06 20:52:25, Robert Sesek wrote:
> > On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > > Is this really deps (i.e., components_browsertests won't link w/o it), or
> > should
> > > it be data_deps, in which case you don't need to also specify data, I
don't
> > > think (unless maybe things are confused by this being a directory?)
> > 
> > No, it's not really a link deps. But specifying data_deps doesn't result in
> > Content Shell.app getting built.
> 
> It should; it would be a rather severe bug if it doesn't.

Do'h. I built the wrong target--I thought this was in _unittests not
_browsertests. It does build, but it doesn't properly package the directory (see
tryjob on ps5).

[Robert Sesek, 2016-05-06 22:22:16]

Patchset #6 (id:140001) has been deleted

[Dirk Pranke, 2016-05-06 22:28:56]

On 2016/05/06 22:11:04, Robert Sesek wrote:
> https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
> File components/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
> components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
> On 2016/05/06 20:56:34, Dirk Pranke wrote:
> > On 2016/05/06 20:52:25, Robert Sesek wrote:
> > > On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > > > Is this really deps (i.e., components_browsertests won't link w/o it),
or
> > > should
> > > > it be data_deps, in which case you don't need to also specify data, I
> don't
> > > > think (unless maybe things are confused by this being a directory?)
> > > 
> > > No, it's not really a link deps. But specifying data_deps doesn't result
in
> > > Content Shell.app getting built.
> > 
> > It should; it would be a rather severe bug if it doesn't.
> 
> Do'h. I built the wrong target--I thought this was in _unittests not
> _browsertests. It does build, but it doesn't properly package the directory
(see
> tryjob on ps5).

Hm. And regular deps makes this work? I'm not sure why the isolate choked on
this; that's
something I'd have to look into.

lgtm to land whatever works for now.

[Robert Sesek, 2016-05-09 15:11:12]

On 2016/05/06 22:28:56, Dirk Pranke wrote:
> On 2016/05/06 22:11:04, Robert Sesek wrote:
> > https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn
> > File components/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1944943003/diff/100001/components/BUILD.gn#ne...
> > components/BUILD.gn:425: deps += [ "//content/shell:content_shell" ]
> > On 2016/05/06 20:56:34, Dirk Pranke wrote:
> > > On 2016/05/06 20:52:25, Robert Sesek wrote:
> > > > On 2016/05/05 22:59:15, Dirk Pranke wrote:
> > > > > Is this really deps (i.e., components_browsertests won't link w/o it),
> or
> > > > should
> > > > > it be data_deps, in which case you don't need to also specify data, I
> > don't
> > > > > think (unless maybe things are confused by this being a directory?)
> > > > 
> > > > No, it's not really a link deps. But specifying data_deps doesn't result
> in
> > > > Content Shell.app getting built.
> > > 
> > > It should; it would be a rather severe bug if it doesn't.
> > 
> > Do'h. I built the wrong target--I thought this was in _unittests not
> > _browsertests. It does build, but it doesn't properly package the directory
> (see
> > tryjob on ps5).
> 
> Hm. And regular deps makes this work? I'm not sure why the isolate choked on
> this; that's
> something I'd have to look into.
> 
> lgtm to land whatever works for now.

Yup, not sure why that is either. Thanks for the review.

[Robert Sesek, 2016-05-09 15:11:23]

rsesek@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Robert Sesek, 2016-05-09 15:11:24]

+sdefresne for //components OWNERS

[sdefresne, 2016-05-09 17:43:10]

lgtm

[Robert Sesek, 2016-05-09 17:57:11]

The CQ bit was checked by rsesek@chromium.org

[Robert Sesek, 2016-05-09 17:57:11]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1944943003/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-05-09 17:57:45]

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

[commit-bot: I haz the power, 2016-05-09 18:03:40]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2016-05-09 18:08:48]

Description was changed from

==========
[Mac/GN] Start running components_unittests, components_browsertests, and
unit_tests.

BUG=431177
R=dpranke@chromium.org
==========

to

==========
[Mac/GN] Start running components_unittests, components_browsertests, and
unit_tests.

BUG=431177
R=dpranke@chromium.org

Committed: https://crrev.com/40f9bc31a00d01256da2f6819e4f3f30518b3261
Cr-Commit-Position: refs/heads/master@{#392362}
==========

[commit-bot: I haz the power, 2016-05-09 18:08:49]

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