[Android] Remove android_commands uses from build/android/.

[Android] Remove android_commands uses from build/android/.

This also adds filtering to AdbWrapper.GetDevices().

BUG=267773
Committed: https://crrev.com/55d1762e76ebe2494c882594c620993419d97b98
Cr-Commit-Position: refs/heads/master@{#325254}

[jbudorick, 2015-04-14 02:30:24]

jbudorick@chromium.org changed reviewers:
+ klundberg@chromium.org, perezju@chromium.org

[jbudorick, 2015-04-14 02:31:22]

On 2015/04/14 02:30:24, jbudorick wrote:

Obviously, this does not remove android_commands uses from subdirectories of
build/android/. I plan to do that in a separate CL.

[perezju, 2015-04-14 12:39:11]

just a few comments

https://codereview.chromium.org/1088793002/diff/1/build/android/enable_assert...
File build/android/enable_asserts.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/enable_assert...
build/android/enable_asserts.py:29: if options.set_asserts == None:
maybe make the options required and mutually exclusive?

https://codereview.chromium.org/1088793002/diff/1/build/android/host_heartbea...
File build/android/host_heartbeat.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/host_heartbea...
build/android/host_heartbeat.py:26: 'touch /sdcard/host_heartbeat')
pass command as list, and check_return=True?

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:171: returned list.
I feel this is a bit over-complicated, I would either:

1. take as filter a single function, f(adb, desc) -> bool, and filter using
that; or even better

2. do not provide the option at all, store the description string into the adb
object, and let the caller do the filtering itself.

[jbudorick, 2015-04-14 15:29:58]

https://codereview.chromium.org/1088793002/diff/1/build/android/enable_assert...
File build/android/enable_asserts.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/enable_assert...
build/android/enable_asserts.py:29: if options.set_asserts == None:
On 2015/04/14 12:39:10, perezju wrote:
> maybe make the options required and mutually exclusive?

I'm going to regret making a behavioral change in this CL, but done.

https://codereview.chromium.org/1088793002/diff/1/build/android/host_heartbea...
File build/android/host_heartbeat.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/host_heartbea...
build/android/host_heartbeat.py:26: 'touch /sdcard/host_heartbeat')
On 2015/04/14 12:39:10, perezju wrote:
> pass command as list, and check_return=True?

same, done.

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:171: returned list.
On 2015/04/14 12:39:10, perezju wrote:
> I feel this is a bit over-complicated, I would either:
> 
> 1. take as filter a single function, f(adb, desc) -> bool, and filter using
> that; or even better
> 
> 2. do not provide the option at all, store the description string into the adb
> object, and let the caller do the filtering itself.

after looking at this, I think we already have this info available via
AdbWrapper.GetState(), so I can eliminate the description.

I've gone back and forth on this. We _already_ filter on state == 'device', and
very, very few clients want blacklisted devices. Passing back a list of
everything is not what we want to do.

[perezju, 2015-04-15 09:25:45]

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:171: returned list.
On 2015/04/14 15:29:58, jbudorick wrote:
> On 2015/04/14 12:39:10, perezju wrote:
> > I feel this is a bit over-complicated, I would either:
> > 
> > 1. take as filter a single function, f(adb, desc) -> bool, and filter using
> > that; or even better
> > 
> > 2. do not provide the option at all, store the description string into the
adb
> > object, and let the caller do the filtering itself.
> 
> after looking at this, I think we already have this info available via
> AdbWrapper.GetState(), so I can eliminate the description.
> 
> I've gone back and forth on this. We _already_ filter on state == 'device',
and
> very, very few clients want blacklisted devices. Passing back a list of
> everything is not what we want to do.

I'm not sure about this. AdbWrapper is meant to be "a thin wrapper around the
adb interface", and having knowledge about a blacklist file on some particular
location of a chrome checkout seems to go a bit too far.

Also, as implemented, it seems that this would silently skip devices not
matching the filter (which may or may not be what we want on different
scenarios).

Having some ready made filters sounds like a great idea, but I would pass the
responsibility of what filters to use to the caller. Also, when developing or
testing things locally, the blacklist is kind of annoying, I had to disable it
once when experimenting with provision devices for the android test.

Just my final 2 cents, but I'm ok with whatever you decide in the end.

(Also if you decide to keep this, the docstring should document the default
behavior.)

https://codereview.chromium.org/1088793002/diff/20001/build/android/pylib/dev...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/20001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:184: devices = [AdbWrapper(line[0])
for line in lines if len(line) == 2]
nit: not that it makes much of a difference, but you can avoid building the
intermediate lists by turning them into generators, i.e.:

lines = (line.split() ...)
devuces = (AdbWrapper ...)

[jbudorick, 2015-04-15 13:06:59]

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:171: returned list.
On 2015/04/15 09:25:44, perezju wrote:
> On 2015/04/14 15:29:58, jbudorick wrote:
> > On 2015/04/14 12:39:10, perezju wrote:
> > > I feel this is a bit over-complicated, I would either:
> > > 
> > > 1. take as filter a single function, f(adb, desc) -> bool, and filter
using
> > > that; or even better
> > > 
> > > 2. do not provide the option at all, store the description string into the
> adb
> > > object, and let the caller do the filtering itself.
> > 
> > after looking at this, I think we already have this info available via
> > AdbWrapper.GetState(), so I can eliminate the description.
> > 
> > I've gone back and forth on this. We _already_ filter on state == 'device',
> and
> > very, very few clients want blacklisted devices. Passing back a list of
> > everything is not what we want to do.
> 
> I'm not sure about this. AdbWrapper is meant to be "a thin wrapper around the
> adb interface", and having knowledge about a blacklist file on some particular
> location of a chrome checkout seems to go a bit too far.
> 

Yeah, I thought about this too and am still not sure. I'll try it with no
default filters and see how it looks.

I suspect the implementation of the blacklist will have to change when we
extract it anyway, so think of it less in terms of this knowing about the
current implementation of the blacklist and more about it knowing about the
blacklist at all.

> Also, as implemented, it seems that this would silently skip devices not
> matching the filter (which may or may not be what we want on different
> scenarios).

It does, and that is bad.

> 
> Having some ready made filters sounds like a great idea, but I would pass the
> responsibility of what filters to use to the caller. Also, when developing or
> testing things locally, the blacklist is kind of annoying, I had to disable it
> once when experimenting with provision devices for the android test.

wit the current implementation, just rm -rf out/bad_devices.json

but yes, I've hit the same thing.

> 
> Just my final 2 cents, but I'm ok with whatever you decide in the end.
> 
> (Also if you decide to keep this, the docstring should document the default
> behavior.)

https://codereview.chromium.org/1088793002/diff/20001/build/android/pylib/dev...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1088793002/diff/20001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:184: devices = [AdbWrapper(line[0])
for line in lines if len(line) == 2]
On 2015/04/15 09:25:44, perezju wrote:
> nit: not that it makes much of a difference, but you can avoid building the
> intermediate lists by turning them into generators, i.e.:
> 
> lines = (line.split() ...)
> devuces = (AdbWrapper ...)

I think we should encourage using generators over lists in single-use scenarios.
Thanks for calling these out, I'll make the switch.

[perezju, 2015-04-15 13:25:40]

On 2015/04/15 13:06:59, jbudorick wrote:
> On 2015/04/15 09:25:44, perezju wrote:
> > testing things locally, the blacklist is kind of annoying, I had to disable
it
> > once when experimenting with provision devices for the android test.
> 
> wit the current implementation, just rm -rf out/bad_devices.json

That's what I meant by "I had to disable it" :P

[jbudorick, 2015-04-15 14:36:57]

GetDevices() -> Devices() (since it's adb devices, not adb get-devices) and
default filters removed. wdyt?

[perezju, 2015-04-15 14:52:28]

lgtm w/nit

https://codereview.chromium.org/1088793002/diff/40001/build/android/pylib/dev...
File build/android/pylib/device/device_filter.py (right):

https://codereview.chromium.org/1088793002/diff/40001/build/android/pylib/dev...
build/android/pylib/device/device_filter.py:9: def DefaultFilters():
nit: missing docstrings

[jbudorick, 2015-04-15 15:14:33]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-04-15 15:14:34]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/1088793002/#ps60001
(title: "docstrings for device_filter.py")

[commit-bot: I haz the power, 2015-04-15 15:15:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088793002/60001

[commit-bot: I haz the power, 2015-04-15 16:11:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-15 16:12:57]

Patchset 4 (id:??) landed as
https://crrev.com/55d1762e76ebe2494c882594c620993419d97b98
Cr-Commit-Position: refs/heads/master@{#325254}

[jbudorick, 2015-04-15 18:22:29]

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

The reason for reverting is: Broken for AdbWrapper.Devices(filters=None).