GetAttachedDevices() should not include bad devices listed in .bad_devices file.

GetAttachedDevices() should not include bad devices listed in .bad_devices file.

BUG=334724
TBR=craigdh@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254905

[craigdh, 2014-02-27 23:53:07]

https://codereview.chromium.org/181433020/diff/1/build/android/pylib/android_...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/181433020/diff/1/build/android/pylib/android_...
build/android/pylib/android_commands.py:131: bad_devices = f.read().splitlines()
where do you open f?

[navabi, 2014-02-28 00:01:51]

Thanks Craig. I still need to test. I'm WFH today and can't get adb over tcp
working. I'll test tomorrow when I get in, but would appreciate a look.

https://codereview.chromium.org/181433020/diff/1/build/android/pylib/android_...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/181433020/diff/1/build/android/pylib/android_...
build/android/pylib/android_commands.py:131: bad_devices = f.read().splitlines()
On 2014/02/27 23:53:08, craigdh wrote:
> where do you open f?

oops. fixed.

[bulach, 2014-02-28 10:13:33]

good stuff! lgtm, but make everybody else happy too :)

just some small suggestions:

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:128: bad_devices_file =
os.path.join(constants.GetOutDirectory(), '.bad_devices')
glad to see this!
some questions:
1) is this file going to be maintained by bb_device_status_check?
2) how about making it visible? the "." makes it a bit harder to find..
3) also, to avoid any possible conflict, how about "bad_devices.pylib", or
something? ideally, define it constants.py so that the other side maintaining it
will use the same value..

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:133: devices = [device for device in
devices if device not in bad_devices]
nit: could remove 129, and indent this, something like:

if os.path.exists(bad_devices_file):
  with with open(bad_devices_file) as f:
    bad_devices = f.read().splitlines()
  logging.info('Avoiding bad devices %s', ' '.join(bad_devices))
  devices = filter(lambda d: d not in bad_devices, devices)

[navabi, 2014-02-28 20:46:52]

Thanks Marcus!

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:128: bad_devices_file =
os.path.join(constants.GetOutDirectory(), '.bad_devices')
On 2014/02/28 10:13:33, bulach wrote:
> glad to see this!
> some questions:
> 1) is this file going to be maintained by bb_device_status_check?
> 2) how about making it visible? the "." makes it a bit harder to find..
> 3) also, to avoid any possible conflict, how about "bad_devices.pylib", or
> something? ideally, define it constants.py so that the other side maintaining
it
> will use the same value..

Good suggestions.
1) It will be maintained by bb_device_status_check and full_deploy. Right now,
only full_deploy is writing this file as inspired by the original bug. That
raises a good point. Here is what I suggest: bb_device_status_check removes
devices from the file (i.e. resets since the list is a per build list).
bb_device_status_check and subsequent steps (e.g. full_deploy) can add devices
to that file.

2/3) I made it a json file and defined the file in constants.py.

https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:133: devices = [device for device in
devices if device not in bad_devices]
On 2014/02/28 10:13:33, bulach wrote:
> nit: could remove 129, and indent this, something like:
> 
> if os.path.exists(bad_devices_file):
>   with with open(bad_devices_file) as f:
>     bad_devices = f.read().splitlines()
>   logging.info('Avoiding bad devices %s', ' '.join(bad_devices))
>   devices = filter(lambda d: d not in bad_devices, devices)

Done(ish). I like the remove 129/indent, but I prefer the [device for device in
devices ...] to filtering with a lambda. I think it's more readable.

[navabi, 2014-02-28 22:29:58]

On 2014/02/28 20:46:52, navabi wrote:
> Thanks Marcus!
> 
>
https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
> build/android/pylib/android_commands.py:128: bad_devices_file =
> os.path.join(constants.GetOutDirectory(), '.bad_devices')
> On 2014/02/28 10:13:33, bulach wrote:
> > glad to see this!
> > some questions:
> > 1) is this file going to be maintained by bb_device_status_check?
> > 2) how about making it visible? the "." makes it a bit harder to find..
> > 3) also, to avoid any possible conflict, how about "bad_devices.pylib", or
> > something? ideally, define it constants.py so that the other side
maintaining
> it
> > will use the same value..
> 
> Good suggestions.
> 1) It will be maintained by bb_device_status_check and full_deploy. Right now,
> only full_deploy is writing this file as inspired by the original bug. That
> raises a good point. Here is what I suggest: bb_device_status_check removes
> devices from the file (i.e. resets since the list is a per build list).
> bb_device_status_check and subsequent steps (e.g. full_deploy) can add devices
> to that file.
> 
> 2/3) I made it a json file and defined the file in constants.py.
> 
>
https://codereview.chromium.org/181433020/diff/20001/build/android/pylib/andr...
> build/android/pylib/android_commands.py:133: devices = [device for device in
> devices if device not in bad_devices]
> On 2014/02/28 10:13:33, bulach wrote:
> > nit: could remove 129, and indent this, something like:
> > 
> > if os.path.exists(bad_devices_file):
> >   with with open(bad_devices_file) as f:
> >     bad_devices = f.read().splitlines()
> >   logging.info('Avoiding bad devices %s', ' '.join(bad_devices))
> >   devices = filter(lambda d: d not in bad_devices, devices)
> 
> Done(ish). I like the remove 129/indent, but I prefer the [device for device
in
> devices ...] to filtering with a lambda. I think it's more readable.

PTAL with https://chrome-internal-review.googlesource.com/#/c/155315/

[bulach, 2014-03-03 11:24:05]

lgtm, nice, thanks!

[bulach, 2014-03-03 11:25:54]

ops, sorry, still lgtm, but you'd probably want to add the "extend bad devices"
at the android_commands.py level rather than downstream, I bet this will be
useful for other bots soon :)

[navabi, 2014-03-03 18:27:27]

Thanks Marcus for the good suggestions. PTAL along with
https://chrome-internal-review.googlesource.com/#/c/155315/

[bulach, 2014-03-04 10:26:58]

lgtm, good stuff, thanks!

[navabi, 2014-03-04 17:48:02]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2014-03-04 17:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/navabi@google.com/181433020/70001

[commit-bot: I haz the power, 2014-03-04 18:17:07]

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

[commit-bot: I haz the power, 2014-03-04 18:17:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[navabi, 2014-03-04 21:02:20]

The CQ bit was checked by navabi@google.com

[commit-bot: I haz the power, 2014-03-04 21:05:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/navabi@google.com/181433020/70001

[commit-bot: I haz the power, 2014-03-05 00:48:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/navabi@google.com/181433020/70001

[commit-bot: I haz the power, 2014-03-05 01:35:48]

Change committed as 254905