1578424 - Refactor code for mozdevice

Status: NEW

(Testing :: Mozbase Rust, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This is based on the landing of the code on bug 1525126. The code needs a better structure, and definitely more tests. I already started on this on the other bug, and will continue once that has been fixed.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from bug 1525126 comment #25)
I have no strong opinions about the existing mozdevice expression: re-write or throw it away and start again. The API is pretty simple; there's probably just:

1. run command
2. run shell command
3. push
4. (eventually) pull

We also need port forward / reverse, push dir, and maybe others to make it work with wdspec. Especially the unreliable return values for handling reverse connections worried me.

If you prefer, maybe write a version that uses adb on the command line to achieve that? Personally I have found the TCP connection to be much more robust, but there are all sorts of weird things in that protocol (and the current expression, which isn't tidy).

The current way utilizes the adb server to communicate with the host and the device via TCP, whereby the communication with the device is routed through the host. There would also be the way to communicate with the device directly, but then the whole adb protocol would need to be implemented. Something similar what https://github.com/fluxxu/adb-rs is doing, but which we considered not to use given that the package isn't used by other crates yet, and the author is not really in Open Source.

Another problem I was facing with TCP is that when killing the server via a adb command, there is no way to get the server started again unless you run adb via the command line. But maybe I'm missing something?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #1)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from bug 1525126 comment #25)
I have no strong opinions about the existing mozdevice expression: re-write or throw it away and start again. The API is pretty simple; there's probably just:

1. run command
2. run shell command
3. push
4. (eventually) pull

We also need port forward / reverse, push dir, and maybe others to make it work with wdspec. Especially the unreliable return values for handling reverse connections worried me.

Right: port forwarding (and reverse forwarding) are "adb commands", which are pretty easy to invoke with adb .... I.e., it's just adb forward host:port target:port, etc. adb push is recursive; it's my directory walking that misses the recursion.

The unreliable return values for reverse connections is a device problem: some Android versions are broken/work differently. Can't get around it by running the ADB protocol or adb or anything else.

If you prefer, maybe write a version that uses adb on the command line to achieve that? Personally I have found the TCP connection to be much more robust, but there are all sorts of weird things in that protocol (and the current expression, which isn't tidy).

The current way utilizes the adb server to communicate with the host and the device via TCP, whereby the communication with the device is routed through the host. There would also be the way to communicate with the device directly, but then the whole adb protocol would need to be implemented. Something similar what https://github.com/fluxxu/adb-rs is doing, but which we considered not to use given that the package isn't used by other crates yet, and the author is not really in Open Source.

I would have used https://github.com/fluxxu/adb-rs if I had seen it, but it's no different (philosophically) than the approach I implemented: it opens a TCP connection to the adb daemon on the host. (It does appear to be a much slicker implementation.) The only other alternative is to manage the USB connection yourself, which can be done but isn't worth the effort. That's what https://github.com/google/python-adb does, IIUC: manages USB (and then connects to the adb daemon on the target).

So it's not really true that you can communicate with the device directly: you still have to speak ADB to the device over some connection (TCP or USB). This way, the adb daemon on the host manages that connection, and you get a lightweight TCP socket that gets forwarded.

Another problem I was facing with TCP is that when killing the server via a adb command, there is no way to get the server started again unless you run adb via the command line. But maybe I'm missing something?

Yes, if you kill the server, you need to restart it via adb. You shouldn't ever need to kill the server. (The server multiplexes; if you kill it, you kill everybody's connection.)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Here some links which I got from Nick, and found myself which could be helpful for this refactoring:

https://android.googlesource.com/platform/system/core/+/master/adb/OVERVIEW.TXT
https://android.googlesource.com/platform/system/core/+/master/adb/protocol.txt
https://android.googlesource.com/platform/system/core/+/master/adb/SERVICES.TXT
https://github.com/Swind/pure-python-adb/
https://github.com/cstyan/adbDocumentation
https://github.com/noahgoldman/adbpy/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: [WIP] Refactor mozdevice rust

I have to work on some Puppeteer stuff first before I get to this bug again. In the meantime I want to attach the patch which I have locally just for safety that nothing gets lost. A good chunk of that work has already been included in the initial landing, so that the patch clearly would need a clean-up once I get started.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

With bug 1650891 we will get more changes to mozdevice. Lets wait for it.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please see all the left-open issues on https://phabricator.services.mozilla.com/D87464 that need to be taken care of.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bug 1749444 will add wdspec tests on Android. It will make it easier to verify that the refactoring doesn't cause regressions.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

When working on the refactor it might be good to have a look at the following adb documentation which explains the exact steps to run for certain commands:

https://github.com/cstyan/adbDocumentation