1611554 - Generate GeckoView's AndroidManifest.xml from template

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

For e10s-multi, we're going to need to specify N services, where N is some maximum number of content processes. Instead of doing that manually, we should be able to specify a manifest template, with the build system generating the final manifest based on a set of arguments.

e.g. Chromium uses jinja for generating their manifest.

Comment 1

[Nick Alexander :nalexander [he/him]]

I'm sure I've written this in tickets before, but:

1. Strongly prefer doing this outside of Gradle. Inside Gradle, you will be fighting the Android-Gradle build plugin very hard. (I've spent a long time trying to figure out how to make it work, and conclude that before about version 3.3 it's not feasible.)

2. If it could be part of the pre-export, it should Just Work with a GENERATED_FILE definition. Well, mach build would work, but modifying the definitions/template and then building within Android Studio might not see those changes. But I think even AS would see those changes.

3. Strongly prefer generating an "overlay" manifest in debug/AndroidManifest.xml that just has the bits that vary, so that Android Studio can find the "main" manifest at Gradle configuration time. Android Studio will be very unhappy if that can come and go.

Overall, it seems to me that pre-declaring 16 processes and then managing them at runtime makes more sense than declaring the maximum at build time.

One trick that might be possible here is to use the tools: XML namespace to toggle android:disabled dynamically. Or, maybe simpler, is it possible to use manifest placeholders with disabled? Like

<declaration N>
  <android:disabled=$placeholderN>
</declaration N>

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Nick Alexander :nalexander [he/him] from comment #1)

Overall, it seems to me that pre-declaring 16 processes and then managing them at runtime makes more sense than declaring the maximum at build time.

That's not how Android works, unfortunately. Every service that the application wants must be declared in the application's manifest.

This is relaxed somewhat beginning with Android 10, but to do that we still need at least one service definition in the manifest and our services need to support isolatedProcess=true, which they currently do not. And, of course, that, that doesn't help us with older Android versions.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1611554: Part 1 - Add capability for build system to generate files during pre-export tier when building GeckoView; r=nalexander

These are the minimum changes that we need to make to common build system code
to allow us to generate files during pre-export.

We add a required_before_export flag to GeneratedFile to indicate when a
particular file must be generated in pre-export. We set that flag when there
are .jinja input files and we're configured for a GeckoView build, otherwise
it is set to False.

Then the recursive make backend assigns any GeneratedFiles that have
required_before_export set to run in the pre-export tier.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1611554: Part 2 - Modify GeckoView build to generate manifest and service definitions from jinja files; r=nalexander!,#geckoview-reviewers

(Nick, I know that in the bug you commented that you would prefer a manifest
overlay; I spent a long time reading docs and had a hard time figuring out how
that needs to look if we want these generated manifest augmentations to be
present for every variant. If you still feel that we need to take that approach
here, I would definitely appreciate some hints!)

• We add a config option for setting the number of content services;
• We add a config option to indicate whether content services should be isolated.
  This one is just a project_flag since it doesn't really need the ability to
  be overridden; it's something whose default we would want to flip when the
  time comes;
• We set a dependency so that mobile/android/base/pre-export is executed;
• We add the gen_from_jinja.py script which is mostly just a dumb shim that
  takes the input template and the config arguments, instantiates jinja,
  generates the final output, and dumps it to the output fd.
• We add the requisite moz.build statements to generate the manifest and the
  service definitions.
• We update build.gradle so that Gradle knows to look for the generated files
  when building the apk.

Depends on D82576

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Bug 1611554: Part 3 - Use jinja templates to generate GeckoChildProcessServices.java and AndroidManifest_overlay.xml; r=#geckoview-reviewers

This patch converts AndroidManifest.xml and GeckoChildProcessServices.java
into jinja templates.

Note that even though Gradle supports simple substitution of variables in
manifests, I opted not to use that functionality. Since we need the more
powerful template functionality that jinja provides, I felt that having multiple
ways to substitute information into the manifest would be confusing, so we're
using jinja exclusively.

Depends on D82577

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1611554: ALTERNATIVE APPROACH - TO FOLD/ABSORB - Preprocess just an overlay rather than the whole manifest.

This leverages Android manifest
merging to
merge in the generated pieces rather than preprocessing the whole
manifest. The advantage that I see is that tooling, both the
Android-Gradle plugin itself at configuration time and Android
Studio, will always see a valid AndroidManifest.xml. This matters
in edge cases: running a Gradle command in an unconfigured tree, or
just using the IDE when its watching for file changes, when
clobbering.

Comment 7

[Nick Alexander :nalexander [he/him]]

aklotz: hey! This seems to have stalled, and I'd hate to see something so close die on the vine.

Refreshing my memory of the patch series, I see:

1. r+ from build peers on the .jinja approach. This feels like there just some small tweaks to generalize first patch.

2. a real open question about manifest overlays -- did you run with this locally? Does it work for you?

3. a few small nits.

Is there some other context that's blocking this, or did it just fall by the way side? I'm not militant about making changes; if you have no energy for this project let's address some nits and land without the overlay approach.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

I still want to land it, but my attention has been diverted to a sec bug that needs to be dealt with.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc905e5db0cc
Part 1 - Add capability for build system to generate files during pre-export tier when building GeckoView; r=nalexander,rstewart
https://hg.mozilla.org/integration/autoland/rev/0d4317da1006
Part 2 - Modify GeckoView build to generate manifest and service definitions from jinja files; r=nalexander,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/bb1ce63d73ec
Part 3 - Use jinja templates to generate GeckoChildProcessServices.java and AndroidManifest_overlay.xml; r=geckoview-reviewers,agi

Comment 10

[Atila Butkovits]

Backed out for causing bustage on recurse.mk.

Backout link: https://hg.mozilla.org/integration/autoland/rev/687d3f798e03419c1a64fb919f275a7d893c738e

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Gjy3KQHGSkKADWw4cE7lfQ.0&resultStatus=testfailed%2Cbusted%2Crunnable&revision=bb1ce63d73ec74c3d8a12bdaa29d1a16af0cb8b2&searchStr=build

Failure log: https://treeherder.mozilla.org/logviewer?job_id=326533544&repo=autoland&lineNumber=1280

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0edf97f5cb76
Part 1 - Add capability for build system to generate files during pre-export tier when building GeckoView; r=nalexander,rstewart
https://hg.mozilla.org/integration/autoland/rev/373f29f3f07c
Part 2 - Modify GeckoView build to generate manifest and service definitions from jinja files; r=nalexander,geckoview-reviewers,snorp
https://hg.mozilla.org/integration/autoland/rev/cd593818662e
Part 3 - Use jinja templates to generate GeckoChildProcessServices.java and AndroidManifest_overlay.xml; r=geckoview-reviewers,agi

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0edf97f5cb76
https://hg.mozilla.org/mozilla-central/rev/373f29f3f07c
https://hg.mozilla.org/mozilla-central/rev/cd593818662e

Comment 13

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/0edf97f5cb76
https://hg.mozilla.org/mozilla-central/rev/373f29f3f07c
https://hg.mozilla.org/mozilla-central/rev/cd593818662e