[Android] Rework device filtering and add DeviceUtils.HealthyDevices.

[Android] Rework device filtering and add DeviceUtils.HealthyDevices.

BUG=267773
Committed: https://crrev.com/119e45757ba123428de29eadb01bda22f322e788
Cr-Commit-Position: refs/heads/master@{#326825}

[jbudorick, 2015-04-21 22:22:15]

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

[perezju, 2015-04-22 08:34:25]

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

https://codereview.chromium.org/1101603002/diff/1/build/android/pylib/device/...
build/android/pylib/device/device_filter.py:61: serial_re: The regex to match
against a device's serial number.
maybe we could be nice and compile the pattern if serial_re is a str?

https://codereview.chromium.org/1101603002/diff/1/build/android/pylib/device/...
build/android/pylib/device/device_filter.py:73: HardwareFilter =
SerialFilter(re.compile('^[a-zA-Z0-9_:.-]+$'))
it looks like everything matched by the EmulatorFilter is also matched by the
HardwareFilter, is that intended?

[jbudorick, 2015-04-23 01:56:08]

I didn't address either of your comments in this CL. Instead, I'm looking for
your opinion on possible changes:
 - in adb_install_apk.py and adb_wrapper.py, the raw "hypothetical" you
mentioned. I saved the blacklist beforehand because, otherwise, we open and read
the blacklist file for each device.
 - in adb_reverse_forwarder.py and device_utils.py, a possible convenience
function for the most common case of AdbWrapper.Devices, and a closer analogue
of AndroidCommands.GetAttachedDevices.

wdyt?

[perezju, 2015-04-23 10:48:50]

On 2015/04/23 01:56:08, jbudorick wrote:
> I didn't address either of your comments in this CL. Instead, I'm looking for
> your opinion on possible changes:
>  - in adb_install_apk.py and adb_wrapper.py, the raw "hypothetical" you
> mentioned. I saved the blacklist beforehand because, otherwise, we open and
read
> the blacklist file for each device.
>  - in adb_reverse_forwarder.py and device_utils.py, a possible convenience
> function for the most common case of AdbWrapper.Devices, and a closer analogue
> of AndroidCommands.GetAttachedDevices.
> 
> wdyt?

Ah, I replied on the other thread before reading this changes.

- The idea of HealthyDevices sgtm, this also avoids having to do magic caching
of
  the blacklist (as I suggested elsewhere) and that's probably a good thing.

- I still think it's a good idea to keep a "only_ready" option defaulting to
True
  in AdbWrapper.Devices. Mostly because filtering by "device/ready" is the
  absolutely most common thing ever, and we get it for free after running
  'adb devices' (with no extra adb calls, and without going too far from being a
  "thin layer" around adb).

This is looking very good!

[jbudorick, 2015-04-23 22:52:40]

On 2015/04/23 10:48:50, perezju wrote:
> On 2015/04/23 01:56:08, jbudorick wrote:
> > I didn't address either of your comments in this CL. Instead, I'm looking
for
> > your opinion on possible changes:
> >  - in adb_install_apk.py and adb_wrapper.py, the raw "hypothetical" you
> > mentioned. I saved the blacklist beforehand because, otherwise, we open and
> read
> > the blacklist file for each device.
> >  - in adb_reverse_forwarder.py and device_utils.py, a possible convenience
> > function for the most common case of AdbWrapper.Devices, and a closer
analogue
> > of AndroidCommands.GetAttachedDevices.
> > 
> > wdyt?
> 
> Ah, I replied on the other thread before reading this changes.
> 
> - The idea of HealthyDevices sgtm, this also avoids having to do magic caching
> of
>   the blacklist (as I suggested elsewhere) and that's probably a good thing.
> 
> - I still think it's a good idea to keep a "only_ready" option defaulting to
> True
>   in AdbWrapper.Devices. Mostly because filtering by "device/ready" is the
>   absolutely most common thing ever, and we get it for free after running
>   'adb devices' (with no extra adb calls, and without going too far from being
a
>   "thin layer" around adb).
> 
> This is looking very good!

HealthyDevices makes an only_ready option unnecessary.

Also, device_filter and the filters kwarg are gone.

[perezju, 2015-04-24 08:30:31]

lgtm with a couple of nits and an optional comment.

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

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:167: # TODO(jbudorick): Remove this
function once no more clients are using it.
nit: maybe log a warning?

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:178: in the returned list.
nit: if option is gone, also remove docstring

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:1588: if adb.is_ready and not
adb.GetDeviceSerial() in blacklist]
Having this function was such a great idea! Much cleaner usage everywhere.

Optional comment: I would still argue for
- moving the "is_ready" check into AdbWrapper.Devices, if only for efficiency
reasons (each "is_ready" costs an extra adb call here).
- not sure if we should also log info/warning on blacklisted devices?

[jbudorick, 2015-04-24 15:59:41]

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

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:167: # TODO(jbudorick): Remove this
function once no more clients are using it.
On 2015/04/24 08:30:31, perezju wrote:
> nit: maybe log a warning?

I will be converting the few remaining callers of this function within the next
few days, so I'm not sure a warning is necessary.

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/adb_wrapper.py:178: in the returned list.
On 2015/04/24 08:30:31, perezju wrote:
> nit: if option is gone, also remove docstring

whoops, done.

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1101603002/diff/60001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:1588: if adb.is_ready and not
adb.GetDeviceSerial() in blacklist]
On 2015/04/24 08:30:31, perezju wrote:
> Having this function was such a great idea! Much cleaner usage everywhere.
> 
> Optional comment: I would still argue for
> - moving the "is_ready" check into AdbWrapper.Devices, if only for efficiency
> reasons (each "is_ready" costs an extra adb call here).

done

> - not sure if we should also log info/warning on blacklisted devices?

We definitely should. Done.

[jbudorick, 2015-04-24 15:59:47]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-04-24 15:59:47]

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/1101603002/#ps80001
(title: "perezju comments")

[commit-bot: I haz the power, 2015-04-24 16:00:20]

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

[commit-bot: I haz the power, 2015-04-24 17:20:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-24 17:21:07]

Patchset 5 (id:??) landed as
https://crrev.com/119e45757ba123428de29eadb01bda22f322e788
Cr-Commit-Position: refs/heads/master@{#326825}