1140406 - [mozdevice] Allow connecting to a remote ADB server in adb.py

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Similar to bug 1101655 we need adb.py to support sending commands to a remote ADB server. This is needed for running any tests in our remote device lab, such as the b2gmonkey [1] tests, which are currently using adb.py

[1] https://github.com/mozilla-b2g/b2gmonkey

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I suspect this will require us to add host and port keyword arguments to ADBDevice and pass these through to the command methods. If these are present then they should be translated to -H <host> and -P <port> in the executed command.

BC: Do you think this is a candidate for good first/next bug? Do you have any thoughts on this, and would you be interested in mentoring someone on this bug?

Comment 2

[Bob Clary [:bc] (inactive)]

Dave, this is just about being able to run multiple adb servers on the same machine? This doesn't involve connecting to the devices over adb over tcpip?

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

No, this is about an ADB server that's running on a remote machine. So you use the local ADB binary but passing -H and -P connects it to a remote ADB server.

Comment 4

[Bob Clary [:bc] (inactive)]

Adding the arguments -H and -P should be a good candidate for a good first/next bug I think.

As we discussed over irc, I can't get this to work for me locally but I don't want to block this enhancement for you based on just that. Since I can't get the -H -P remote adb connection to work, if you would provide the mentoring I would appreciate it. I'll follow along in the bug and offer my 2 cents as appropriate. I'd like to review the final patch as well.

Perhaps we can figure out the appropriate incantation to get this to work in general.

Comment 5

[William Lachance (:wlach)]

I think getting the API and implementation details right on this would preclude this being a good first bug (I made the mistake of merging in some somewhat broken code into devicemanagerADB for adb-over-tcp a few years ago, and have regretted it ever since). If this something we need doing, I think we should probably get either staff or a senior community member to fix this.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support connecting to a remote ADB server. v1.0

This is working for me locally but I'd like to do further testing. In particular I'm not sure start/kill server works (or makes any sense) with the concept of a remote ADB server. I'd appreciate feedback while I look into a similar limitation in mozrunner.

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 8578005 [details] [diff] [review]
Support connecting to a remote ADB server. v1.0

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

A couple of nits about initialization. 

Also adb -H host -P port kill-server works but start-server won't start the server on the remote host at least for me. Include a comment in the docstrings about the need to start the remote adb using adb -a fork-server server on the remote host. Also that doing something like:

while true; do
   adb -a fork-server server
done

on the remote host in order to restart it may be necessary.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +325,2 @@
>          ADBCommand.__init__(self, adb=adb, logger_name=logger_name,
>                              timeout=timeout, verbose=verbose)

You should pass the adb_host, adb_port parameters to the ADBCommand.__init__ and allow the constructor to initialize them and not explicitly initialize them here.

@@ +523,3 @@
>                              timeout=timeout, verbose=verbose)
> +        self._adb_host = adb_host
> +        self._adb_port = adb_port

No need to do this here since they are initialized by the constructor call immediately prior to this.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support connecting to a remote ADB server. v1.1

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f2395e25463

Comment 9

[Bob Clary [:bc] (inactive)]

Comment on attachment 8581565 [details] [diff] [review]
Support connecting to a remote ADB server. v1.1

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

r+ with the nits addressed. I tested this manually, and it appears to work fine. I didn't review the runners.py changes as I am un-familiar with that code.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +391,5 @@
> +        you need to start a remote ADB server then you'll need access to the
> +        remote machine's terminal where you can run
> +        ``adb -a fork-server server`` or if you need it be able to restart
> +        itself:
> +

Attempting to use start_server with any adb_host value other than None will fail with an ADBError exception.

You will need to start the server on the remote host via the command:

        ``adb -a fork-server server``

If you wish the remote adb server to restart automatically, you can enclose the command in a loop as in:

@@ +534,2 @@
>                              timeout=timeout, verbose=verbose)
> +        self._adb = adb

This is not needed and the property name is wrong. self._adb_path is initialized by the ADBCommand.__init__ call.

@@ +588,5 @@
>  
>      def _get_device_serial(self, device):
>          if device is None:
> +            devices = ADBHost(adb=self._adb, adb_host=self._adb_host,
> +                              adb_port=self._adb_port).devices()

use self._adb_path here instead of self._adb.

Comment 10

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Support connecting to a remote ADB server. v1.2

Hey BC, if you wouldn't mind could you have a final look over this patch before I land it? Thanks!

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=585651622bcf

Comment 11

[Bob Clary [:bc] (inactive)]

Comment on attachment 8581784 [details] [diff] [review]
Support connecting to a remote ADB server. v1.2

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

r+ with minor nit assuming your try run looks good though it may take a while from the looks of treeherder. ;-)

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +386,5 @@
>              specified, the value set in the ADBHost constructor is used.
>          :raises: * ADBTimeoutError
>                   * ADBError
>  
> +        Attempting to user start_server with any adb_host value other than None

s/user/use/

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d6322d42b7

The try failure is a timeout that's also occurring on mozilla-central so is not related to this patch.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48d6322d42b7

Comment 14

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/678709bb90e7751e222a59d947aea3be25c79351