1492242 - Use mozdevice for adb access in AndroidMixin

Status: RESOLVED FIXED

(Testing :: General, enhancement, P1)

Description

[Geoff Brown [:gbrown]]

AndroidMixin uses subprocess to invoke adb directly for some functions. It might be advantageous to use mozdevice instead.

Comment 1

[Geoff Brown [:gbrown]]

But mozdevice is not part of the mozharness package, so mozharness scripts do not have access to it. mozharness uses some other mozbase projects, like mozprocess, but this has been made possible by copying code from mozbase into mozharness -- ugly, inconvenient, and error prone (copies of mozbase projects always seem to become out-of-sync with the original).

Comment 2

[Geoff Brown [:gbrown]]

Attachment: wrap mozdevice with adb_cmd.py

:bc - I believe you want to use mozdevice for adb access everywhere, including mozharness, and I think that's a good goal. For mozharness, there is the complication that mozdevice is not easily available to mozharness. We have made other mozbase projects available to mozharness by copying the mozbase project under mozharness (like https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/mozprocess) but I don't like the code duplication or mozharness code bloat caused by that approach. 

Here's an alternative: Call adb from a separate process via this adb_cmd.py wrapper -- it will pick up the mozdevice that is pip installed by the mozharness script. Pretty simple so far, but if we go this way, adb_cmd.py would be extended over time to handle pass-through of more and more mozdevice capabilities. What do you think of the approach?

Comment 3

[Bob Clary [:bc] (inactive)]

President Not Sure says: "Let me think about that for a bit."

Comment 4

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1,2,3&revision=d530da9c94d83169e8db3b2d064465d8fb836c78

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 9010407 [details] [diff] [review]
wrap mozdevice with adb_cmd.py

There is a better way...

Comment 6

[Geoff Brown [:gbrown]]

Attachment: use mozdevice in AndroidMixin

Still runs fine locally (emulator/ubuntu host) and on try:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1%2C2%2C3&revision=394ac696cc083527e509d7d3af3395987a3d62a6

logcat remains a special snowflake for now, and I expect AndroidMixin to grow with bug 1492239 and bug 1492240, but I think this is a good start.

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 9010457 [details] [diff] [review]
use mozdevice in AndroidMixin

Review of attachment 9010457 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this is absolutely great! Some minor tweaks on the handling of failure to install.

::: testing/mozharness/mozharness/mozilla/testing/android.py
@@ +127,3 @@
>              self.fatal('INFRA-ERROR: Failed to install %s on %s' %
>                         (self.installer_path, self.device_name),
>                         EXIT_STATUS_DICT[TBPL_RETRY])

Should return False here.

@@ +127,4 @@
>              self.fatal('INFRA-ERROR: Failed to install %s on %s' %
>                         (self.installer_path, self.device_name),
>                         EXIT_STATUS_DICT[TBPL_RETRY])
> +        return True

We don't actually use the return value...

::: testing/mozharness/mozharness/mozilla/testing/raptor.py
@@ +366,4 @@
>  
>      def install(self):
>          if self.app == "geckoview":
> +            self.install_apk(self.installer_path)

We should either check the return value here and then terminate gracefully if it fails or remove the return value from the install_apk and just let the exception kill us.

Comment 8

[Geoff Brown [:gbrown]]

Attachment: use mozdevice in AndroidMixin

I removed the return from install_apk(). On failure, self.fatal() should bring execution to a halt and also trigger a task retry (where supported). I also considered removing the catch/fatal and relying on the ADBError; I prefer the fatal() for its specific error message and continuity with existing error handling.

Comment 9

[Bob Clary [:bc] (inactive)]

Comment on attachment 9010735 [details] [diff] [review]
use mozdevice in AndroidMixin

Review of attachment 9010735 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fbe78137004
Use mozdevice to access adb in mozharness AndroidMixin; r=bc

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/1fbe78137004