Closed Bug 1140406 Opened 9 years ago Closed 9 years ago

[mozdevice] Allow connecting to a remote ADB server in


(Testing :: Mozbase, defect)

Not set


(firefox39 fixed)

Tracking Status
firefox39 --- fixed


(Reporter: davehunt, Assigned: davehunt)




(1 file, 2 obsolete files)

Similar to bug 1101655 we need 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

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?
Flags: needinfo?(bob)
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?
Flags: needinfo?(dave.hunt)
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.
Flags: needinfo?(dave.hunt)
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.
Flags: needinfo?(bob)
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.
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.
Assignee: nobody → dave.hunt
Attachment #8578005 - Flags: feedback?(bob)
Blocks: 1143834
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

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

::: testing/mozbase/mozdevice/mozdevice/
@@ +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.
Attachment #8578005 - Flags: feedback?(bob) → feedback+
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 changes as I am un-familiar with that code.

::: testing/mozbase/mozdevice/mozdevice/
@@ +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.
Attachment #8581565 - Flags: review?(bob) → review+
Hey BC, if you wouldn't mind could you have a final look over this patch before I land it? Thanks!

Try run:
Attachment #8581565 - Attachment is obsolete: true
Attachment #8581784 - Flags: review?(bob)
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/
@@ +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

Attachment #8581784 - Flags: review?(bob) → review+
Landed on inbound:

The try failure is a timeout that's also occurring on mozilla-central so is not related to this patch.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.