adb path defaults to $PATH instead of .mozbuild path

RESOLVED FIXED in Firefox 58

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aastha.gupta4104, Assigned: gbrown)

Tracking

55 Branch
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments)

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.
Flags: needinfo?(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.
Component: mach → Mozbase
Flags: needinfo?(gps) → needinfo?(bob)
Product: Core → Testing
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!
Attachment #8920893 - Flags: feedback+
gbrown: can you suggest a way to share the mozbase/mozrunner and mozdevice code?
Flags: needinfo?(bob) → needinfo?(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
Assignee: nobody → gbrown
Flags: needinfo?(gbrown)
(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.
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.
Attachment #8921552 - Flags: review?(jmaher)
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.
Attachment #8921552 - Flags: review?(jmaher) → review+
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.
Flags: needinfo?(bob)
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?
Flags: needinfo?(bob)
Yes (briefly!) -- looks okay.
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dfe93e7c081
Help mach tests find adb when running Android tests; r=jmaher
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d85f1f40c9
Follow-up to fix some minor python lint problems
https://hg.mozilla.org/mozilla-central/rev/4dfe93e7c081
https://hg.mozilla.org/mozilla-central/rev/a6d85f1f40c9
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.