[Android] Convert to DeviceUtils versions of IsOnline, HasRoot, and EnableRoot.

[Android] Convert to DeviceUtils versions of IsOnline, HasRoot, and EnableRoot.

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

[jbudorick, 2014-05-14 19:20:47]

+cjhopman for b/a/gyp/*

[craigdh, 2014-05-14 20:33:08]

https://codereview.chromium.org/285143002/diff/1/build/android/gyp/util/build...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/gyp/util/build...
build/android/gyp/util/build_device.py:70: logging.getLogger().enabled = True
I believe you meant disabled = True here

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/decorators.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/decorators.py:18:
sys.path.append(os.path.join(constants.DIR_SOURCE_ROOT, 'third_party',
if this is temporary please add a TODO saying when it should be removed

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:106: raise
device_errors.DeviceUnreachableError('Unexpected device type?')
This error message is confusing (is my physical device the wrong type?). Maybe
"Unsupported type passed for argument 'device'". This should really be a
standard ValueError as this is programmer error.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:111: '_default_timeout',
'_default_retries')
I think it would be fine to default to these values, I don't know that being
explicit is particularly useful here and it adds a bit of noise. Just my two
cents.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
File build/android/pylib/valgrind_tools.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
build/android/pylib/valgrind_tools.py:140: pass
I really dislike this. Why are exceptions so often being suppressed for
EnableRoot? If it's conditional let's check the condition and only call enable
root if it should succeed.

[jbudorick, 2014-05-14 21:02:00]

https://codereview.chromium.org/285143002/diff/1/build/android/gyp/util/build...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/gyp/util/build...
build/android/gyp/util/build_device.py:70: logging.getLogger().enabled = True
On 2014/05/14 20:33:08, craigdh wrote:
> I believe you meant disabled = True here

Done.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/decorators.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/decorators.py:18:
sys.path.append(os.path.join(constants.DIR_SOURCE_ROOT, 'third_party',
On 2014/05/14 20:33:08, craigdh wrote:
> if this is temporary please add a TODO saying when it should be removed

Done.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:106: raise
device_errors.DeviceUnreachableError('Unexpected device type?')
On 2014/05/14 20:33:08, craigdh wrote:
> This error message is confusing (is my physical device the wrong type?). Maybe
> "Unsupported type passed for argument 'device'". This should really be a
> standard ValueError as this is programmer error.

Done.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:111: '_default_timeout',
'_default_retries')
On 2014/05/14 20:33:08, craigdh wrote:
> I think it would be fine to default to these values, I don't know that being
> explicit is particularly useful here and it adds a bit of noise. Just my two
> cents.

Holding for now, but I'm interested to hear what the others have to say about
this. I don't really have an opinion either way. Doing it explicitly seems a bit
more clear, but it also is a bit noisier.

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
File build/android/pylib/valgrind_tools.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
build/android/pylib/valgrind_tools.py:140: pass
On 2014/05/14 20:33:08, craigdh wrote:
> I really dislike this. Why are exceptions so often being suppressed for
> EnableRoot? If it's conditional let's check the condition and only call enable
> root if it should succeed.

My default stance for this series of interface switches was to not change
behavior at all. I'm not necessarily opposed to doing so on an individual basis,
but it does make the landing a bit more risky.

I don't think we have a way in either the old or the new api of checking whether
or not enable root "should" succeed, although I basically did it in adb shell in
device_utils_test.py.

[craigdh, 2014-05-14 21:21:11]

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
File build/android/pylib/valgrind_tools.py (right):

https://codereview.chromium.org/285143002/diff/1/build/android/pylib/valgrind...
build/android/pylib/valgrind_tools.py:140: pass
At the very very very least log the exception so it'll show up if (when) someone
goes debugging. And add TODOs to all of them.

[frankf, 2014-05-15 01:27:58]

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:116: timeout: An integer containing
the number of seconds to wait for the
DRY applies to docstrings as well ;)

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:128: """ Checks whether or not adbd
has root priveleges.
typo

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:153: if not
self.old_interface.EnableAdbRoot():
Why can't you inline EnableAdbRoot here?

[jbudorick, 2014-05-15 05:03:54]

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:116: timeout: An integer containing
the number of seconds to wait for the
On 2014/05/15 01:27:59, frankf wrote:
> DRY applies to docstrings as well ;)

"An integer containing the seconds to wait ...?" I'm not sure about that. Sounds
a little weird.

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:153: if not
self.old_interface.EnableAdbRoot():
On 2014/05/15 01:27:59, frankf wrote:
> Why can't you inline EnableAdbRoot here?

What exactly do you mean by "inline" in a python context? Moving the
AndroidCommands implementation in here?

[Primiano Tucci (use gerrit), 2014-05-15 12:39:08]

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
build/android/gyp/util/build_device.py:68: class LoggingDisabled(object):
Is there a specific reason for having all this boilerplate (i.e. the "with"
LoggingDisabled object) instead of just?

logging.getLogger().disabled = True
try:
  device.EnableRoot()
  has_root = True
except device_errors.CommandFailedError:
  has_root = False
finally:
  logging.getLogger().disabled = False

[jbudorick, 2014-05-15 14:10:19]

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
build/android/gyp/util/build_device.py:68: class LoggingDisabled(object):
On 2014/05/15 12:39:09, Primiano Tucci wrote:
> Is there a specific reason for having all this boilerplate (i.e. the "with"
> LoggingDisabled object) instead of just?
> 
> logging.getLogger().disabled = True
> try:
>   device.EnableRoot()
>   has_root = True
> except device_errors.CommandFailedError:
>   has_root = False
> finally:
>   logging.getLogger().disabled = False
> 

No, not really. The context manager would make more sense if the logger were
disabled in more than one place, but that obviously isn't the case.

[Primiano Tucci (use gerrit), 2014-05-15 15:21:34]

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
File build/android/gyp/util/build_device.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
build/android/gyp/util/build_device.py:68: class LoggingDisabled(object):
On 2014/05/15 14:10:20, jbudorick wrote:
> On 2014/05/15 12:39:09, Primiano Tucci wrote:
> > Is there a specific reason for having all this boilerplate (i.e. the "with"
> > LoggingDisabled object) instead of just?
> > 
> > logging.getLogger().disabled = True
> > try:
> >   device.EnableRoot()
> >   has_root = True
> > except device_errors.CommandFailedError:
> >   has_root = False
> > finally:
> >   logging.getLogger().disabled = False
> > 
> 
> No, not really. The context manager would make more sense if the logger were
> disabled in more than one place, but that obviously isn't the case.

I don't want to interfere with anybody's work (also I'm not an OWNER there), but
it would be nice for everybody who reads this code (e.g. sheriffs) if we could
avoid introducing unnecessary complexity / abusing of python magic constructs
when not really necessary and just keep things simple.

[jbudorick, 2014-05-15 15:40:22]

On 2014/05/15 15:21:34, Primiano Tucci wrote:
>
https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
> File build/android/gyp/util/build_device.py (right):
> 
>
https://codereview.chromium.org/285143002/diff/20001/build/android/gyp/util/b...
> build/android/gyp/util/build_device.py:68: class LoggingDisabled(object):
> On 2014/05/15 14:10:20, jbudorick wrote:
> > On 2014/05/15 12:39:09, Primiano Tucci wrote:
> > > Is there a specific reason for having all this boilerplate (i.e. the
"with"
> > > LoggingDisabled object) instead of just?
> > > 
> > > logging.getLogger().disabled = True
> > > try:
> > >   device.EnableRoot()
> > >   has_root = True
> > > except device_errors.CommandFailedError:
> > >   has_root = False
> > > finally:
> > >   logging.getLogger().disabled = False
> > > 
> > 
> > No, not really. The context manager would make more sense if the logger were
> > disabled in more than one place, but that obviously isn't the case.
> 
> I don't want to interfere with anybody's work (also I'm not an OWNER there),
but
> it would be nice for everybody who reads this code (e.g. sheriffs) if we could
> avoid introducing unnecessary complexity / abusing of python magic constructs
> when not really necessary and just keep things simple.

With no compelling reason - e.g. reusability - to keep the context manager
around, I'm fine with replacing it with your suggestion. It is definitely
simpler.

(I'm not sure that using context managers qualifies as "abusing ... python magic
constructs," though.)

[Primiano Tucci (use gerrit), 2014-05-15 15:47:44]

> (I'm not sure that using context managers qualifies as "abusing ... python
magic
> constructs," though.)
See that in this way. If you had used the LoggingDisabled() in another place
down in the file, you would have forced somebody to scroll up to find
LoggingDisabled() to see what that was doing. It's not a big deal itself either
ways, but when you start adding ten of things like that, it becomes a deal :)

Thanks btw

[frankf, 2014-05-15 17:40:08]

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:116: timeout: An integer containing
the number of seconds to wait for the
On 2014/05/15 05:03:54, jbudorick wrote:
> On 2014/05/15 01:27:59, frankf wrote:
> > DRY applies to docstrings as well ;)
> 
> "An integer containing the seconds to wait ...?" I'm not sure about that.
Sounds
> a little weird.

To clarify, you've repeated the same Args section for all the methods below.
It'd be good to define them once and refer to it instead of duplicating it.

https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:153: if not
self.old_interface.EnableAdbRoot():
Yes, otherwise you would to do another pass to move the implementation here. I
thought part of the refactoring is to review the existing implementation. Or are
you just concerned with the interface at this point?


On 2014/05/15 05:03:54, jbudorick wrote:
> On 2014/05/15 01:27:59, frankf wrote:
> > Why can't you inline EnableAdbRoot here?
> 
> What exactly do you mean by "inline" in a python context? Moving the
> AndroidCommands implementation in here?

[jbudorick, 2014-05-15 17:42:28]

On 2014/05/15 17:40:08, frankf wrote:
>
https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
> File build/android/pylib/device/device_utils.py (right):
> 
>
https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:116: timeout: An integer containing
> the number of seconds to wait for the
> On 2014/05/15 05:03:54, jbudorick wrote:
> > On 2014/05/15 01:27:59, frankf wrote:
> > > DRY applies to docstrings as well ;)
> > 
> > "An integer containing the seconds to wait ...?" I'm not sure about that.
> Sounds
> > a little weird.
> 
> To clarify, you've repeated the same Args section for all the methods below.
> It'd be good to define them once and refer to it instead of duplicating it.
> 

Oh, ok. That seems reasonable.

>
https://codereview.chromium.org/285143002/diff/20001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:153: if not
> self.old_interface.EnableAdbRoot():
> Yes, otherwise you would to do another pass to move the implementation here. I
> thought part of the refactoring is to review the existing implementation. Or
are
> you just concerned with the interface at this point?
> 
> 
> On 2014/05/15 05:03:54, jbudorick wrote:
> > On 2014/05/15 01:27:59, frankf wrote:
> > > Why can't you inline EnableAdbRoot here?
> > 
> > What exactly do you mean by "inline" in a python context? Moving the
> > AndroidCommands implementation in here?

Yeah, the current series of CLs is just concerned with the interface. The next
pass will switch from implementations based on AndroidCommands /
android_testrunner to ones based on AdbWrapper.

[cjhopman, 2014-05-15 21:18:21]

b/a/g lgtm

[craigdh, 2014-05-15 22:44:55]

lgtm.

[frankf, 2014-05-15 23:07:42]

pylib/device lgtm

[jbudorick, 2014-05-16 18:53:24]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-05-16 18:53:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/285143002/80001

[commit-bot: I haz the power, 2014-05-17 05:26:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-17 15:52:52]

Change committed as 271213