Add crash_service to Telemetry's isolates.

Add crash_service to Telemetry's isolates.

Switch to using binary_manager for the crash_service executable,
eliminating downloads of this binary from cloud storage. (A follow-on
CL will likely forbid the bots from downloading this and other
executables from cloud storage entirely, rather than continuing to use
this as a fallback path.)

Telemetry's isolates are currently specified fairly differently for
the GYP and GN builds; other CLs underway will unify their
specification more.

Follow up CL's will be needed to add the client config to gpu's tests.

authors=kbr@,aiolos@

BUG=466877
Committed: https://crrev.com/6bdfb7dfb3ce2e571697629895089cffc42b48e6
Cr-Commit-Position: refs/heads/master@{#351196}

Committed: https://crrev.com/70c5d0c0e2ff03c3b6cef0bdce391f93c35d218a
Cr-Commit-Position: refs/heads/master@{#351803}

[aiolos (Not reviewing), 2015-09-22 18:00:03]

aiolos@chromium.org changed reviewers:
+ dpranke@chromium.org, kbr@chromium.org, maruel@chromium.org

[aiolos (Not reviewing), 2015-09-22 20:50:30]

aiolos@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[aiolos (Not reviewing), 2015-09-22 20:50:47]

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

[commit-bot: I haz the power, 2015-09-22 20:52:11]

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

[M-A Ruel, 2015-09-22 21:02:35]

Thanks for doing this! (And enduring me)

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
File tools/perf/core/binary_dependencies.json (right):

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
tools/perf/core/binary_dependencies.json:8:
"../../../out/Debug/crash_service.exe",
This is not correct on Win x64 though

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/project...
File tools/perf/core/project_config.py (right):

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/project...
tools/perf/core/project_config.py:15: binary_dependencies_file =
os.path.join(os.path.dirname(__file__),
os.path.abspath()

so it's os.chdir() safe.

[Ken Russell (switch to Gerrit), 2015-09-22 21:16:58]

Thanks for picking this up. It looks good overall. Have you verified locally
again that this prevents Telemetry from going to cloud storage for
crash_service.exe?

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
File tools/perf/core/binary_dependencies.json (right):

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
tools/perf/core/binary_dependencies.json:8:
"../../../out/Debug/crash_service.exe",
On 2015/09/22 21:02:35, M-A Ruel wrote:
> This is not correct on Win x64 though

I think Telemetry's notion of the platform is different than Chromium's build.
We could add more local_paths if it turns out that Telemetry's "win_AMD64"
platform has to support both 32- and 64-bit builds.

[aiolos (Not reviewing), 2015-09-22 21:36:05]

On 2015/09/22 21:16:58, Ken Russell wrote:
> Thanks for picking this up. It looks good overall. Have you verified locally
> again that this prevents Telemetry from going to cloud storage for
> crash_service.exe?
> 

It does in tools/perf benchmark runs, but
https://codereview.chromium.org/1358903004/ will need to land in order for the
gpu tests to use the local copy. If you want that added to this CL, I can do so.

I also have a couple CL's that are needed to fix it for the unittests that
require some Telemetry changes.

[Dirk Pranke, 2015-09-22 21:36:21]

lgtm w/ the others' concerns addressed.

[aiolos (Not reviewing), 2015-09-22 21:39:47]

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
File tools/perf/core/binary_dependencies.json (right):

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/binary_...
tools/perf/core/binary_dependencies.json:8:
"../../../out/Debug/crash_service.exe",
On 2015/09/22 21:16:58, Ken Russell wrote:
> On 2015/09/22 21:02:35, M-A Ruel wrote:
> > This is not correct on Win x64 though
> 
> I think Telemetry's notion of the platform is different than Chromium's build.
> We could add more local_paths if it turns out that Telemetry's "win_AMD64"
> platform has to support both 32- and 64-bit builds.

Telemetry's notion is different, which is why I added some logging in the
dependency_manager to help determine when we're looking at the local files for
any platforms that we've missed. win_AMD64 is the only platform we have
supported in the current system (support_binaries). Instead of guessing what's
broken because we don't have support for it, I figured I'd add support where we
know we need it, and do a second pass for any bots that aren't fixed.

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/project...
File tools/perf/core/project_config.py (right):

https://codereview.chromium.org/1361523003/diff/40001/tools/perf/core/project...
tools/perf/core/project_config.py:15: binary_dependencies_file =
os.path.join(os.path.dirname(__file__),
On 2015/09/22 21:02:35, M-A Ruel wrote:
> os.path.abspath()
> 
> so it's os.chdir() safe.

Done. Thanks.

[Ken Russell (switch to Gerrit), 2015-09-22 21:40:52]

On 2015/09/22 21:36:05, aiolos wrote:
> On 2015/09/22 21:16:58, Ken Russell wrote:
> > Thanks for picking this up. It looks good overall. Have you verified locally
> > again that this prevents Telemetry from going to cloud storage for
> > crash_service.exe?
> > 
> 
> It does in tools/perf benchmark runs, but
> https://codereview.chromium.org/1358903004/ will need to land in order for the
> gpu tests to use the local copy. If you want that added to this CL, I can do
so.

It looks like there were some severe test failures in the other CL. Were they
caused by that CL implicitly depending on this one, but this one hadn't landed?

From my standpoint, please feel free to include it or not include it, as you see
fit.

LGTM overall.

> I also have a couple CL's that are needed to fix it for the unittests that
> require some Telemetry changes.

OK.

[aiolos (Not reviewing), 2015-09-22 21:50:22]

On 2015/09/22 21:40:52, Ken Russell wrote:
> On 2015/09/22 21:36:05, aiolos wrote:
> > On 2015/09/22 21:16:58, Ken Russell wrote:
> > > Thanks for picking this up. It looks good overall. Have you verified
locally
> > > again that this prevents Telemetry from going to cloud storage for
> > > crash_service.exe?
> > > 
> > 
> > It does in tools/perf benchmark runs, but
> > https://codereview.chromium.org/1358903004/ will need to land in order for
the
> > gpu tests to use the local copy. If you want that added to this CL, I can do
> so.
> 
> It looks like there were some severe test failures in the other CL. Were they
> caused by that CL implicitly depending on this one, but this one hadn't
landed?

No, this cl should have been applied first due to the branch dependencies. I'll
need to rebase the other cl once this lands, but otherwise there shouldn't be
any issues.

The failures were in non-gpu tests even though the only change is in
run_gpu_test.py, so it's likely due to unrelated issues.

> 
> From my standpoint, please feel free to include it or not include it, as you
see
> fit.
> 
> LGTM overall.
> 
> > I also have a couple CL's that are needed to fix it for the unittests that
> > require some Telemetry changes.
> 
> OK.

[aiolos (Not reviewing), 2015-09-22 21:56:23]

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

[commit-bot: I haz the power, 2015-09-22 21:57:05]

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

[commit-bot: I haz the power, 2015-09-23 03:35:28]

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

[commit-bot: I haz the power, 2015-09-23 03:35:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[aiolos (Not reviewing), 2015-09-23 18:59:55]

From discussion in https://codereview.chromium.org/1358903004/ with kbr: should
I move content/telemetry_binary_manager.isolate to chrome instead?

[Ken Russell (switch to Gerrit), 2015-09-23 19:06:13]

On 2015/09/23 18:59:55, aiolos wrote:
> From discussion in https://codereview.chromium.org/1358903004/ with kbr:
should
> I move content/telemetry_binary_manager.isolate to chrome instead?

Since crash_service is a target at the chrome/ level, not content/, I do think
that telemetry_binary_manager.isolate should be moved to chrome/.

That might also imply that telemetry.isolate itself should be moved to chrome/
from content/, since it has implicit dependencies in chrome/. I moved it into
content/ in https://codereview.chromium.org/1212113003 but maybe that was a
mistake. Please feel free to move it back. The GN build should be updated in
that case too.

[aiolos (Not reviewing), 2015-09-24 00:48:25]

On 2015/09/23 19:06:13, Ken Russell wrote:
> On 2015/09/23 18:59:55, aiolos wrote:
> > From discussion in https://codereview.chromium.org/1358903004/ with kbr:
> should
> > I move content/telemetry_binary_manager.isolate to chrome instead?
> 
> Since crash_service is a target at the chrome/ level, not content/, I do think
> that telemetry_binary_manager.isolate should be moved to chrome/.
> 
> That might also imply that telemetry.isolate itself should be moved to chrome/
> from content/, since it has implicit dependencies in chrome/. I moved it into
> content/ in https://codereview.chromium.org/1212113003 but maybe that was a
> mistake. Please feel free to move it back. The GN build should be updated in
> that case too.

If the telemetry.isolate is moved out of content to chrome, so should the
telemetry_gpu_unittetss.isolate I've updated the CL to reflect that change for
the isolates.

This change will allow anyone using telemetry.isolate to use the local files.
I'm not sure if this is the correct approach or not. I'm not 100% sure that the
gpu_unittests will need the local files, but I know that the perf and telemetry
unittests do. It might be better to leave the binary_manager dependencies in
telemetry_chrome_test.isolate instead of telemetry.isolate and remove the
changes to gpu_unittests and see if things are broken.

If you aren't using any of the codepaths that pull in dependencies, it should
just work without the local files or the json file.

(Note that the BULID.gn files aren't updated since I wanted to vet that this is
actually the approach we want to take before making those changes.)

[Ken Russell (switch to Gerrit), 2015-09-24 01:46:33]

On 2015/09/24 00:48:25, aiolos wrote:
> On 2015/09/23 19:06:13, Ken Russell wrote:
> > On 2015/09/23 18:59:55, aiolos wrote:
> > > From discussion in https://codereview.chromium.org/1358903004/ with kbr:
> > should
> > > I move content/telemetry_binary_manager.isolate to chrome instead?
> > 
> > Since crash_service is a target at the chrome/ level, not content/, I do
think
> > that telemetry_binary_manager.isolate should be moved to chrome/.
> > 
> > That might also imply that telemetry.isolate itself should be moved to
chrome/
> > from content/, since it has implicit dependencies in chrome/. I moved it
into
> > content/ in https://codereview.chromium.org/1212113003 but maybe that was a
> > mistake. Please feel free to move it back. The GN build should be updated in
> > that case too.
> 
> If the telemetry.isolate is moved out of content to chrome, so should the
> telemetry_gpu_unittetss.isolate I've updated the CL to reflect that change for
> the isolates.

Still LGTM

> This change will allow anyone using telemetry.isolate to use the local files.
> I'm not sure if this is the correct approach or not. I'm not 100% sure that
the
> gpu_unittests will need the local files, but I know that the perf and
telemetry
> unittests do. It might be better to leave the binary_manager dependencies in
> telemetry_chrome_test.isolate instead of telemetry.isolate and remove the
> changes to gpu_unittests and see if things are broken.

Probably telemetry_binary_manager.isolate should be included by
telemetry_chrome_test.isolate, since the other binaries like crash_service are
only needed if launching a browser. Conceptually, telemetry.isolate doesn't
include any facilities for launching a browser.

nednguyen found in https://codereview.chromium.org/1354223004/ that
telemetry_gpu_unittests does actually need a Chrome binary right now, which is a
bug, so if you want to keep things organized the way they are in this CL for
now, that's fine. Once the other bug is fixed, the telemetry_gpu_unittests'
isolate can be freed of the Chrome dependency, and the telemetry_chrome_test
isolate can be the one including the telemetry_binary_manager isolate.

> If you aren't using any of the codepaths that pull in dependencies, it should
> just work without the local files or the json file.
> 
> (Note that the BULID.gn files aren't updated since I wanted to vet that this
is
> actually the approach we want to take before making those changes.)

[Paweł Hajdan Jr., 2015-09-24 14:26:08]

LGTM

[aiolos (Not reviewing), 2015-09-25 17:36:38]

Patchset #6 (id:100001) has been deleted

[aiolos (Not reviewing), 2015-09-25 18:50:43]

Patchset #6 (id:120001) has been deleted

[aiolos (Not reviewing), 2015-09-25 20:09:55]

ptal

[Dirk Pranke, 2015-09-25 20:16:20]

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

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:222: source_set("telemtry_binary_deps") {
if there's no sources, it shouldn't be a source_set(). 

Does group() work (I think it should)?

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:223: # Telemetry's chrome built dependencies.
I don't understand this comment?

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:618: ":telemtry_binary_deps",
typo: ":telemetry" ?

https://codereview.chromium.org/1361523003/diff/140001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1361523003/diff/140001/content/test/BUILD.gn#...
content/test/BUILD.gn:226: static_library("telemetry_test_support") {
same comment.

Does it make sense to move this target into a new file in
//tools/telemetry/BUILD.gn ?

[aiolos (Not reviewing), 2015-09-25 20:40:07]

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

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:222: source_set("telemtry_binary_deps") {
On 2015/09/25 20:16:20, Dirk Pranke wrote:
> if there's no sources, it shouldn't be a source_set(). 
> 
> Does group() work (I think it should)?

Yep. I went with that since the only other target I saw that only specified
data_deps was a source_set.

Done.

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:223: # Telemetry's chrome built dependencies.
On 2015/09/25 20:16:20, Dirk Pranke wrote:
> I don't understand this comment? 

Meant to convey that this target is specifically for specifying the location of
the binary dependencies that are built with chrome/chromium, and not part of
telemetry itself. Removed to avoid confusion.

https://codereview.chromium.org/1361523003/diff/140001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:618: ":telemtry_binary_deps",
On 2015/09/25 20:16:20, Dirk Pranke wrote:
> typo: ":telemetry" ?

Done.

https://codereview.chromium.org/1361523003/diff/140001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1361523003/diff/140001/content/test/BUILD.gn#...
content/test/BUILD.gn:226: static_library("telemetry_test_support") {
On 2015/09/25 20:16:20, Dirk Pranke wrote:
> same comment.
> 
> Does it make sense to move this target into a new file in
> //tools/telemetry/BUILD.gn ?

Done.

[Dirk Pranke, 2015-09-25 20:42:04]

lgtm

[Ken Russell (switch to Gerrit), 2015-09-25 21:49:11]

Please hold off submitting this.
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
indicates catastrophic failures. I don't know whether they were caused by this
CL and I don't know why the tryjob appeared to pass. Filed crbug.com/536192
about why the tryjob was green.

[aiolos (Not reviewing), 2015-09-25 21:54:03]

On 2015/09/25 21:49:11, Ken Russell wrote:
> Please hold off submitting this.
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> indicates catastrophic failures. I don't know whether they were caused by this
> CL and I don't know why the tryjob appeared to pass. Filed crbug.com/536192
> about why the tryjob was green.

Thanks for catching that. I'm looking into the failures now.

[aiolos (Not reviewing), 2015-09-25 21:55:23]

On 2015/09/25 21:54:03, aiolos wrote:
> On 2015/09/25 21:49:11, Ken Russell wrote:
> > Please hold off submitting this.
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > indicates catastrophic failures. I don't know whether they were caused by
this
> > CL and I don't know why the tryjob appeared to pass. Filed crbug.com/536192
> > about why the tryjob was green.
> 
> Thanks for catching that. I'm looking into the failures now.

(They appear to be failing due to not failing to find the browser, but I haven't
determined if it's due to this CL or not.)

[Ken Russell (switch to Gerrit), 2015-09-26 02:57:29]

On 2015/09/25 21:55:23, aiolos wrote:
> On 2015/09/25 21:54:03, aiolos wrote:
> > On 2015/09/25 21:49:11, Ken Russell wrote:
> > > Please hold off submitting this.
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
> > > indicates catastrophic failures. I don't know whether they were caused by
> this
> > > CL and I don't know why the tryjob appeared to pass. Filed
crbug.com/536192
> > > about why the tryjob was green.
> > 
> > Thanks for catching that. I'm looking into the failures now.
> 
> (They appear to be failing due to not failing to find the browser, but I
haven't
> determined if it's due to this CL or not.)

The failures aren't caused by your CL, per
https://code.google.com/p/chromium/issues/detail?id=536192#c19 ; they've been
fixed. However, there's an unfortunate bug in the trybots' scripts which would
allow deep breakage of the Telemetry framework to slip through the commit queue,
per https://code.google.com/p/chromium/issues/detail?id=536192#c20 .

If you wouldn't mind waiting for
https://code.google.com/p/chromium/issues/detail?id=536192 to be fixed fully, it
would avoid any possibility of your change breaking the GPU tests. Otherwise,
please send another dry run and carefully examine the output from
linux_chromium_rel_ng, mac_chromium_rel_ng and win_chromium_rel_ng, and
double-check that no failures are reported for steps like
webgl_conformance_tests. Thanks much.

[Ken Russell (switch to Gerrit), 2015-09-26 02:58:00]

For the record, LGTM again, assuming that this passes the tests.

[M-A Ruel, 2015-09-28 16:42:12]

cc stip, jbudorick, pkotwicz, agrieve for android/isolate part.

lgtm with nits

https://codereview.chromium.org/1361523003/diff/160001/tools/perf/core/projec...
File tools/perf/core/project_config.py (right):

https://codereview.chromium.org/1361523003/diff/160001/tools/perf/core/projec...
tools/perf/core/project_config.py:7: 
Create one clear variable:

THIS_DIR = os.path.dirname(os.path.abspath((__file__))

then replace all the uses of __file__, as most of them are incorrect anyway and
wouldn't survive a os.chdir().

[aiolos (Not reviewing), 2015-09-28 17:35:18]

https://codereview.chromium.org/1361523003/diff/160001/tools/perf/core/projec...
File tools/perf/core/project_config.py (right):

https://codereview.chromium.org/1361523003/diff/160001/tools/perf/core/projec...
tools/perf/core/project_config.py:7: 
On 2015/09/28 16:42:12, M-A Ruel wrote:
> Create one clear variable:
> 
> THIS_DIR = os.path.dirname(os.path.abspath((__file__))
> 
> then replace all the uses of __file__, as most of them are incorrect anyway
and
> wouldn't survive a os.chdir().

Done.

[aiolos (Not reviewing), 2015-09-28 21:48:26]

crbug.com/536192 has been fixed, so I'm going to send this through the CQ.

[aiolos (Not reviewing), 2015-09-28 21:48:36]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-09-28 21:48:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, dpranke@chromium.org, maruel@chromium.org,
kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/1361523003/#ps180001
(title: "Maruel nits.")

[commit-bot: I haz the power, 2015-09-28 21:52:42]

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

[commit-bot: I haz the power, 2015-09-28 22:08:11]

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

[commit-bot: I haz the power, 2015-09-28 22:08:12]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ken Russell (switch to Gerrit), 2015-09-28 22:40:10]

On 2015/09/28 22:08:12, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Why is this failing? maruel@ is an OWNER of those files. Maybe it's because
you've updated the patch set since his last thumbs-up?

[aiolos (Not reviewing), 2015-09-28 22:48:44]

aiolos@chromium.org changed reviewers:
+ piman@chromium.org

[aiolos (Not reviewing), 2015-09-28 22:52:21]

Adding pieman as a secondary owner, and filed crbug.com/536970

[piman, 2015-09-28 22:52:26]

lgtm

[aiolos (Not reviewing), 2015-09-28 22:53:12]

The CQ bit was checked by aiolos@chromium.org

[commit-bot: I haz the power, 2015-09-28 22:54:09]

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

[commit-bot: I haz the power, 2015-09-28 23:13:44]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-09-28 23:14:40]

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

[kcarattini, 2015-09-29 04:14:16]

A revert of this CL (patchset #8 id:180001) has been created in
https://codereview.chromium.org/1379473002/ by kcarattini@chromium.org.

The reason for reverting is: Suspected build fail see
https://code.google.com/p/chromium/issues/detail?id=536998 and
https://code.google.com/p/chromium/issues/detail?id=537065.

[aiolos (Not reviewing), 2015-09-30 02:27:04]

tldr: Dirk was right about the issue. PTAL

I repro'd the build failure locally, and have verified that this change fixes it
on my local linux and mac checkouts.

To repro locally: make sure your GYP_DEFINES contains only: buildtype=Official
branding=Chrome
And build all release targets:
ninja -C out/Release


I have also included the run_gpu_test changes from 1358903004 to avoid
interdependencies in the event of any future reverts.

[Dirk Pranke, 2015-09-30 02:53:30]

and ... to answer my confusion, apparently test_isolation_mode=="check" by
default on the official desktop builds, even though we don't use swarming on the
official desktop builds, which is kinda dumb. I thought it was defaulting to
"noop".

[M-A Ruel, 2015-09-30 13:16:06]

On 2015/09/30 02:53:30, Dirk Pranke wrote:
> and ... to answer my confusion, apparently test_isolation_mode=="check" by
> default on the official desktop builds, even though we don't use swarming on
the
> official desktop builds, which is kinda dumb. I thought it was defaulting to
> "noop".

http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Linux%...

It works with check. Eventually we want to support Swarming on official builds
so we didn't want to break it.

[Ken Russell (switch to Gerrit), 2015-09-30 18:07:29]

Still LGTM.

[aiolos (Not reviewing), 2015-09-30 18:20:48]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-09-30 18:20:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, phajdan.jr@chromium.org, maruel@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1361523003/#ps200001
(title: "Fix gyp all targets build failure.")

[commit-bot: I haz the power, 2015-09-30 18:22:38]

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

[Dirk Pranke, 2015-09-30 18:24:00]

lgtm w/ one minor comment.

https://codereview.chromium.org/1361523003/diff/200001/tools/telemetry/catapu...
File tools/telemetry/catapult_base/dependency_manager/dependency_manager.py
(right):

https://codereview.chromium.org/1361523003/diff/200001/tools/telemetry/catapu...
tools/telemetry/catapult_base/dependency_manager/dependency_manager.py:221:
logging.info('local_path %s exists.', local_path)
Do you really want to commit these logging lines?

[aiolos (Not reviewing), 2015-09-30 18:24:53]

The CQ bit was unchecked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-09-30 18:32:16]

https://codereview.chromium.org/1361523003/diff/200001/tools/telemetry/catapu...
File tools/telemetry/catapult_base/dependency_manager/dependency_manager.py
(right):

https://codereview.chromium.org/1361523003/diff/200001/tools/telemetry/catapu...
tools/telemetry/catapult_base/dependency_manager/dependency_manager.py:221:
logging.info('local_path %s exists.', local_path)
On 2015/09/30 18:24:00, Dirk Pranke wrote:
> Do you really want to commit these logging lines?

Done.

[aiolos (Not reviewing), 2015-09-30 18:32:23]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-09-30 18:32:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, phajdan.jr@chromium.org, maruel@chromium.org,
kbr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1361523003/#ps220001
(title: "Dirk nit.")

[commit-bot: I haz the power, 2015-09-30 18:35:02]

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

[commit-bot: I haz the power, 2015-09-30 18:43:43]

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

[commit-bot: I haz the power, 2015-09-30 18:43:45]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[aiolos (Not reviewing), 2015-09-30 19:50:56]

Patchset #12 (id:260001) has been deleted

[aiolos (Not reviewing), 2015-09-30 22:49:58]

Patchset #12 (id:280001) has been deleted

[aiolos (Not reviewing), 2015-09-30 23:01:52]

Patchset #12 (id:300001) has been deleted

[aiolos (Not reviewing), 2015-10-01 00:11:13]

Patchset #12 (id:320001) has been deleted

[aiolos (Not reviewing), 2015-10-01 14:57:19]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-10-01 14:57:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, phajdan.jr@chromium.org, maruel@chromium.org,
kbr@chromium.org, piman@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1361523003/#ps340001 (title: " ")

[commit-bot: I haz the power, 2015-10-01 14:58:20]

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

[commit-bot: I haz the power, 2015-10-01 15:08:43]

Committed patchset #12 (id:340001)

[commit-bot: I haz the power, 2015-10-01 15:09:36]

Patchset 12 (id:??) landed as
https://crrev.com/70c5d0c0e2ff03c3b6cef0bdce391f93c35d218a
Cr-Commit-Position: refs/heads/master@{#351803}

[jwd, 2015-10-01 15:58:01]

A revert of this CL (patchset #12 id:340001) has been created in
https://codereview.chromium.org/1369213004/ by jwd@chromium.org.

The reason for reverting is: Seems to be causing failures on linux test bots
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/318....