1335944 - |mach android-emulator| fails with 'ValueError: invalid literal for int() with base 10'

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Geoff Brown [:gbrown]]

We recently started checking the sdk version in devicemanagerADB's initialization. That's a problem when there is no connected device, like when running 'mach android-emulator':

gbrown@mozpad:~/src$ ./mach android-emulator
Error running mach:

    ['android-emulator']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ValueError: invalid literal for int() with base 10: "error: device 'emulator-5554' not found"

  File "/home/gbrown/src/mobile/android/mach_commands.py", line 118, in emulator
    emulator = AndroidEmulator(version, verbose, substs=self.substs, device_serial='emulator-5554')
  File "/home/gbrown/src/testing/mozbase/mozrunner/mozrunner/devices/android_device.py", line 325, in __init__
    deviceSerial=device_serial)
  File "/home/gbrown/src/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py", line 74, in __init__
    self._verifyADB()
  File "/home/gbrown/src/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py", line 765, in _verifyADB
    self._sdk_version = int(proc.output[0])

Comment 1

[Geoff Brown [:gbrown]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=efe5448d30a9bce0eedbdcc2a60b54e0287c828e

Comment 2

[Geoff Brown [:gbrown]]

https://public-artifacts.taskcluster.net/ELp9Wtv_Q5uVnlQZXUUPBQ/0/public/logs/live_backing.log

[task 2017-02-02T00:52:36.150202Z] 00:52:36     INFO -  mozdevice Detected adb %s
[task 2017-02-02T00:52:36.283156Z] 00:52:36     INFO -  mozdevice Detected Android sdk %s

Looks like a long-standing problem - oops!

Comment 3

[Geoff Brown [:gbrown]]

Attachment: delay sdk version check until connect()

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58807fccc3740e9a6609eba2c5a72c412e00a17

Comment 4

[Geoff Brown [:gbrown]]

Attachment: delay sdk version check until connect()

Sorry!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4ebce17d9788da2f8a7af0d119ba7e7e7bbac4

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 8832719 [details] [diff] [review]
delay sdk version check until connect()

># HG changeset patch
># Parent  f3d187bd0733b1182dffc97b5dfe623e18f92a44
>Bug 1335944 - Delay devicemanagerADB sdk check until connect() time; r=bc
>
>diff --git a/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py b/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
>--- a/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
>+++ b/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
>@@ -85,6 +85,12 @@ class DeviceManagerADB(DeviceManager):
>             # verify that we can connect to the device. can't continue
>             self._verifyDevice()
> 
>+            # Note SDK version
>+            proc = self._runCmd(["shell", "getprop", "ro.build.version.sdk"],
>+                                timeout=self.short_timeout)
>+            self._sdk_version = int(proc.output[0])
>+            self._logger.info("Detected Android sdk %s" % self._sdk_version)
>+

These days after reading https://pylint.readthedocs.io/en/latest/reference_guide/features.html#id27 logging-not-lazy (W1201)
I write this as

self._logger.info("Detected Android sdk %s", self._sdk_version)

Did this cause a problem? Or is it better to just be self consistent within the file?

Should we wrap the int(...) in a try: except ValueError: and set the version to 0 and log a warning if int(...) fails or is it better to just fail hard?

I see now that my placing this in _verifyADB was wrong since we are issuing the command to the device.

>             # Some commands require root to work properly, even with ADB (e.g.
>             # grabbing APKs out of /data). For these cases, we check whether
>             # we're running as root. If that isn't true, check for the
>@@ -759,11 +765,7 @@ class DeviceManagerADB(DeviceManager):
>             re_version = re.compile(r'Android Debug Bridge version (.*)')
>             proc = self._runCmd(["version"], timeout=self.short_timeout)
>             self._adb_version = re_version.match(proc.output[0]).group(1)
>-            self._logger.info("Detected adb %s", self._adb_version)
>-            proc = self._runCmd(["shell", "getprop", "ro.build.version.sdk"],
>-                                timeout=self.short_timeout)
>-            self._sdk_version = int(proc.output[0])
>-            self._logger.info("Detected Android sdk %s", self._sdk_version)
>+            self._logger.info("Detected adb %s" % self._adb_version)

ditto logging-not-lazy (W1201)

>         except os.error as err:
>             raise DMError(
>                 "unable to execute ADB (%s): ensure Android SDK is installed "

I'll leave it to you to decide about the try: except ValueError: and the logging-not-lazy.

By the way, devicemanagerADB is only used by Autophone during and by the unit tests so the try run with the S1S2Test won't test this.

You can add Autophone jobs to your try run by executing the following on the autophone servers:

. /mozilla/autophone/venv/bin/activate

cd /mozilla/autophone/autophone

python trigger_runs.py --build-location=tinderbox --repo=try --test=autophone-mochitest-toolkit-widgets --test=autophone-reftest-ogg-video --build-url=https://queue.taskcluster.net/v1/task/Qd8meSFmSyKVhA1LiDcP3/runs/0/artifacts/public/build/

python trigger_runs.py --build-location=tinderbox --repo=try --test=autophone-mochitest-toolkit-widgets --test=autophone-reftest-ogg-video --build-url=https://queue.taskcluster.net/v1/task/FTTgxbNHSqeoiqWZe0IU_g/runs/0/artifacts/public/build/

r+ with green Autophone mochitest and reftest test types on try.

Comment 6

[Bob Clary [:bc] (inactive)]

I went ahead and submitted the try jobs and they are green.

Comment 8

[Geoff Brown [:gbrown]]

(In reply to Bob Clary [:bc:] from comment #5)
> self._logger.info("Detected Android sdk %s", self._sdk_version)
> 
> Did this cause a problem? Or is it better to just be self consistent within
> the file?

Yes, at least for xpcshell -- see comment 2. (I haven't debugged it.)
 
> Should we wrap the int(...) in a try: except ValueError: and set the version
> to 0 and log a warning if int(...) fails or is it better to just fail hard?

Yes, that is a good idea -- will update my patch.

Thanks much for the extra try tests!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2ada5ea290
Delay devicemanagerADB sdk check until connect() time; r=bc

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f78fc2f94d07

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2d2ada5ea290

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d3dcda2046c2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d13001d68a21