[Android] Add a configurable environment for devil/.

[Android] Add a configurable environment for devil/.

BUG=476719
Committed: https://crrev.com/975fc02186d195c3baf6f0380fed06433c636670
Cr-Commit-Position: refs/heads/master@{#352359}

[jbudorick, 2015-09-09 23:47:06]

jbudorick@chromium.org changed reviewers:
+ mikecase@chromium.org, perezju@chromium.org, rnephew@chromium.org

[jbudorick, 2015-09-09 23:47:07]

nednguyen: fyi, because I plan to use this (or whatever comes of it) in
telemetry instead of the references to pylib.constants

https://codereview.chromium.org/1316413003/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/test_runner.p...
build/android/test_runner.py:122: devil.env.android_sdk =
devil.env.android_sdk_config.AndroidSdkConfig(
Configuring devil's environment is intentionally ugly because it:
 - shouldn't be done often, and
 - shouldn't be easy to do on accident

[mikecase (-- gone --), 2015-09-10 00:54:16]

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
File build/android/devil/env/android_sdk_config.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:18: or
os.environ.get(DEVIL_ANDROID_SDK_ROOT))
I think this could be a little confusing since these configs are stored as
global variables in env/__init__.py and its unclear where and if they are set
(or even that you have to set them).

Maybe raise an exception if both its not configured and the environment variable
isn't set to anything? Could make it easier to debug, but it might make the code
overly confusing.

[nednguyen, 2015-09-10 02:39:02]

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

[nednguyen, 2015-09-10 02:39:02]

+Kari since we are taking a similar strategy here.

[perezju, 2015-09-10 09:45:49]

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/android...
File build/android/devil/android/battery_utils_test.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/android...
build/android/devil/android/battery_utils_test.py:23:
sys.path.append(devil.env.dependencies.pymock_path)
+1!! Love how this looks.

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/__i...
File build/android/devil/env/__init__.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/__i...
build/android/devil/env/__init__.py:13: 
Maybe add some comments here explaining how/where these configs should be
set/changed? (With a remark that this probably shouldn't be done too often?)

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
File build/android/devil/env/android_sdk_config.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:10: DEVIL_ANDROID_SDK_ROOT =
'DEVIL_ANDROID_SDK_ROOT'
nit: it's not clear from the name if this is the _value_ of the SDK_ROOT or, as
it's actually the case, the _key_ used to look up the value within environment
vars.

Also, we probably don't need 'DEVIL' in the name of the constant (it's fine in
the name of the key).

maybe: ANDROID_SDK_ROOT_KEY = 'DEVIL_ANDROID_SDK_ROOT'?

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:18: or
os.environ.get(DEVIL_ANDROID_SDK_ROOT))
On 2015/09/10 00:54:16, mikecase wrote:
> I think this could be a little confusing since these configs are stored as
> global variables in env/__init__.py and its unclear where and if they are set
> (or even that you have to set them).
> 
> Maybe raise an exception if both its not configured and the environment
variable
> isn't set to anything? Could make it easier to debug, but it might make the
code
> overly confusing.

+1 to raising an exception. Either that or providing some other sensible default
other than |None|.

https://codereview.chromium.org/1316413003/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/test_runner.p...
build/android/test_runner.py:122: devil.env.android_sdk =
devil.env.android_sdk_config.AndroidSdkConfig(
On 2015/09/09 23:47:07, jbudorick wrote:
> Configuring devil's environment is intentionally ugly because it:
>  - shouldn't be done often, and
>  - shouldn't be easy to do on accident

Throwing some ideas: If the intention is that configs shouldn't easily change
after being set. I think, e.g., AndroidSdkConfig should return a named tuple
with all the config values already set, rather than an object with a hidden dict
and properties whose values might change between calls. With a named tuple
values can't be accidentally changed by clients or the environment. This also
tightens things up as we wouldn't be able to set values for non-existent
properties.

Now trying to _read_ from an unset devil.env.android_sdk should either throw an
exception or try to guess some sensible defaults *and fix them* at that time.

[aiolos (Not reviewing), 2015-09-10 12:25:50]

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

[aiolos (Not reviewing), 2015-09-10 12:25:50]

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
File build/android/devil/env/android_sdk_config.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:17: return
(self._configured.get('root_path')
You could use the dependency_manager instead. You would have your default paths
in a BaseConfig (initialized with a json file that just specified the
local_paths without any of the cloud_storage information). And depending on
whether the configured path locations were consistent across projects, you could
either have them proceed the default paths in the local_files list, or have a
(for example) telemetry-specific config that you only use as needed.

(It should already work for finding local paths, but you'd have to depend on
telemetry/catapult_base to use it currently. If that's an issue, I can probably
move it to catapult before Telemetry moves.)

It doesn't have any dependencies in it currently, but you can see the setup for
a global DependencyManager here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

And an example of what the json should look like in the doc string here:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/ca...

[jbudorick, 2015-09-12 02:31:05]

Rewrote the entire CL w/ dependency_manager.

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/__i...
File build/android/devil/env/__init__.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/__i...
build/android/devil/env/__init__.py:13: 
On 2015/09/10 at 09:45:48, perezju wrote:
> Maybe add some comments here explaining how/where these configs should be
set/changed? (With a remark that this probably shouldn't be done too often?)

obsolete

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
File build/android/devil/env/android_sdk_config.py (right):

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:10: DEVIL_ANDROID_SDK_ROOT =
'DEVIL_ANDROID_SDK_ROOT'
On 2015/09/10 at 09:45:48, perezju wrote:
> nit: it's not clear from the name if this is the _value_ of the SDK_ROOT or,
as it's actually the case, the _key_ used to look up the value within
environment vars.
> 
> Also, we probably don't need 'DEVIL' in the name of the constant (it's fine in
the name of the key).
> 
> maybe: ANDROID_SDK_ROOT_KEY = 'DEVIL_ANDROID_SDK_ROOT'?

obsolete

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:17: return
(self._configured.get('root_path')
On 2015/09/10 at 12:25:50, aiolos wrote:
> You could use the dependency_manager instead. You would have your default
paths in a BaseConfig (initialized with a json file that just specified the
local_paths without any of the cloud_storage information). And depending on
whether the configured path locations were consistent across projects, you could
either have them proceed the default paths in the local_files list, or have a
(for example) telemetry-specific config that you only use as needed.
> 
> (It should already work for finding local paths, but you'd have to depend on
telemetry/catapult_base to use it currently. If that's an issue, I can probably
move it to catapult before Telemetry moves.)
> 
> It doesn't have any dependencies in it currently, but you can see the setup
for a global DependencyManager here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> And an example of what the json should look like in the doc string here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/ca...

Done, and the current dependency on telemetry/catapult_base is fine.

dependency_manager is cool (and is a marked improvement over what I had, imo),
but it seems to have a couple rough edges when it comes to supporting cases like
this:
 - it seems like it's a bit too coupled to binary_manager: I ran into the
platform_architecture assert that seemed like it was a requirement set by
binary_manager but enforced by dependency_manager
 - no support for dynamic configuration: BaseConfig is determined to load
configurations from an existing JSON file, so I had to write a json to a
tempfile after processing our command-line flags.

https://codereview.chromium.org/1316413003/diff/1/build/android/devil/env/and...
build/android/devil/env/android_sdk_config.py:18: or
os.environ.get(DEVIL_ANDROID_SDK_ROOT))
On 2015/09/10 at 09:45:48, perezju wrote:
> On 2015/09/10 00:54:16, mikecase wrote:
> > I think this could be a little confusing since these configs are stored as
> > global variables in env/__init__.py and its unclear where and if they are
set
> > (or even that you have to set them).
> > 
> > Maybe raise an exception if both its not configured and the environment
variable
> > isn't set to anything? Could make it easier to debug, but it might make the
code
> > overly confusing.
> 
> +1 to raising an exception. Either that or providing some other sensible
default other than |None|.

need to verify how the new dependency_manager implementation handles this case,
I'll come back to this.

[perezju, 2015-09-14 09:13:58]

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
File build/android/devil/android/battery_utils_test.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
build/android/devil/android/battery_utils_test.py:23:
devil_env.InitializeForTests()
I don't like having to add this bit all over the place in all modules. What if I
want to import this module into another script, but use a different non-tests
config? What is the expected behavior if two modules imported by the same script
use different InitializeFor* methods? It sounds all very confusing.

I have a suggestion to get rid of this in devil_env.

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
build/android/devil/android/battery_utils_test.py:24:
sys.path.append(devil_env.instance.pymock_path)
suggestion nit: maybe rename "instance" to "config"?

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
build/android/devil/android/device_utils.py:45: 
is this missing the devil_env.InitializeForTests()?

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
build/android/devil/devil_env.py:23: def __init__(self, config_files):
Suggestion, make it something like:

class _Environment(object):
  def __init__(self):
    self._config = None

  def Initialize(self, config_files=None):
    if config_files is None:
      config_files = # use default devil_config.json
    # something like you have now in Initialize + __init__
    self._config = {}
    self._config['adb_path'] = ...
    # etc.

  def __getattr__(self, name):
    if self._config is None:
      self.Initialize() # uses default config
    if name in self._config:
      return self._config[name]
    else:
      raise AttributeError(
          '%r object has no attribute %r' % (type(self).__name__, name))

instance = _Environment()

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
build/android/devil/devil_env.py:39: build_tools_contents.sort()
if there are multiple build_tools, how do they typically look like? i.e. does
picking the last one after sorting always pick the most recent one?

https://codereview.chromium.org/1316413003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/test_runn...
build/android/test_runner.py:138: with tempfile.NamedTemporaryFile() as
devil_dynamic_config_file:
Could we hide this tempfile thing in devil_env?

[jbudorick, 2015-09-14 13:12:39]

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
File build/android/devil/android/battery_utils_test.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
build/android/devil/android/battery_utils_test.py:23:
devil_env.InitializeForTests()
On 2015/09/14 at 09:13:58, perezju wrote:
> I don't like having to add this bit all over the place in all modules. What if
I want to import this module into another script, but use a different non-tests
config? What is the expected behavior if two modules imported by the same script
use different InitializeFor* methods? It sounds all very confusing.

Note that this is only necessary for tests that import pymock.

> 
> I have a suggestion to get rid of this in devil_env.

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
File build/android/devil/android/device_utils.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/and...
build/android/devil/android/device_utils.py:45: 
On 2015/09/14 at 09:13:58, perezju wrote:
> is this missing the devil_env.InitializeForTests()?

nope

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
build/android/devil/devil_env.py:23: def __init__(self, config_files):
On 2015/09/14 at 09:13:58, perezju wrote:
> Suggestion, make it something like:
> 
> class _Environment(object):
>   def __init__(self):
>     self._config = None
> 
>   def Initialize(self, config_files=None):
>     if config_files is None:
>       config_files = # use default devil_config.json
>     # something like you have now in Initialize + __init__
>     self._config = {}
>     self._config['adb_path'] = ...
>     # etc.
> 
>   def __getattr__(self, name):
>     if self._config is None:
>       self.Initialize() # uses default config
>     if name in self._config:
>       return self._config[name]
>     else:
>       raise AttributeError(
>           '%r object has no attribute %r' % (type(self).__name__, name))
> 
> instance = _Environment()

Hm, this is an interesting idea. I'll try it & get back to you.

https://codereview.chromium.org/1316413003/diff/20001/build/android/devil/dev...
build/android/devil/devil_env.py:39: build_tools_contents.sort()
On 2015/09/14 at 09:13:58, perezju wrote:
> if there are multiple build_tools, how do they typically look like? i.e. does
picking the last one after sorting always pick the most recent one?

I've never seen multiple, but they're typically named either by SDK level, e.g.
"23.0.0", or by letter, e.g. "android-M". If there were multiple, we'd pick the
highest version, which is what I'd guess we'd want.

https://codereview.chromium.org/1316413003/diff/20001/build/android/test_runn...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1316413003/diff/20001/build/android/test_runn...
build/android/test_runner.py:138: with tempfile.NamedTemporaryFile() as
devil_dynamic_config_file:
On 2015/09/14 at 09:13:58, perezju wrote:
> Could we hide this tempfile thing in devil_env?

I need to talk to Kari about doing dynamic config natively in
dependency_manager, but in the meantime I can move it to devil_env.

[jbudorick, 2015-09-14 16:02:17]

Addressed perezju's comments. ptal

[perezju, 2015-09-14 16:15:06]

awesome, lgtm, just a couple of comments

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:60: config_files=[next_config_file.name] +
(config_files or []))
ugh, this is a somewhat ugly use of recursion... after a while I figured this
probably is because of the tempfile's, to open/close them properly.

nit: Maybe just add a comment explaining that's why we can't do a more simple
iterative implementation? And/or a TODO to avoid this if at some point the
dependency manager supports loading from dicts in addition to files?

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:106: raise AttributeError('devil environment
has no %r attribute', name)
This is not logging. I think here you really need: msg % params

[jbudorick, 2015-09-14 17:45:39]

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:60: config_files=[next_config_file.name] +
(config_files or []))
On 2015/09/14 at 16:15:06, perezju wrote:
> ugh, this is a somewhat ugly use of recursion... after a while I figured this
probably is because of the tempfile's, to open/close them properly.

Correct.

> 
> nit: Maybe just add a comment explaining that's why we can't do a more simple
iterative implementation? And/or a TODO to avoid this if at some point the
dependency manager supports loading from dicts in addition to files?

Will do.

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:106: raise AttributeError('devil environment
has no %r attribute', name)
On 2015/09/14 at 16:15:06, perezju wrote:
> This is not logging. I think here you really need: msg % params

O_O

fixed.

[jbudorick, 2015-09-15 23:41:00]

aiolos: ping

[aiolos (Not reviewing), 2015-09-16 00:16:27]

Sorry about that. I got back from France last night, and today has been
Telemetry design discussions and meetings. I'll do more than a cursory look
later tonight, but in the mean time. Do you want a config class that lets you
specify dependencies on the fly?

[jbudorick, 2015-09-16 00:19:04]

On 2015/09/16 at 00:16:27, aiolos wrote:
> Sorry about that. I got back from France last night, and today has been
Telemetry design discussions and meetings. I'll do more than a cursory look
later tonight, but in the mean time. Do you want a config class that lets you
specify dependencies on the fly?

Ah, didn't realize it was last night. Sorry.

Re dependencies on the fly: eventually, yes. It doesn't necessarily need to
happen prior to this CL, though, as I've got it working in an easily-removable
way up in devil_env.py.

[aiolos (Not reviewing), 2015-09-16 03:37:54]

The assert that you're running into in the dependency_manager was there for
landing to make sure things were working for the binary_manager on all
platforms, and can just be removed at this point.

lgtm

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:60: config_files=[next_config_file.name] +
(config_files or []))
On 2015/09/14 17:45:39, jbudorick wrote:
> On 2015/09/14 at 16:15:06, perezju wrote:
> > ugh, this is a somewhat ugly use of recursion... after a while I figured
this
> probably is because of the tempfile's, to open/close them properly.
> 
> Correct.
> 
> > 
> > nit: Maybe just add a comment explaining that's why we can't do a more
simple
> iterative implementation? And/or a TODO to avoid this if at some point the
> dependency manager supports loading from dicts in addition to files?
> 
> Will do.

I designed the dependency_manager to allow subclassing of base config to allow
for more flexibility, and can easily add one that allows you to use a dictionary
instead of a json file. Feel free to block on that or land as-is.

[jbudorick, 2015-09-18 15:08:19]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-09-18 15:08:20]

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
File build/android/devil/devil_env.py (right):

https://codereview.chromium.org/1316413003/diff/60001/build/android/devil/dev...
build/android/devil/devil_env.py:60: config_files=[next_config_file.name] +
(config_files or []))
On 2015/09/16 at 03:37:54, aiolos wrote:
> On 2015/09/14 17:45:39, jbudorick wrote:
> > On 2015/09/14 at 16:15:06, perezju wrote:
> > > ugh, this is a somewhat ugly use of recursion... after a while I figured
this
> > probably is because of the tempfile's, to open/close them properly.
> > 
> > Correct.
> > 
> > > 
> > > nit: Maybe just add a comment explaining that's why we can't do a more
simple
> > iterative implementation? And/or a TODO to avoid this if at some point the
> > dependency manager supports loading from dicts in addition to files?
> > 
> > Will do.
> 
> I designed the dependency_manager to allow subclassing of base config to allow
for more flexibility, and can easily add one that allows you to use a dictionary
instead of a json file. Feel free to block on that or land as-is.

Landing as-is. I'll send you a follow-up CL.

[jbudorick, 2015-09-18 15:08:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
perezju@chromium.org, aiolos@chromium.org
Link to the patchset: https://codereview.chromium.org/1316413003/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-09-18 15:08:54]

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

[commit-bot: I haz the power, 2015-09-18 15:19:51]

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

[commit-bot: I haz the power, 2015-09-18 15:19:52]

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...)

[jbudorick, 2015-09-18 17:33:20]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-09-18 17:33:21]

The patchset sent to the CQ was uploaded after l-g-t-m from aiolos@chromium.org,
perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/1316413003/#ps160001
(title: "fix linux presubmit, round 3")

[commit-bot: I haz the power, 2015-09-18 17:33:55]

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

[commit-bot: I haz the power, 2015-09-18 19:50:09]

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

[commit-bot: I haz the power, 2015-09-18 19:50:09]

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...)

[jbudorick, 2015-09-29 13:28:58]

aiolos: ptal

https://codereview.chromium.org/1316413003/diff/180001/build/android/test_run...
File build/android/test_runner.py (right):

https://codereview.chromium.org/1316413003/diff/180001/build/android/test_run...
build/android/test_runner.py:132: devil_dynamic_config =
devil_env.GenerateDynamicConfig(devil_dynamic_deps)
As written, the dynamic config takes precedence over the static config, so a
user with out/Debug/... and custom_output_dir/Debug/... that passes
--output-directory=custom_output_dir/Debug will correctly use the binaries from
custom_output_dir/Debug/...

https://codereview.chromium.org/1316413003/diff/180001/tools/telemetry/catapu...
File tools/telemetry/catapult_base/dependency_manager/dependency_manager.py
(left):

https://codereview.chromium.org/1316413003/diff/180001/tools/telemetry/catapu...
tools/telemetry/catapult_base/dependency_manager/dependency_manager.py:92:
logging.error(
I removed both of these logging messages because:
 - in the case where this is a problem, the exception should handle the logging
(whether at the point where it's caught or if it propagates all the way through
uncaught)
 - in the case where it isn't a problem, logging will only confuse devs

[jbudorick, 2015-09-30 12:24:37]

aiolos: ping

(the red linux_android_rel_ng runs are from an unrelated failure in the webview
instrumentation tests that has been resolved.)

[aiolos (Not reviewing), 2015-10-02 22:44:47]

Still LGTM, regardless of whether you move over to using specific file locations
in devil_env instead of android_sdk_build_tools_path like discussed offline.

[jbudorick, 2015-10-05 15:48:36]

On 2015/10/02 22:44:47, aiolos wrote:
> Still LGTM, regardless of whether you move over to using specific file
locations
> in devil_env instead of android_sdk_build_tools_path like discussed offline.

I (finally) got a chance to look at your suggestion. I opted against
implementing it for now because of the directory structure of
$(ANDROID_SDK_ROOT)/sdk/build-tools. Specifically, the directory inside is named
according the the current SDK level -- e.g., currently, it's
$(ANDROID_SDK_ROOT)/sdk/build-tools/23.0.0/{aapt,dexdump,split-select} -- and
that directory name changes in each release.

[jbudorick, 2015-10-05 15:48:48]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-10-05 15:48:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
perezju@chromium.org, aiolos@chromium.org
Link to the patchset: https://codereview.chromium.org/1316413003/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-05 15:49:32]

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

[aiolos (Not reviewing), 2015-10-05 15:58:46]

On 2015/10/05 15:48:36, jbudorick wrote:
> On 2015/10/02 22:44:47, aiolos wrote:
> > Still LGTM, regardless of whether you move over to using specific file
> locations
> > in devil_env instead of android_sdk_build_tools_path like discussed offline.
> 
> I (finally) got a chance to look at your suggestion. I opted against
> implementing it for now because of the directory structure of
> $(ANDROID_SDK_ROOT)/sdk/build-tools. Specifically, the directory inside is
named
> according the the current SDK level -- e.g., currently, it's
> $(ANDROID_SDK_ROOT)/sdk/build-tools/23.0.0/{aapt,dexdump,split-select} -- and
> that directory name changes in each release.

sgtm.

[commit-bot: I haz the power, 2015-10-05 18:31:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-10-05 18:32:04]

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

[jbudorick, 2015-10-05 21:41:00]

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

The reason for reverting is: breaks on non-default output directories..