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

build/android/pylib/device/adb_wrapper.py

 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """This module wraps Android's adb tool.
 
 This is a thin wrapper around the adb interface. Any additional complexity
 should be delegated to a higher level (ex. DeviceUtils).
 """
 
 import collections
 import errno
 import logging
 import os
 
 from pylib import cmd_helper
 from pylib import constants
 from pylib.device import decorators
 from pylib.device import device_errors
+from pylib.device import device_filter
 from pylib.utils import timeout_retry
 
 
 _DEFAULT_TIMEOUT = 30
 _DEFAULT_RETRIES = 2
 
 
 def _VerifyLocalFileExists(path):
   """Verifies a local file exists.
 
@@ skipping 121 matching lines @@
   @classmethod
   def KillServer(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES):
     cls._RunAdbCmd(['kill-server'], timeout=timeout, retries=retries)
 
   @classmethod
   def StartServer(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES):
     # CPU affinity is used to reduce adb instability http://crbug.com/268450
     cls._RunAdbCmd(['start-server'], timeout=timeout, retries=retries,
                    cpu_affinity=0)
 
-  # TODO(craigdh): Determine the filter criteria that should be supported.
   @classmethod
-  def GetDevices(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES):
+  def GetDevices(cls, filters=None, timeout=_DEFAULT_TIMEOUT,
+                 retries=_DEFAULT_RETRIES):
     """Get the list of active attached devices.
 
     Args:
+      filters: (optional) A list of binary functions that take an AdbWrapper
+          instance and a string description. Any device for which all provided
+          filter functions do not return True will not be included in the
+          returned list.

[perezju, 2015/04/14 12:39:10]

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]

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:44]

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

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

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.
It does, and that is bad.
wit the current implementation, just rm -rf out/bad_devices.json

but yes, I've hit the same thing.

       timeout: (optional) Timeout per try in seconds.
       retries: (optional) Number of retries to attempt.
 
     Yields:
       AdbWrapper instances.
     """
+    if filters is None:
+      filters = device_filter.DefaultFilters()
+
     output = cls._RunAdbCmd(['devices'], timeout=timeout, retries=retries)
     lines = [line.split() for line in output.splitlines()]
-    return [AdbWrapper(line[0]) for line in lines
-            if len(line) == 2 and line[1] == 'device']
+
+    devices_and_descriptions = [
+        (AdbWrapper(line[0]), line[1]) for line in lines if len(line) == 2]
+
+    return [dev for dev, desc in devices_and_descriptions
+            if all(f(dev, desc) for f in filters)]
 
   def GetDeviceSerial(self):
     """Gets the device serial number associated with this object.
 
     Returns:
       Device serial number as a string.
     """
     return self._device_serial
 
   def Push(self, local, remote, timeout=60*5, retries=_DEFAULT_RETRIES):
@@ skipping 336 matching lines @@
     """Restarts the adbd daemon with root permissions, if possible.
 
     Args:
       timeout: (optional) Timeout per try in seconds.
       retries: (optional) Number of retries to attempt.
     """
     output = self._RunDeviceAdbCmd(['root'], timeout, retries)
     if 'cannot' in output:
       raise device_errors.AdbCommandFailedError(
           ['root'], output, device_serial=self._device_serial)