1521996 - Separate Desktop and Android `mach run` and `mach install` implementations

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Benjamin De Kosnik [:bdekoz]]

I'm building gecko trunk for --target-i686-linux-android on a linux host (f29) and am set up via a USB link to an android target device.

ADB setup, build, package, install, no-argument run etc are fine. However, when I go to test the new build with a specific profile directory (and user.js file), I run into problems.

mkdir android-profile
cp user.js ./android-profile
adb push android-profile /data/local/tmp/
./mach -v run --profile /data/local/tmp/android-profile

No joy. I also tried other arguments, seemingly without use-value:

./mach -v run --setings FILE
./mach -v run --new-window URL

What does work:

./mach run

Comment 1

[Geoff Brown [:gbrown]]

As far as I know, we have only ever attempted to support basic 'mach run' on android: I expect all arguments are ignored on android.

Comment 2

[Geoff Brown [:gbrown]]

There is some old documentation (may not be entirely accurate) at

https://wiki.mozilla.org/Mobile/Fennec/Android/AdvancedTopics#Arguments_and_Environment_Variables

that may help with some of this.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Brown [:gbrown] from comment #1)

As far as I know, we have only ever attempted to support basic 'mach run' on android: I expect all arguments are ignored on android.

It's a little more complicated. Arguments are getting passed through, but they're not getting interpreted by mach run itself. So the help string is completely wrong and you need to have knowledge of how Gecko itself parses arguments, etc. We can do a little better here, although (IIRC) making mach have totally different command implementations with different argument sets per-platform is hard to achieve.

Comment 4

[Nick Alexander :nalexander [he/him]]

In the same vein: can we make it easier to debug GV-based things with this? See Bug 1522318 for a use case.

Comment 5

[Benjamin De Kosnik [:bdekoz]]

Hey thanks for the hint Geoff.

I can get at the -d URL issue and other tweaks as per help from Nick yesterday, so I'm further along. So close!

I read the doc you pointed at
https://wiki.mozilla.org/Mobile/Fennec/Android/AdvancedTopics#Arguments_and_Environment_Variables

And tried to run the --profile example (with a profile I created with 'adb shell' in the form of /mnt/sdcard2/moz-perf-profile) under ADB with limited results:

%adb shell am start -a android.intent.action.MAIN -c android.intent.category.LAUNCHER -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp --es args '--profile /mnt/sdcard2/moz-perf-profile'
Starting: Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=/mnt/sdcard2/moz-perf-profile (has extras) }
Error type 3
Error: Activity class {/mnt/sdcard2/moz-perf-profile} does not exist.

This cmp=XXX part appears to be wrong, as

cmp=/mnt/sdcard2/moz-perf-profile

should be

cmp=org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp

Comment 6

[Geoff Brown [:gbrown]]

Many automated tests launch fennec with a custom profile. Mochitest test logs show how that's done. Here's an example:

[task 2019-01-24T11:17:29.218Z] 11:17:29 INFO - adb launch_application: am start -W -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp -a android.intent.action.VIEW --es env9 MOZ_UPLOAD_DIR=/sdcard/tests/mozlog --es env8 R_LOG_DESTINATION=stderr --es args "-no-remote -profile /sdcard/tests/profile//" --es env3 DISABLE_UNSAFE_CPOW_WARNINGS=1 --es env2 R_LOG_VERBOSE=1 --es env1 XPCOM_DEBUG_BREAK=stack --es env0 MOZ_CRASHREPORTER=1 --es env7 MOZ_LOG_FILE=/sdcard/tests/mozlog/moz.log --es env6 MOZ_CRASHREPORTER_SHUTDOWN=1 --es env5 MOZ_IN_AUTOMATION=1 --es env4 MOZ_DISABLE_NONLOCAL_CONNECTIONS=1 --es env12 MOZ_HIDE_RESULTS_TABLE=1 --es env11 R_LOG_LEVEL=6 --es env10 MOZ_CRASHREPORTER_NO_REPORT=1 -d "http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&logFile=%2Fsdcard%2Ftests%2Flogs%2Fmochitest.log&fileLevel=INFO&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Fsdcard%2Ftests"

Comment 7

[Geoff Brown [:gbrown]]

It looks like there's just one hyphen in -profile - NOT --profile!

Comment 8

[Benjamin De Kosnik [:bdekoz]]

Thanks Geoff. Any chance you point at a URL for a log I can scope please?

Here's what I am doing ATM

adb push moz-perf-profile /mnt/sdcard2/

adb shell am start -a android.intent.action.MAIN -c android.intent.category.LAUNCHER -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp -d about:blank

adb shell

am start -W -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp -a android.intent.action.VIEW --es args "-no-remote -profile /mnt/sdcard2/moz-perf-profile/" -d https://www.instagram.com/justintimberlake/

which seems to work on the device, but then when I go inspect the profile directory, it only contains my copied-in user.js file, and nothing else, which indicates to me that this is not working....

Comment 9

[Geoff Brown [:gbrown]]

Any of the Android 4.3 test logs from treeherder should do. Like:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=223901610&repo=mozilla-central&lineNumber=1462

Our test jobs start from a freshly booted Android image, install Firefox for Android, copy the profile to the device, then launch Firefox for Android. From comment 8, it looks like Firefox is already started with a default profile, then you direct it to the new profile...but I bet that "second" Firefox is the same process as the first one...I wonder what happens to the profiles in that case. I'd try:

adb shell am force-stop org.mozilla.fennec_aurora
adb push moz-perf-profile /mnt/sdcard2/
adb shell am start -W -n org.mozilla.fennec_aurora/org.mozilla.gecko.BrowserApp -a android.intent.action.VIEW --es args "-no-remote -profile /mnt/sdcard2/moz-perf-profile/" -d https://www.instagram.com/justintimberlake/

Comment 10

[Benjamin De Kosnik [:bdekoz]]

profile dir must be on sdcard0 on fireOS, apparently. external sd cards (sdcard1) do not have permissions.

/sdcard/Download/profile-dir

seems to work

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1521996 - Part 1: Add `conditional_name` to mach @Command definition. r?ahal

We want mach run for Android to be wildly different than mach run
for Desktop. But right now, mach really doesn't support two different
implementations of the same underlying named command. The avenues
that might support different implementations, mostly run through
conditions.

conditions were added to mach commands in Bug 901972, and never
really anticipated this use case: commands are keyed by name condition
evaluation is delayed until dispatch-time. In order to have different
commands with the same name, and have full support for --help,
command matching, suggestions, etc, we really need conditions to
evaluate at parse-time. Indeed, since Bug 901972 landed, we've moved
context creation earlier in the dispatch flow and hacked in things
that look like parse-time conditions (see Bug 1291335 and Bug
1305695).

This approach is not the prettiest, but it handles this narrow
use-case -- making mach run and mach install different on Android
-- without much code churn.

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1521996 - Part 2: Specialize `mach {run,install}` for Android. r?ahal,gbrown

This just separates out the Android definitions into
mobile/android/mach_commands.py. There was vestigial support for
running on Android with debuggers, but it was for wiring up JimDB,
which is no longer supported and in fact hasn't worked on actual
devices for a very long time. (The new flow for running on Android
under a debugger goes through the Android Studio hybrid debugger.)

Depends on D18290

Comment 13

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1521996 - Part 3: Make `mach run` for Android bring Fennec forward by default. r?gbrown

mach run as it is doesn't really parallel mach run on Desktop;
this makes it a little closer more fully featured. The underlying
functionality is all there in layers of mozharness; let's make it
easier to get to.

Depends on D18291

Comment 14

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1521996 - Part 4: Generalize `mach run` on Android to GeckoView-based targets. r?gbrown

This turned out to be finicky, because mozharness has layers of
helpers that don't bottom out in the layer we actually want, which is
a GeckoView-specific layer. Specifically:

• launch_application: does not handle moz_env.
• launch_fennec: hard-codes "org.mozilla.gecko.BrowserApp".
• launch_activity: hard-codes "{app}.Activity" (i.e., does not accept a fully qualified class name).

The e10s does appear to be required, so we handle it specially.

I expected this to work for
org.mozilla.geckoview.test/org.mozilla.geckoview.test.TestRunnerActivity
but it appears not to, so I've removed it. I think that's missing
functionality in TestRunnerActivity, not an error in the intent
dispatch encoded here.

Depends on D18292

Comment 15

[Nick Alexander :nalexander [he/him]]

The patch series that I posted doesn't handle the things bdekoz actually wants (prefs, settings, profile manipulation) but it sets things up: it stops mach run from lying about what's supported (!) and shows how to expose existing functionality from mozharness and the existing layers to consumers.

I have limited time to push this, so I wanted to get feedback (in the form of first review) before trying to do any more. Try build is percolating at

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

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3a0fedff65e
Part 1: Add `conditional_name` to mach @Command definition. r=ahal
https://hg.mozilla.org/integration/autoland/rev/376efab28772
Part 2: Specialize `mach {run,install}` for Android. r=ahal,gbrown
https://hg.mozilla.org/integration/autoland/rev/c0df3469a4cb
Part 3: Make `mach run` for Android bring Fennec forward by default. r=gbrown

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/e3a0fedff65e
https://hg.mozilla.org/mozilla-central/rev/376efab28772
https://hg.mozilla.org/mozilla-central/rev/c0df3469a4cb

Comment 18

[Andrew Halberstadt [:ahal]]

I know I reviewed this and should have thought of this back then, but I was doing some debugging in bug 1580280 and realized this causes us to invoke configure before mach even parses the command line. So e.g, every mach invocation ever is going into configure.

So I just wanted to point out that mach supports a callable parser, so it would be totally possible to have different command line arguments for Android vs Desktop in a single mach implementation (as well as branching in the implementation itself). In fact, mochitest's ArgumentParser has this exact condition already:
https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/mochitest/mach_commands.py#212

In other words, we could scrap conditional_names entirely and re-work the install and run commands to have different arguments and branches depending on whether we have an Android build or not. Wish I thought of this during review.

Anyway, it's not a super high priority or anything.. but I think it would be good to avoid using more conditional_names in the future.

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Andrew Halberstadt [:ahal] from comment #18)

I know I reviewed this and should have thought of this back then, but I was doing some debugging in bug 1580280 and realized this causes us to invoke configure before mach even parses the command line. So e.g, every mach invocation ever is going into configure.

So I just wanted to point out that mach supports a callable parser, so it would be totally possible to have different command line arguments for Android vs Desktop in a single mach implementation (as well as branching in the implementation itself). In fact, mochitest's ArgumentParser has this exact condition already:
https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/mochitest/mach_commands.py#212

In other words, we could scrap conditional_names entirely and re-work the install and run commands to have different arguments and branches depending on whether we have an Android build or not. Wish I thought of this during review.

Anyway, it's not a super high priority or anything.. but I think it would be good to avoid using more conditional_names in the future.

This is helpful to know, but could you also file a ticket tracking this?

Regardless of what we do with conditional names, there's still an issue, 'cuz just asking for the build configuration shouldn't trigger configure. I think this is something new with the transition to Python 3. Definitely I've tested running mach lint --linter android-lint with a Desktop mozconfig and with no mozconfig at all and I didn't see mach configure running. Do you see this when running with Python 3 as well?

Comment 20

[Andrew Halberstadt [:ahal]]

Filed bug 1584567 to track removing conditional_names. I actually have patches that remove them already because I thought it might fix my android-lints issue (still pending results).

But yeah, I agree there's something else going on here, this definitely only happens with Python 3. Personally my goal is to avoid having to debug environment variable setting in configure.. I'm the wrong person for that task. But someone will have to figure it out sooner or later (even if we punt on it for now).

Fwiw, I've actually seen the MOZ_AUTOMATION issue before:
https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/mach#154

So reproducing it won't be a problem.

Edit: Let's move discussion back to bug 1580280.