Adding isolate for cc_perftests and triggering job on FYI waterfall

Adding isolate for cc_perftests and triggering job on FYI waterfall

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/7c79b91b7309efe242aaf0f34dd2c5272c519648
Cr-Commit-Position: refs/heads/master@{#428563}

[eyaich1, 2016-10-21 16:08:03]

eyaich@chromium.org changed reviewers:
+ dtu@chromium.org, kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-10-21 18:50:21]

lgtm

https://codereview.chromium.org/2442663004/diff/20001/testing/scripts/run_gte...
File testing/scripts/run_gtest_perf_test.py (right):

https://codereview.chromium.org/2442663004/diff/20001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:6: """Runs an isolate non-Telemetry perf
test .
Suggest "isolate bundled" or "isolated".

[eyaich1, 2016-10-21 19:48:19]

eyaich@chromium.org changed reviewers:
+ dpranke@chromium.org

[eyaich1, 2016-10-21 19:48:20]

+ dpranke@ for owners approval

[eyaich1, 2016-10-24 13:28:55]

https://codereview.chromium.org/2442663004/diff/20001/testing/scripts/run_gte...
File testing/scripts/run_gtest_perf_test.py (right):

https://codereview.chromium.org/2442663004/diff/20001/testing/scripts/run_gte...
testing/scripts/run_gtest_perf_test.py:6: """Runs an isolate non-Telemetry perf
test .
On 2016/10/21 18:50:21, Ken Russell wrote:
> Suggest "isolate bundled" or "isolated".

Done.

[dtu, 2016-10-25 21:48:24]

lgtm

[Dirk Pranke, 2016-10-26 18:55:10]

This is confusing to me ...

Why are we adding a target to //chrome/test for something that only depends on
//cc ? Can you just add the `data` dependencies to cc_perftests directly?

https://codereview.chromium.org/2442663004/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.json (right):

https://codereview.chromium.org/2442663004/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.json:29: ],
are all three of these args actually needed? Why isn't "name" sufficient?

[eyaich1, 2016-10-27 14:39:53]

So maybe you can clear up my understanding of gn files and the targets.  the
cc_perftests current target is a test(), where as the one I am adding is a
group().  I had a hard time distinguishing the difference between the two, but I
do know that the test targets goes in a data_deps attribute that is needed at
run time, but the other files are needed at build time.  It wasn't clear to me
that it would work the same if I just added the test cc_perftests target at the
"label" in gn_isolate_map.pyl

https://codereview.chromium.org/2442663004/diff/40001/testing/buildbot/chromi...
File testing/buildbot/chromium.perf.fyi.json (right):

https://codereview.chromium.org/2442663004/diff/40001/testing/buildbot/chromi...
testing/buildbot/chromium.perf.fyi.json:29: ],
On 2016/10/26 18:55:10, Dirk Pranke wrote:
> are all three of these args actually needed? Why isn't "name" sufficient?

name and isolate_name are needed as the name is the perf id which just happens
to be the same in this instance.  override_compile_targets I believe is just for
gyp so I *think* I can safely remove it.

[Dirk Pranke, 2016-10-27 23:20:15]

On 2016/10/27 14:39:53, eyaich1 wrote:
> So maybe you can clear up my understanding of gn files and the targets.  the
> cc_perftests current target is a test(), where as the one I am adding is a
> group().  I had a hard time distinguishing the difference between the two, but
I
> do know that the test targets goes in a data_deps attribute that is needed at
> run time, but the other files are needed at build time.  It wasn't clear to me
> that it would work the same if I just added the test cc_perftests target at
the
> "label" in gn_isolate_map.pyl

Yup, this is a good question, as these are confusing differences. In fact, 
I was just staring at your change in //BUILD.gn and wondering why it
was using data_deps instead of deps, only to realize I approved that change
a month ago :).

For non-group targets, the idea is that things specified in `data` and
`data_deps`
are needed by the target at runtime, but not at compile time. For example, 
in the way we use the ICU library, we need a big data file that gets loaded at 
runtime (icudtl.dat), but that file isn't used to actually compile chrome.

Normally if you have target("foo") { deps = [ ":bar" ] }, then foo's runtime
dependencies include bar's runtime dependencies.

However, if foo is a group, that doesn't happen. There are reasons for
this (see crbug.com/536289) but unfortunately that bug doesn't explain
what those reasons are, and I don't remember the details at the moment :(.

So, for groups, if you want the data_deps to propagate, to have to explicitly
tell it to do so.

Now, on desktop, a test() is simply an executable() that has testonly=true
(on Android and iOS life is more complicated, but that's not important for
this discussion). So, the rule is that if you need X to run the test, you 
should specify X as a data or data_deps for the test().

This gets further complicated because we have this schizophrenic world
at the moment where we distinguish between what you need to run locally
and what you need to run under an isolate, and so in some cases we
have additional targets that specify additional data things (like stuff
in //testing/scripts). Really we should just bundle those into the main 
test targets so that we don't have to think about this.

That's why I'm telling you to add these things to //cc even thought the
other targets in //chrome/test don't do this. Everything should work fine
(if it doesn't, come back to me and we'll figure out why it doesn't).

Does that make sense? I know it's a long explanation ...

[eyaich1, 2016-10-28 14:27:28]

eyaich@chromium.org changed reviewers:
+ enne@chromium.org

[eyaich1, 2016-10-28 14:27:28]

I think it makes sense, thanks for the explanation.  I was able to get it to
work with the existing cc_perftests target instead of adding a new one.  Added
enne@ for approval in that directory.

[eyaich1, 2016-10-28 14:28:00]

Description was changed from

==========
Adding isolate for cc_perftests and triggering job on FYI waterfall

BUG=chromium:633253
==========

to

==========
Adding isolate for cc_perftests and triggering job on FYI waterfall

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[enne (OOO), 2016-10-28 18:39:42]

rubber stamp lgtm

[Dirk Pranke, 2016-10-28 18:50:54]

lgtm

[eyaich1, 2016-10-28 23:34:14]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-10-28 23:34:14]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
dtu@chromium.org
Link to the patchset: https://codereview.chromium.org/2442663004/#ps120001
(title: "Using cc build target")

[commit-bot: I haz the power, 2016-10-28 23:34:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 00:39:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-29 00:47:06]

Description was changed from

==========
Adding isolate for cc_perftests and triggering job on FYI waterfall

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Adding isolate for cc_perftests and triggering job on FYI waterfall

BUG=chromium:633253
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/7c79b91b7309efe242aaf0f34dd2c5272c519648
Cr-Commit-Position: refs/heads/master@{#428563}
==========

[commit-bot: I haz the power, 2016-10-29 00:47:07]

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