Add dependency_manager initialization to binary_manager

Add dependency_manager initialization to binary_manager

BUG=510231
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

Committed: https://crrev.com/f592ba18312f43fe4186cc526db00960a1db0b48
Cr-Commit-Position: refs/heads/master@{#343320}

Committed: https://crrev.com/780b193b7e92cef04c6c1063068e6f5edbf8e3f2
Cr-Commit-Position: refs/heads/master@{#343547}

[aiolos (Not reviewing), 2015-08-07 23:28:46]

aiolos@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com, sullivan@chromium.org

[aiolos (Not reviewing), 2015-08-10 16:48:26]

Please ignore the dependency_manager file. It will get rebased out of this cl
once https://codereview.chromium.org/1273223002/ lands. (Which is currently
blocked on CQ issues.)

[nednguyen, 2015-08-10 16:54:46]

lgtm

[nednguyen, 2015-08-10 17:01:10]

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/binary_manager.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:21:
util.GetTelemetryDir(), 'telemetry', 'internal', 'binary_dependencies.json')
Did you already add this file?

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:38:
logging.debug('Initialized the dependency manager.')
This log should come before the dependency_manager.DependencyManager in case of
failure?
Also maybe add the content of config_files in the debug message?

[eakuefner, 2015-08-10 17:07:03]

lgtm

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/b...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/b...
tools/telemetry/telemetry/benchmark_runner.py:80: TODO(aiolos): Add link to
description/example of a ProjectDependency file.
Can you move the todo out of the docstring itself?

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/binary_manager.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:29: def
InitDependencyManager(environment_config):
nit: two newlines between top-level definitions.

[eakuefner, 2015-08-10 19:57:23]

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/binary_manager_unittest.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager_unittest.py:7:
util.AddDirToPythonPath(util.GetTelemetryDir(), 'third_party', 'mock')
Note: as crrev.com/1276173004 has landed, this is no longer necessary!

You can write instead:
from telemetry.third_party import mock

[aiolos (Not reviewing), 2015-08-10 20:37:27]

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/b...
File tools/telemetry/telemetry/benchmark_runner.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/b...
tools/telemetry/telemetry/benchmark_runner.py:80: TODO(aiolos): Add link to
description/example of a ProjectDependency file.
On 2015/08/10 17:07:03, eakuefner wrote:
> Can you move the todo out of the docstring itself?

Done.

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/binary_manager.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:21:
util.GetTelemetryDir(), 'telemetry', 'internal', 'binary_dependencies.json')
On 2015/08/10 17:01:10, nednguyen wrote:
> Did you already add this file?

Not yet. We don't actually do anything with the file yet, so I was going to add
it in the CL that included the dependency_manager's actual initialization code.
But thought knowing how it was getting passed in was useful for this CL. I can
easily add an empty json file if you'd prefer.

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:29: def
InitDependencyManager(environment_config):
On 2015/08/10 17:07:03, eakuefner wrote:
> nit: two newlines between top-level definitions.

Done.

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager.py:38:
logging.debug('Initialized the dependency manager.')
On 2015/08/10 17:01:10, nednguyen wrote:
> This log should come before the dependency_manager.DependencyManager in case
of
> failure?

That's actually why I wanted it after. I've cared less about whether we tried to
initialize it and more that we succeeded in my debugging thus far, although that
may not always be the case. I'm going to add a log before hand instead of moving
it.

> Also maybe add the content of config_files in the debug message?

Done.

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/util/binary_manager_unittest.py (right):

https://codereview.chromium.org/1280903003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/util/binary_manager_unittest.py:7:
util.AddDirToPythonPath(util.GetTelemetryDir(), 'third_party', 'mock')
On 2015/08/10 19:57:23, eakuefner wrote:
> Note: as crrev.com/1276173004 has landed, this is no longer necessary!
> 
> You can write instead:
> from telemetry.third_party import mock

Done.

[aiolos (Not reviewing), 2015-08-10 22:00:20]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-08-10 22:00:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1280903003/#ps40001
(title: "rebase")

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

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

[commit-bot: I haz the power, 2015-08-10 22:34:19]

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

[commit-bot: I haz the power, 2015-08-10 22:34:20]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aiolos (Not reviewing), 2015-08-12 19:41:30]

Patchset #5 (id:80001) has been deleted

[aiolos (Not reviewing), 2015-08-12 20:23:49]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-08-12 20:23:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1280903003/#ps100001
(title: "Fix failing tests. Move InitBinaryManagerForUnittests to
telemetry.testing")

[commit-bot: I haz the power, 2015-08-12 20:24:09]

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

[commit-bot: I haz the power, 2015-08-12 20:28:54]

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

[commit-bot: I haz the power, 2015-08-12 20:28:55]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_perf_bisect/...)

[aiolos (Not reviewing), 2015-08-13 00:38:48]

Patchset #8 (id:160001) has been deleted

[aiolos (Not reviewing), 2015-08-13 00:52:46]

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

[aiolos (Not reviewing), 2015-08-13 00:56:38]

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

[commit-bot: I haz the power, 2015-08-13 00:56:49]

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

[commit-bot: I haz the power, 2015-08-13 01:00:38]

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

[commit-bot: I haz the power, 2015-08-13 01:00:39]

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

[aiolos (Not reviewing), 2015-08-13 01:12:09]

Patchset #8 (id:120002) has been deleted

[nednguyen, 2015-08-13 20:18:43]

nednguyen@google.com changed reviewers:
+ dpranke@google.com

[nednguyen, 2015-08-13 20:18:44]

+Dirk: Kari is having trouble landing this patch due to the way typ's
parallelization works on windows. Can you help?

[Dirk Pranke, 2015-08-13 20:30:42]

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

[Dirk Pranke, 2015-08-13 20:30:42]

https://codereview.chromium.org/1280903003/diff/100002/tools/telemetry/teleme...
File tools/telemetry/telemetry/testing/run_tests.py (right):

https://codereview.chromium.org/1280903003/diff/100002/tools/telemetry/teleme...
tools/telemetry/telemetry/testing/run_tests.py:66:
binary_manager.InitDependencyManager(args.client_config)
You should try to avoid setting global variables as they don't really work in
Python on windows in a multiprocessing context (there's no fork, so the state
isn't copied over).

If you really need a global, and need this routine to be called in every
process, then make sure it is called via _SetUpProcess(), below.

[aiolos (Not reviewing), 2015-08-13 20:32:58]

On 2015/08/13 20:18:44, nednguyen wrote:
> +Dirk: Kari is having trouble landing this patch due to the way typ's
> parallelization works on windows. Can you help?

I *could* go through and initialize things in the unittests/the testing helper
functions to unblock this CL, but that won't work long term. (With the
assumption that we want the client config to work for run_tests as well as for
the benchmarks.)

[aiolos (Not reviewing), 2015-08-13 21:42:16]

Patchset #9 (id:190001) has been deleted

[nednguyen, 2015-08-13 22:20:45]

lgtm with nits

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
File tools/telemetry/telemetry/testing/run_tests.py (right):

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
tools/telemetry/telemetry/testing/run_tests.py:66:
binary_manager.InitDependencyManager(args.client_config)
I think it's better to put this in the Run(self, args) method below. Putting
this in ProcessCommandLineArgs seems a bit off..

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
tools/telemetry/telemetry/testing/run_tests.py:174: def _SetUpProcess(child,
context): # pylint: disable=W0613
Nit: add comment explaining why we need to init the dependency manager again
here.

[aiolos (Not reviewing), 2015-08-13 22:28:33]

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
File tools/telemetry/telemetry/testing/run_tests.py (right):

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
tools/telemetry/telemetry/testing/run_tests.py:66:
binary_manager.InitDependencyManager(args.client_config)
On 2015/08/13 22:20:45, nednguyen wrote:
> I think it's better to put this in the Run(self, args) method below. Putting
> this in ProcessCommandLineArgs seems a bit off..

We can only do this if we move the check for possible browser to Run as well.
The call to browser_finder.FindBrowser will raise an exception if we don't
initialize the dependency_manager first.

https://codereview.chromium.org/1280903003/diff/210001/tools/telemetry/teleme...
tools/telemetry/telemetry/testing/run_tests.py:174: def _SetUpProcess(child,
context): # pylint: disable=W0613
On 2015/08/13 22:20:45, nednguyen wrote:
> Nit: add comment explaining why we need to init the dependency manager again
> here.

Acknowledged.

[aiolos (Not reviewing), 2015-08-13 22:35:07]

ptal

[aiolos (Not reviewing), 2015-08-13 22:36:54]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-08-13 22:36:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1280903003/#ps230001
(title: "Nits")

[nednguyen, 2015-08-13 22:37:00]

lgtm

[commit-bot: I haz the power, 2015-08-13 22:37:23]

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

[commit-bot: I haz the power, 2015-08-13 22:41:28]

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

[commit-bot: I haz the power, 2015-08-13 22:41:29]

Try jobs failed on following builders:
  mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_perf_bisect/...)

[aiolos (Not reviewing), 2015-08-13 22:46:15]

On 2015/08/13 22:41:29, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_perf_bisect on tryserver.chromium.perf (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_perf_bisect/...)

removing the mac_perf try job since it's failing on gclient hooks

[aiolos (Not reviewing), 2015-08-13 22:46:20]

The CQ bit was checked by aiolos@chromium.org

[commit-bot: I haz the power, 2015-08-13 22:46:43]

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

[commit-bot: I haz the power, 2015-08-13 23:53:48]

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

[commit-bot: I haz the power, 2015-08-13 23:53:49]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[aiolos (Not reviewing), 2015-08-14 00:44:46]

The CQ bit was checked by aiolos@chromium.org

[commit-bot: I haz the power, 2015-08-14 00:45:09]

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

[commit-bot: I haz the power, 2015-08-14 02:16:26]

Committed patchset #10 (id:230001)

[commit-bot: I haz the power, 2015-08-14 02:17:08]

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

[binjin, 2015-08-14 10:10:39]

A revert of this CL (patchset #10 id:230001) has been created in
https://codereview.chromium.org/1296563002/ by binjin@chromium.org.

The reason for reverting is: speculative reverts: break the XP bots
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds....

[aiolos (Not reviewing), 2015-08-14 20:39:11]

Patchset #11 (id:250001) has been deleted

[nednguyen, 2015-08-14 22:17:49]

lgtm

[aiolos (Not reviewing), 2015-08-15 00:19:22]

The CQ bit was checked by aiolos@chromium.org

[aiolos (Not reviewing), 2015-08-15 00:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
eakuefner@chromium.org
Link to the patchset: https://codereview.chromium.org/1280903003/#ps270001
(title: "Fix and refactor the unittest.")

[commit-bot: I haz the power, 2015-08-15 00:19:43]

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

[commit-bot: I haz the power, 2015-08-15 02:54:31]

Committed patchset #11 (id:270001)

[commit-bot: I haz the power, 2015-08-15 02:55:28]

Patchset 11 (id:??) landed as
https://crrev.com/780b193b7e92cef04c6c1063068e6f5edbf8e3f2
Cr-Commit-Position: refs/heads/master@{#343547}