Marionette should use the adb in the path first

RESOLVED FIXED in mozilla17


6 years ago
5 years ago


(Reporter: mwu, Unassigned)


Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [good first bug][mentor=mdas][lang=python])


(3 attachments, 4 obsolete attachments)



6 years ago
using the adb in the path, if available, prevents adb version mismatches.
Whiteboard: [good first bug][mentor=mdas][lang=python]

I would like to try my hand at this bug. It's my first try, so I will most likely ask several stupid questions along the way.

Could you give me some details as I am not familiar with the structure of mozilla-central yet. E.g., I know the path is "./mozilla-central/testing/marionette/client/marionette", but which files are affected?

Any light reading that may give me a headstart? adb = android debug bridge?

Thanks for your patience
Created attachment 653814 [details]
fake files

Right now, the marionette client will try to use the adb (android debug bridge) contained in the passed in homedir variable.

The goal of this bug is to get (mozilla-central)/testing/marionette/client/marionette/ using the adb version that exists in the user's PATH to be used if available. If it is not, then we'll use the one that's contained in the path given by the homedir variable.

I've attached which is a collection of files you can use to fake a home directory that's needed by the test runner. Unzip this file as (mozilla-central)/testing/marionette/client/marionette/homedir. 

You can start testing out this stuff by creating a python virtualenv to play in:

cd (mozilla-central)/testing/marionette/client/marionette/
sh <path to python2.7>
cd ../marionette_venv
. bin/activate

I'd add the appropriate changes and add a print statement once you've set the new self.adb.
Then you can test it using:

cd (mozilla-central)/testing/marionette/client/marionette/

python --homedir=/Users/mdas/Code/mozilla-central/testing/marionette/client/marionette/homedir --emulator=arm tests/unit-tests.ini 

This will run the test runner with the fake files. It will get to the point where it assigns the adb value, prints the new adb path, and then exits due to failure to find files (expected).
Created attachment 653959 [details] [diff] [review]
First Patch to this bug

This is the first patch.

@mdas: Thanks a lot for your help!

I hope it is fine, as it is.
Comment on attachment 653959 [details] [diff] [review]
First Patch to this bug

Review of attachment 653959 [details] [diff] [review]:

Awesome job!

I added some revisions, so I'm anticipating version 2 of the patch :)

I forgot to mention it before, but when you add a new patch, please add me (mdas @ as a reviewer. This means, select the '?' in the review drop down and add my email address when you upload your next attachment.

::: testing/marionette/client/marionette/
@@ +144,5 @@
>      def _check_for_adb(self):
> +        host_dir = "linux-x86"
> +        if platform.system() == "Darwin":
> +            host_dir = "darwin-x86"        
> +        

clean up whitespace (there are extra spaces)

@@ +150,2 @@
>                                         stdout=subprocess.PIPE,
>                                         stderr=subprocess.STDOUT)

One thing I realized is that we do another check in the original file that we do not do here. We should add this as another alternate adb path:

This is the (homedir)/out/host/(host_dir)/adb path.

@@ +150,4 @@
>                                         stdout=subprocess.PIPE,
>                                         stderr=subprocess.STDOUT)
> +        alternative_adb = os.path.join(self.homedir,
> +                            'glue/gonk/out/host',host_dir ,'/bin/adb')

so doing os.path.join() with "/bin/adb" will not create a viable path. You need to use "bin/adb", and actually, to be as correct as possible we should do:

os.path.join(self.homedir, "glue", "gonk", "out", "host", host_dir, "bin", "adb")

The reason for this is os.path.join will create a path according to the system it is running in. So for posix systems you'll get directories with '/' between them, but for windows, you'll get '\'. I know some parts of the file doesn't use the right convention, but if you're willing to update them, go for it;)

@@ +151,5 @@
>                                         stderr=subprocess.STDOUT)
> +        alternative_adb = os.path.join(self.homedir,
> +                            'glue/gonk/out/host',host_dir ,'/bin/adb')
> +        last_chance_adb = os.path.join(self.homedir, 'bin/adb')
> +                            

clean up whitespace after line 154
Created attachment 654127 [details] [diff] [review]
First revision of patch

I applied the fixes. Regarding whitespaces for commands that go over several lines (Here the call to subprocess.Popen), I am not too sure what the conventions here are. I usually align it with the open bracket for clarity if it is the arguments that go over the line. With Strings, I align it with the opening ". Is that the common convention here, too?

Thanks for your help.
Attachment #653959 - Attachment is obsolete: true
Attachment #654127 - Flags: review?(mdas)
Comment on attachment 654127 [details] [diff] [review]
First revision of patch

Review of attachment 654127 [details] [diff] [review]:

Perfect, it works as expected, thanks!

I do have one more improvement that I'm suggesting in-line, which is an opportunity for you to use some more programming concepts.

::: testing/marionette/client/marionette/
@@ +154,5 @@
> +                                        'bin', 'adb')
> +        last_chance_adb = os.path.join(self.homedir, 'bin','adb')
> +        if adb.wait() == 0:
> +            self.adb = # remove trailing newline
> +        elif os.path.exists(alternative_adb):

Instead of these sequential elif statements, I think you can put the concept of a for-loop to good use here... ping me on IRC or add a comment if you're having trouble!
Attachment #654127 - Flags: review?(mdas) → review+
Once this patch is done, the one who pushes the changes to mozilla-central (probably me) will need to merge these changes into the version in /build/mobile.
Created attachment 654371 [details] [diff] [review]
Second revision of patch

Ok, here is the revised version. This was fun.
Attachment #654127 - Attachment is obsolete: true
Attachment #654371 - Flags: review?(mdas)
Comment on attachment 654371 [details] [diff] [review]
Second revision of patch

Review of attachment 654371 [details] [diff] [review]:

Excellent patch, thanks for your help!

I'll get this patch landed, and merge the related changes into the /build/mobile/ version as well
Attachment #654371 - Flags: review?(mdas) → review+
Created attachment 654394 [details] [diff] [review]
update with changes

Since the version in /build/mobile is quite different from the marionette client version, I'd like jgriffin to take a look at this patch.

This patch just applies the relevant changes in _check_for_adb from nebelhom's patch.
Attachment #654394 - Flags: review?(jgriffin)
Comment on attachment 654394 [details] [diff] [review]
update with changes

Review of attachment 654394 [details] [diff] [review]:

This looks good, although ideally I think we'd make something like a _which_adb method in build/mobile/, and have both and use that, rather than duplicating the 'which adb' logic.
Attachment #654394 - Flags: review?(jgriffin) → review+
Created attachment 654612 [details] [diff] [review]
update with changes
Created attachment 654613 [details] [diff] [review]
update with changes v2.0

I changed the name to _default_adb, since _which_adb and _check_for_adb sound similar.
Attachment #654394 - Attachment is obsolete: true
Attachment #654612 - Attachment is obsolete: true
Attachment #654613 - Flags: review?(jgriffin)
Comment on attachment 654613 [details] [diff] [review]
update with changes v2.0

Review of attachment 654613 [details] [diff] [review]:

Cool, thanks!
Attachment #654613 - Flags: review?(jgriffin) → review+
Landed nebelhom's patch in inbound as:

and the build/mobile patch as:
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.