1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement, P3)

Description

[Robert Wood [:rwood]]

Raptor uses 'adb reverse' so it's webext can talk to the control server, and so the geckoview app on the device can access the benchmarks running on localhost on the host machine.

Instead of using 'adb reverse' from within raptor directly, an 'adb_reverse' command should be added to mozdevice.ADBAndroid.

Comment 1

[Bob Clary [:bc] (inactive)]

Bikeshedding for a moment.... adb_reverse seems like a redundant name considering where it will live. I would go with just 'reverse' but I'm ok with adb_reverse if you think that is more understandable as to its use.

adb reverse doesn't require any explicit android functionality on the device does it? I would think we could put it in ADBDevice where it could be reused by anyone still using the adb_b2g stuff. It doesn't really matter if we don't care about the b2g stuff anymore and I'm sure you would like to put that in the past as well. ;-)

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

possibly a "good first bug" ?

Comment 3

[Bob Clary [:bc] (inactive)]

egao: I think this will be a good bug to work on. the forward command is very similar and you can see how it is implemented in https://searchfox.org/mozilla-central/source/testing/mozbase/mozdevice/mozdevice/adb.py#907

Comment 4

[Edwin Takahashi (:egao | infrequent contributor)]

I've committed proposed additions of the 'reverse' method to adb.py.

Method is heavily inspired and taken from the 'forward' method.

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

Comment 5

[Edwin Takahashi (:egao | infrequent contributor)]

Upon closer inspection, I realized I was not iterating through a list of tuples signifying if the port is local or remote.

Try run for the fixed segment of code:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=603180c3f3faf206b911aed272bb4a7e7681a3d3

Comment 6

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: reverse_mozdevice_android.patch

Patch attached, includes correction to the for loop compared to the first submission.

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 9005040 [details] [diff] [review]
reverse_mozdevice_android.patch

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

Looks good as far as it goes. Let's complete the methods to fully implement adb reverse.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1050,3 @@
>  
>          self.command_output(cmd, timeout=timeout)
>  

add a comment to show this is distinct from the forwarding methods...

    # Port reversing methods

@@ +1071,5 @@
> +        cmd = ["reverse", local, remote]
> +        if not allow_rebind:
> +            cmd.insert(1, "--no-rebind")
> +        self.command_output(cmd, timeout=timeout)
> +

complete the methods by adding list_reverses and remove_reverses.

Comment 8

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: bug_1484238_list_remove_reverse.patch

Comment 9

[Edwin Takahashi (:egao | infrequent contributor)]

(In reply to Bob Clary [:bc:] from comment #7)
> add a comment to show this is distinct from the forwarding methods...
> 
>     # Port reversing methods
> 

Comment added. Noted in an one-liner comment for both forward and reverse what they do.

> complete the methods by adding list_reverses and remove_reverses.

Implemented request:
- list_reverses
- remove_reverses

Code is essentially identical to the list_forwards and remove_forwards methods; I believe there is room to refactor this into one method in the near future, something akin to remove_ports and list_ports (naming TBD).


-----------------------------------------------------------------

In addition, the latest patch includes corrections to the docstring that referenced the methods where it was copied from (forward). These instances have been fixed to refer to 'reverse'.

Comment 10

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Heavily inspired by the forward() method present in same file.

Comment 11

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Fixed a mistake where for loop was iterating through a list of strings, not a tuple as forward does.

Depends on D4716

Comment 12

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Added list_reverses and remove_reverses commands.
Corrected mistakes in the docstring.

Depends on D4717

Comment 13

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Comment 14

[Bob Clary [:bc] (inactive)]

Comment on attachment 9005656 [details]
Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Bob Clary [:bc:] has approved the revision.

Comment 15

[Geoff Brown [:gbrown]]

Comment on attachment 9005656 [details]
Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Geoff Brown [:gbrown] has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ea388fd649a
Add an 'adb_reverse' command to mozdevice.ADBAndroid r=gbrown,bc

Comment 17

[Edwin Takahashi (:egao | infrequent contributor)]

Try run from the final revision:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa1b38f6b75dbf1067015cebcde4c516b7866708

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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