1012711 - Add adb.py to devicemanager package

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement)

Description

[Bob Clary [:bc] (inactive)]

Attachment: devicemanager-adb.patch

bug 990601 added adb.py to Autophone in order to control devices via adb over usb. Adding this to mozbase allows us to continue to use the existing devicemanagerADB while developing adb.py as a replacement.

wlach, is there anyone who should also review this?

Comment 1

[William Lachance (:wlach)]

Comment on attachment 8424884 [details] [diff] [review]
devicemanager-adb.patch

:ahal may be interested in giving feedback as well.

(on holiday today, will look tomorrow)

Comment 2

[William Lachance (:wlach)]

We should make sure that we have a clear concept of "writable storage area" vs. "directory we can use for writing random temporary test data". See bug 1011040.

Comment 3

[Bob Clary [:bc] (inactive)]

I'm not sure where we go from here. We also have the issue found in bug 1010173 where we need to be able to wait for the sdcard (in whatever incarnation it exists) to be properly mounted.

My thoughts are:

On our Android devices we have /data/local which is definitely internal storage on the device.

We also typically have /sdcard or /mnt/sdcard which may be external (on boards and older Android devices) or internal (on newer Android devices).

For newer devices that have /mnt/sdcard mapped to internal storage there is usually another path which specifies the real external sdcard but which is as far as I know not standardized.

Even when we have an internal /mnt/sdcard, we may still want to use an external sdcard to prevent wearing out an unreplaceable component.

The approach I took in bug 933842 was to provide the option to place the desired path in an ini file on the device.

Perhaps we could try some of the following, perhaps in combination.

1. Check for a configuration file on the device which specifies a preferred storage path. I bet RelEng won't like this.

2. Check a predefined list of paths such as adb.py and devicemanager do now and pick the first that is available. This suffers from the possibility the desired path might be temporarily unavailable due to mounting delays and that the choice of path would be non-deterministic due to possible temporary delays in storage availability.

3. Perform an automated detection of the possible storage options available on the device.

I haven't any experience with b2g devices. How do they compare to our 'regular' Android devices?

Comment 4

[William Lachance (:wlach)]

In general b2g devices should probably work similarly to android devices.

I think we should be careful not to make this over-specific to the particular needs of a particular test environment. Dave Hylands advocates basically for option (3) in this bug, which sounds pretty sensible:

https://bugzilla.mozilla.org/show_bug.cgi?id=1000918#c7

I'd just have a variable called "user_storage_area" or something like that. Individual test harnesses or frameworks can then derive a testing directory from that as they need. Does that make sense?

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8424884 [details] [diff] [review]
devicemanager-adb.patch

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

So yeah, this looks good. Have some minor requests for changes before this becomes part of mozdevice.

My feeling is that we should update the documentation before landing this. Please include some explanatory text on how this is now the preferred interface for devices running adb. You should just need to modify `docs/mozdevice.rst`. So r- on that.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +677,5 @@
> +        except ADBError, e:
> +            if self.process_exist(appname, timeout=timeout):
> +                raise e
> +
> +    def launch_application(self, app_name, activity_name, intent, url=None,

This won't work on B2G, should go into an extension class, similar to what we did with Droid before.

@@ +723,5 @@
> +        cmd = self._escape_command_line(acmd)
> +        self.shell_output(cmd, timeout=timeout)
> +
> +    def launch_fennec(self, app_name, intent="android.intent.action.VIEW",
> +                     moz_env=None, extra_args=None, url=None, wait=True,

As above.

@@ +956,5 @@
> +        get_state() returns device.
> +        """
> +        self.command_output(["reboot"], timeout)
> +        self.command_output(["wait-for-device"], timeout=timeout)
> +        time.sleep(self._reboot_settling_time)

I would probably just take this out. We shouldn't be relying on a timeout to verify that the device is fully ready.

Comment 6

[Bob Clary [:bc] (inactive)]

(In reply to William Lachance (:wlach) from comment #5)

> 
> @@ +956,5 @@
> > +        get_state() returns device.
> > +        """
> > +        self.command_output(["reboot"], timeout)
> > +        self.command_output(["wait-for-device"], timeout=timeout)
> > +        time.sleep(self._reboot_settling_time)
> 
> I would probably just take this out. We shouldn't be relying on a timeout to
> verify that the device is fully ready.

I tested a version without a settling time on Autophone and had numerous problems with the external sdcard on nexus one and some other issues on samsung gs3 due to the device not being 'ready' without the delay. What would you suggest as a means of determining when the device and its storage has completed the boot process? I could test the test_root by attempting to read/write from it to see if it is ready. Would that be acceptable?

Comment 7

[William Lachance (:wlach)]

(In reply to Bob Clary [:bc:] from comment #6)
> I tested a version without a settling time on Autophone and had numerous
> problems with the external sdcard on nexus one and some other issues on
> samsung gs3 due to the device not being 'ready' without the delay. What
> would you suggest as a means of determining when the device and its storage
> has completed the boot process? I could test the test_root by attempting to
> read/write from it to see if it is ready. Would that be acceptable?

Yes, that would be much preferable if it works!

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: devicemanager-adb.patch

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: mozdevice.html

This is the generated rst docs.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: bug-1012711.patch

Mark, I'm still keeping adb.py and adb_android.py in Autophone's repo for now. This patch has the identical bits for adb.py and adb_android.py as in the mozbase patch but also includes the additional changes needed for Autophone. If you could review those that would be nice. Of course, any additional review comments you have about adb.py and adb_android.py would be appreciated as well.

Comment 11

[Dave Hylands [:dhylands]]

(In reply to Bob Clary [:bc:] from comment #6)
> I tested a version without a settling time on Autophone and had numerous
> problems with the external sdcard on nexus one and some other issues on
> samsung gs3 due to the device not being 'ready' without the delay. What
> would you suggest as a means of determining when the device and its storage
> has completed the boot process? I could test the test_root by attempting to
> read/write from it to see if it is ready. Would that be acceptable?

For volumes which are mounted through vold (typically an external sdcard fits into this category), then you can check its status with the vdc volume list command:

For example, on my flame:

> adb shell vdc volume list
> 110 0 sdcard /storage/sdcard 4
> 110 0 sdcard1 /storage/sdcard1 4
> 200 0 Volumes listed.

The 4 at the end means that the volume is "mounted". Any other number means that the volume is not available to software running on the phone. There is a status (0) for NoMedia. See http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsIVolume.idl#13 for a complete list of status codes.

When the phone boots up (and media is present), it will initially be 1 (Idle) and then when b2g starts it will start the mounting process.

Comment 12

[Bob Clary [:bc] (inactive)]

(In reply to Dave Hylands [:dhylands] from comment #11)

The current patch is for adb.py which is android or b2g and adb_android.py where I isolated the Android specific functionality. b2g will come later.

For Android I went with checking if the sdcard was writable combined with a check that pm list commands work without a Package Manager error. From the Android docs, Package Manager is a good stand in for determining boot status.

Availability of the sdcard wasn't sufficient for samsung gs3s since the internal sdcard was mounted before the Package Manager began running. The vdc trick would work for my nexus one devices but not for samsung's gs3s. I think checking writability and Package Manager should be compatible across Android.

For b2g, we'll see when I get my flame devices shortly. If all b2g devices are all vold, then we can look at that, but we'll probably have to have an additional check to make sure we are really ready.

Comment 13

[Dave Hylands [:dhylands]]

(In reply to Bob Clary [:bc:] from comment #12)
> For b2g, we'll see when I get my flame devices shortly. If all b2g devices
> are all vold, then we can look at that, but we'll probably have to have an
> additional check to make sure we are really ready.

So most b2g devices use vold. Anything that is sharable with the PC uses vold (although we're in the process of adding MTP support which will change everything).

The nexus 4/5 doesn't use vold. The Tarako uses vold for the removable sdcard, but not for the internal volume.

The flame uses vold for both internal and external volumes.

I added a facility to the AutoMounter in B2G to be able to read /system/etc/volume.cfg (an AutoMounter specific file format) and it can be used to add volumes which have no vold support.

Comment 14

[Dave Hylands [:dhylands]]

One indication that might work on b2g is to do b2g-ps and see if more than one process is listed. The second process will usually be the homescreen or the FTU app.

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 8441429 [details] [diff] [review]
devicemanager-adb.patch

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

I think it would be awesome if the ADB class didn't try connecting to a device until a command that requires a device to be connected is called. There are several useful adb methods (e.g devices, start-server, wait-for-device) that don't need a device attached. This has always bugged me with the old class. For example, we'd want to use 'adb devices' to introspect which entry is new after starting an emulator. Though we needed to create dm instance anyway, we always needed to create a one-off "run_adb" method [1] because the dm object expects to be connected as soon as it is initialized. It would have been simpler to instantiate a dm object, call dm.devices, then connect the object to the proper device. I don't mind if this is done in a follow up bug though.

Other than that suggestion, I don't have much other feedback. This looks good and is much easier to read than the old class. Thanks for doing this!

[1] http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/emulator.py#196

Comment 16

[Bob Clary [:bc] (inactive)]

(In reply to Andrew Halberstadt [:ahal] from comment #15)

That's a great point and idea.

On a couple of other issues/nits:

* s/can continued to be used/can continue to be used/
* I think I should add a parameter to the kill methods that would allow them to retry up to a maximum number of attempts.

Comment 17

[Bob Clary [:bc] (inactive)]

Comment on attachment 8441429 [details] [diff] [review]
devicemanager-adb.patch

I'm going to deal with ahal's suggestion as well as rework some of the process kill logic. I'll have a new version available soonest.

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: devicemanager-adb.patch v2

I didn't go with class methods for the non-device adb commands. It seemed to me that it was better to go with a base class that implemented command and command_output and then subclass that for the specific cases.

ADBCommand
    ADBHost
    ADBDevice
        ADBAndroid

I put the full output of the sphinx build on http://people.mozilla.org/~bclary/mozdevice/mozdevice.html#adb-interface

Comment 19

[Andrew Halberstadt [:ahal]]

But doesn't that mean we'll need to create and pass around two instances instead of one? I guess this approach could work if ADBDevice has an ADBHost instance in it, but that seems a little complicated.

Comment 20

[Bob Clary [:bc] (inactive)]

It is pretty much zero-cost to create an ADBHost instance. They don't require contacting devices nor maintain state. Compared to controlling specific devices, how often do we need to do host only things such as starting or killing the adb server or getting the device list.

Why not just create an ADBHost object as you need it rather than creating it once and passing the same instance around?

Comment 21

[Bob Clary [:bc] (inactive)]

I didn't want the host only methods to interfere with the device specific methods.

Trying to put the host only methods in the device class seemed very awkward to me. Before I could get an ADBDevice instance I'd have to create one and contact the device. Without a constructor for the host only methods, I'd have make the calls more complicated in terms of passing the parameters that would otherwise be defined in the constructor.

Since there is only one host, we could so something about making it a singleton and creating an easy way to get to the single instance.

Comment 22

[Bob Clary [:bc] (inactive)]

Comment on attachment 8443059 [details] [diff] [review]
devicemanager-adb.patch v2

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

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +313,5 @@
> +        is parsed and placed into an object as in
> +
> +        [{'device_serial': '98095208', 'state': 'device', 'product': 'd2vzw',
> +          'usb': '1-2', 'device': 'd2vzw', 'model': 'SCH_I535' }]
> +

Of course I messed up and included the output for one device and the object for a different one. Will fix.

@@ +317,5 @@
> +
> +        """
> +        # b313b945               device usb:1-7 product:d2vzw model:SCH_I535 device:d2vzw
> +        re_device_info = re.compile(r'([^\s]+)\s+([^\s]+)\s+([^\s]+)\s+([^\s]+)\s+([^\s]+)\s+([^\s]+)')
> +        devices = []

This is too specific and will fail to report devices in error states. Will fix.

Comment 23

[Bob Clary [:bc] (inactive)]

Attachment: devicemanager-adb-followup-1.patch

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: devicemanager-adb.patch v3

Comment 25

[Bob Clary [:bc] (inactive)]

(In reply to Andrew Halberstadt [:ahal] from comment #19)
> But doesn't that mean we'll need to create and pass around two instances
> instead of one? I guess this approach could work if ADBDevice has an ADBHost
> instance in it, but that seems a little complicated.

I suppose I could do a class or static method with default arguments. Would that be better?

Comment 26

[William Lachance (:wlach)]

Comment on attachment 8443571 [details] [diff] [review]
devicemanager-adb.patch v3

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

(sorry about the lateness of this, I forgot to send it on Friday at the office and then didn't make it back until today...)

This looks really good. I don't really have many comments about the code itself as I've already reviewed it. About the only thing that I would like to see is some kind of description of the different options at the head of the mozdevice documentation.

Right now we say this:

Mozdevice provides an interface to interact with a remote device such as an Android- or FirefoxOS-based phone connected to a host machine. Currently there are two implementations of the interface: one uses a custom TCP-based protocol to communicate with a server running on the device, another uses Android’s adb utility.

Should replace it with something like this (feel free to modify to your liking):

"Mozdevice provides several interfaces to interact with a remote device such as an Android- or FirefoxOS-based phone. It allows you to push files to these types of devices, launch processes, and more. There are currently two available interfaces: 

* DeviceManager: an interface to a device that works either via ADB or a custom TCP protocol (the latter requires an agent application running on the device).
* ADB: a similar interface that uses Android Debugger Protocol explicitly

In general, new code should use the ADB abstraction where possible as it is simpler and more reliable."

Comment 27

[Bob Clary [:bc] (inactive)]

(In reply to William Lachance (:wlach) from comment #26)

That seems fine. I've updated it with your comments and put an updated version on http://people.mozilla.org/~bclary/mozdevice/mozdevice.html

Are we ok with the class hierarchy or should I go with class or static methods for the host only commands?

Comment 28

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd65e3c2f4a

Comment 29

[Bob Clary [:bc] (inactive)]

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

I put the wrong bug number in the check in comment.

Comment 30

[Carsten Book [:Tomcat]]

(In reply to Bob Clary [:bc:] from comment #29)
> https://hg.mozilla.org/mozilla-central/rev/1fd65e3c2f4a
> 
> I put the wrong bug number in the check in comment.

no problem correct the bugnumber and all good now :)