1410714 - adb path defaults to $PATH instead of .mozbuild path

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Aastha]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.62 Safari/537.36

Steps to reproduce:

I had adb package installed and bootstrap downloaded adb in .mozbuild directory.
I tried to run mochitest but was not able to run it. I removed adb package then it showed me that there is no adb in the PATH.
mach command is using adb in .mozbuild and while running tests it defaults to adb in $PATH. This causes the adb server to be killed and restarted while running test.


Actual results:

I was able to run test after adding adb in .mozbuild to my path.


Expected results:

The default adb path should be same for mach and while running tests.

Comment 1

[Aastha]

Attachment: patch to resolve the bug

Comment 2

[Gregory Szorc [:gps]]

Moving mozdevice patch to appropriate component and flagging someone who has reviewed mozdevice code recently.

This bug kinda looks similar to bug 1408644, so CCing nalexander.

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8920893 [details] [diff] [review]
patch to resolve the bug

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

This isn't a bad idea, but not everybody is running Linux -- so "android-sdk-linux" isn't great :)

Instead, can you try to use the code (or approach) in http://searchfox.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/devices/android_device.py#601 to use ANDROID_SDK_ROOT?  gbrown can provide additional help; he's much more familiar with this code than I am.

Thanks for the patch!

Comment 4

[Nick Alexander :nalexander [he/him]]

gbrown: can you suggest a way to share the mozbase/mozrunner and mozdevice code?

Comment 5

[Geoff Brown [:gbrown]]

mozdevice is used in automation, away from .mozbuild, so that is also a concern. Overall, I think it would be best to keep mozdevice (devicemanagerADB) the way it is now: use adbPath if specified, otherwise $PATH.

I think it should be possible to use the mozrunner (android_device.py) code, or something like it, to find adb from the test mach commands, and then specify adbPath when starting a test harness. Structurally, I'm thinking of something like [1]...but I need to think/fiddle with it to be more specific. I'll take the bug and try to sort this out some time this week.

1. https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/testing/mochitest/mach_commands.py#239

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Brown [:gbrown] from comment #5)
> mozdevice is used in automation, away from .mozbuild, so that is also a
> concern. Overall, I think it would be best to keep mozdevice
> (devicemanagerADB) the way it is now: use adbPath if specified, otherwise
> $PATH.

Hmm.  I think we should use, in order:

- adbPath
- ANDROID_SDK_ROOT/platform-tools/adb
- $PATH

$PATH and Mozilla's build system should be more or less independent _after_ configure.

Comment 7

[Geoff Brown [:gbrown]]

Attachment: use mozrunner to find adb for mach tests

Wow, this is way more complicated than Aastha's patch!

mozdevice is left untouched -- it continues to use the adb it is told to.

All the mach tests are converted to use mozrunner's get_adb_path() which relies on the existing _find_sdk_exe() to find adb using our tried-and-true algorithm. 

The final priority is:
 - --adbPath test option
 - build system's substs["ADB"]
 - env["ANDROID_SDK_ROOT"]/platform-tools/adb
 - .mozbuild/android-sdk-linux/platform-tools/adb (might want to re-think this one day)
 - $PATH

I've verified by running each of the affected test suites with and without --adbPath and with and without adb in $PATH.

Comment 8

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

Comment on attachment 8921552 [details] [diff] [review]
use mozrunner to find adb for mach tests

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

looks good. I am not familiar with anything else that might need these changes.  Have you verified these work with autophone properly?  I want to make sure we don't break autophone, and if possible apply these updates there for easier code portability in the future.

Comment 9

[Geoff Brown [:gbrown]]

This does not break autophone. I expect it will not affect autophone at all.

It might be awkward to use the same approach in autophone: autophone doesn't currently use mozrunner and autophone is not integrated with mach the same way. I think autophone relies on adb being in $PATH, and that might be okay. ni to bc in case he wants to improve autophone's ability to find adb and finds inspiration here.

Comment 10

[Bob Clary [:bc] (inactive)]

I think the PATH is sufficient for Autophone's needs for now. My only question would be for running autophone via mach. Geoff, did you try that out?

Comment 11

[Geoff Brown [:gbrown]]

Yes (briefly!) -- looks okay.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dfe93e7c081
Help mach tests find adb when running Android tests; r=jmaher

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d85f1f40c9
Follow-up to fix some minor python lint problems

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/4dfe93e7c081
https://hg.mozilla.org/mozilla-central/rev/a6d85f1f40c9