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

RESOLVED FIXED in Firefox 51

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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])
(Assignee)

Updated

2 years ago
Assignee: nobody → gbrown
(Assignee)

Comment 2

2 years ago
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!
(Assignee)

Comment 4

2 years ago
Created attachment 8832719 [details] [diff] [review]
delay sdk version check until connect()

Sorry!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e4ebce17d9788da2f8a7af0d119ba7e7e7bbac4
Attachment #8832715 - Attachment is obsolete: true
Attachment #8832715 - Flags: review?(bob)
Attachment #8832719 - Flags: review?(bob)

Comment 5

2 years ago
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.
Attachment #8832719 - Flags: review?(bob) → review+

Comment 6

2 years ago
I went ahead and submitted the try jobs and they are green.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1336089
(Assignee)

Comment 8

2 years ago
(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

2 years ago
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2ada5ea290
Delay devicemanagerADB sdk check until connect() time; r=bc
(Assignee)

Updated

2 years ago
Blocks: 1334613
Whiteboard: [checkin-needed-aurora][checkin-needed-beta][checkin-needed-release]

Comment 10

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f78fc2f94d07
status-firefox53: --- → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta][checkin-needed-release] → [checkin-needed-beta][checkin-needed-release]

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d2ada5ea290
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Comment 12

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d3dcda2046c2
status-firefox52: --- → fixed
Whiteboard: [checkin-needed-beta][checkin-needed-release] → [checkin-needed-release]

Comment 13

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/d13001d68a21
status-firefox51: --- → fixed
Whiteboard: [checkin-needed-release]
You need to log in before you can comment on or make changes to this bug.