1163082 - Add configure option and build system for specifying Firefox for Android distribution files

Status: RESOLVED FIXED

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

Description

[Nick Alexander :nalexander [he/him]]

Right now, producing a Fennec APK with an embedded distribution requires an easy-but-magic-and-fragile step of manually copying distribution files into dist/bin at just the right time [1].  This ticket tracks adding a --with-mobile-android-distribution=DIR configure flag and the supporting build system work.

[1] https://wiki.mozilla.org/Mobile/Distribution_Files#Testing_a_distribution_locally

Comment 1

[Chris AtLee [:catlee]]

So the workflow here would be something like:

hg clone https://.../mozilla-central src
git clone https://.../disto.git distro-src
export MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR=${PWD}/distro-src

and have the mozconfig pick up ${MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR}?

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Chris AtLee [:catlee] from comment #1)
> So the workflow here would be something like:
> 
> hg clone https://.../mozilla-central src
> git clone https://.../disto.git distro-src
> export MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR=${PWD}/distro-src
> 
> and have the mozconfig pick up ${MOZ_MOBILE_ANDROID_DISTRIBUTION_DIR}?

Yes, exactly.  Right now we manually copy into objdir/dist/bin, but I'll update the build system to make this less fragile.

Comment 3

[Nick Alexander :nalexander [he/him]]

rnewman, margaret: first, Distribution.java is pretty nice.  Bravo!

I see that the distribution files are expected to be in the root of the APK (well, in /distribution/).  This is not convenient for the build system and not future friendly, because Gradle and others expect such files to go into assets/.  Luckily, this expectation occurs only at one place [1].

I'd like to consider expecting distribution files in the APK to be in /assets/distribution/.  Can you comment on any problems that might cause?  It's possible assets/ doesn't work, but I'll test that.  Since this is baked into the APK, there is no legacy issue.  The only problem might be an existing process that needs to change -- but that's what this ticket is about.  Thoughts?

If we can do this, we can piggy back off the separately interesting Bug 1160563.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/distribution/Distribution.java#698

Comment 4

[:Margaret Leibovic]

I don't see that move being a problem. This logic was originally added back in bug 834681, and I don't see any particular reason we chose the root of the APK, other than convenience.

FYI, you'll need to update the mock-package.zip we use for testDistribution.

Comment 5

[Richard Newman [:rnewman]]

My only concern is that it would turn a single path entry into two path entries, so there's some maintenance and testing burden on adjusting the ZipEntry walker.

Other than testing to make sure it works, I don't see any issues with changing the in-APK path.

(This is ignoring the change for anyone who's doing distro-based repacks, of course -- an email to m-f-d when this lands wouldn't go amiss.)

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://1163082/nalexander

/r/8629 - Bug 1163082 - Part 1: Add ANDROID_ASSETS_DIRS to moz.build. r=gps
/r/8631 - Bug 1163082 - Part 2: Prepare existing TEST_HARNESS_FILES code for re-use. r=froydnj
/r/8633 - Bug 1163082 - Part 3: Add ANDROID_ASSETS_FILES and bake assets into the APK. r=froyndj
/r/8635 - Bug 1163082 - Part 4: Add flexible pattern break |. r=gps
/r/8637 - Bug 1163082 - Part 5: Add --with-android-distribution-directory. r=gps

Pull down these commits:

hg pull -r 58b130436e85f010d6fe34d015becdc841c9ffb4 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Nick Alexander :nalexander [he/him]]

gps, froydnj: figure y'all can work out a reasonable review split.  I elected to approach this by addressing a more general case, namely Bug 1160563, as part of the distribution specific work.  (Observe that this allows writing to anywhere in assets/, not just into assets/distribution or similar.)

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander

glandium: I expect you have an opinion on these path changes too.

For context, I tried to make FINAL_TARGET_FILES use this infrastructure as well but decided not to risk busting a complicated thing.

Comment 9

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/8629/#review7311

::: python/mozbuild/mozbuild/frontend/context.py:563
(Diff revision 1)
> +    'ANDROID_ASSETS_DIRS': (List, list,

Make that a ContextDerivedTypedList(SourcePath) instead of a List, and you get automatic support of topsrcdir-relative paths.

Comment 10

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/8633/#review7313

::: mobile/android/base/moz.build:951
(Diff revision 1)
> +ANDROID_ASSETS_DIRS += [TOPOBJDIR + '/dist/android_assets']

FWIW, I have a patch in my queue that adds support for (top)objdir-relative paths for SourcePath (well, the patch renames it to Path)

Comment 11

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/8635/#review7315

::: python/mozbuild/mozbuild/test/frontend/data/test-harness-files-patterns/moz.build:9
(Diff revision 1)
> +TEST_HARNESS_FILES.e += ["foo/inner|*"]

I get what you're trying to get at, but if I put myself in the boots of someone reading such a moz.build without knowing much about it, I have not the remote idea what the hell this is about. The TEST_HARNESS_FILES.path thing is already kind of confusing, this is adding another layer that I'm not sure is good to have.

Too much magic kills the magic. I think it's time to think about exposing FileFinder-like functions to moz.build so that one can do something like:
for file in find('foo/*'):
    TEST_HARNESS_FILES.a += [f[len('foo/'):]]

which I think reads better. (and we could have helper functions to remove top-levels because f[something:] is kind of ugly)

Comment 12

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #10)
> FWIW, I have a patch in my queue that adds support for (top)objdir-relative
> paths for SourcePath (well, the patch renames it to Path)

Also note I also want to change TEST_HARNESS_FILES to use that Path class instead of what it currently uses.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #11)
> Too much magic kills the magic. I think it's time to think about exposing
> FileFinder-like functions to moz.build so that one can do something like:
> for file in find('foo/*'):
>     TEST_HARNESS_FILES.a += [f[len('foo/'):]]

In fact, this could even be a one liner:
  TEST_HARNESS_FILES.a += [f[len('foo/'):] for f in find('foo/*')]

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #11)
> https://reviewboard.mozilla.org/r/8635/#review7315
> 
> :::
> python/mozbuild/mozbuild/test/frontend/data/test-harness-files-patterns/moz.
> build:9
> (Diff revision 1)
> > +TEST_HARNESS_FILES.e += ["foo/inner|*"]
> 
> I get what you're trying to get at, but if I put myself in the boots of
> someone reading such a moz.build without knowing much about it, I have not
> the remote idea what the hell this is about. The TEST_HARNESS_FILES.path
> thing is already kind of confusing, this is adding another layer that I'm
> not sure is good to have.

More explicitly spelling out things for globbing is a Good Thing.  I honestly get confused about how glob patterns (especially recursive ones) get translated to install paths in TEST_HARNESS_FILES (maybe I'm the only one), and anything that would make globs more explicit gets +1 from me.

Comment 15

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/8629/#review7359

r=me with glandium's comment addressed.  Please followup and fix ANDROID_RES_DIRS the same way.

Comment 16

[Nathan Froyd [:froydnj]]

https://reviewboard.mozilla.org/r/8631/#review7361

r=me with some name bikeshedding

::: python/mozbuild/mozbuild/backend/recursivemake.py:912
(Diff revision 1)
> -    def _process_test_harness_files(self, obj, backend_file):
> +    def _process_files(self, obj, backend_file, varname, destdir, install_manifest):

_process_files sounds a bit generic.  WDYT about calling this _process_abstract_file_list?

::: python/mozbuild/mozbuild/frontend/emitter.py:847
(Diff revision 1)
> +    def _process_files(self, context, varname, factory, allow_install_to_root=True):

Same comment from recursivemake.py applies here.

Comment 17

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8629/#review7367

::: python/mozbuild/mozbuild/frontend/context.py:563
(Diff revision 1)
> +    'ANDROID_ASSETS_DIRS': (List, list,
> +        """Android resource directories.
> +
> +        This variable contains a list of directories, each relative to
> +        the srcdir, containing static files to package into an 'assets/'
> +        directory and merge into an APK file.
> +        """, 'export'),

ANDROID_RES_DIRS already exists. At some point we'll probably want a generic hierarchical container for defining what goes into the APK. Is now that time?

::: build/mobile/robocop/Makefile.in:14
(Diff revision 1)
> -ANDROID_ASSETS_DIR := $(TESTPATH)/assets
> +ANDROID_ASSETS_DIRS := \
> +  $(TESTPATH)/assets \
> +  $(NULL)

Typically when we add variables to moz.build, we move them explicitly. This includes adding variables to a blacklist so occurrences in Makefile.in files trigger an error.

Is there a reason you aren't doing that here?

::: mobile/android/base/Makefile.in:428
(Diff revision 1)
> +		$$(addprefix -A ,$$(ANDROID_ASSETS_DIRS)) \

So we weren't packaging these directories before? What was the point of ANDROID_ASSET_DIR before this patch?

Comment 18

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8631/#review7369

::: python/mozbuild/mozbuild/backend/recursivemake.py:912
(Diff revision 1)
> -    def _process_test_harness_files(self, obj, backend_file):
> +    def _process_files(self, obj, backend_file, varname, destdir, install_manifest):

I wouldn't be surprised if this had a noticeable performance impact! Attribute and key lookups in tight loops are often noticeable in profile output!

::: python/mozbuild/mozbuild/frontend/data.py:230
(Diff revision 1)
> +    pass

You don't need `pass` when you have a docstring. That's because docstrings are part of the AST and satisfy the requirement that a block have content.

::: python/mozbuild/mozbuild/frontend/data.py:226
(Diff revision 1)
> +class TestHarnessFiles(AbstractFileList):

I'm assuming this class will do something useful in a subsequent patch. Please try to document these things in your commit message next time.

Comment 19

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8633/#review7371

::: python/mozbuild/mozbuild/backend/recursivemake.py:496
(Diff revision 1)
>          elif isinstance(obj, TestHarnessFiles):
>              self._process_test_harness_files(obj, backend_file)
>  
> +        elif isinstance(obj, AndroidAssetsFiles):
> +            self._process_android_assets_files(obj, backend_file)

Part of me thinks you should inline these methods while you are here...

::: python/mozbuild/mozbuild/frontend/data.py:236
(Diff revision 1)
> +    pass

Don't need pass.

Comment 20

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/8635/#review7375

Why? You've introduced complexity for no documented reason.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8604454 [details]
MozReview Request: bz://1163082/nalexander

I think I have done enough damage with my reviews here.

Comment 23

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1163082 - Part 3: Add ANDROID_ASSETS_FILES and bake assets into the APK. r=froyndj

Comment 24

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1163082 - Part 4: Add flexible pattern break |. r=gps

Comment 25

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Comment 26

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1163082 - Part 5: Add --with-android-distribution-directory. r=gps

Comment 27

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Comment 28

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8629/diff/1-2/

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8631/diff/1-2/

Comment 30

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25cb1053eff7

Comment 31

[Nathan Froyd [:froydnj]]

Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Confirmed with nalexander that I was not supposed to review this patch.

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

https://reviewboard.mozilla.org/r/8631/#review29261

Comment 33

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8629/diff/2-3/

Comment 34

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/8631/diff/2-3/

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8620258 [details]
MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps

https://reviewboard.mozilla.org/r/8629/#review29905

Comment 36

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/e228040a044b7ff7363a178da2cb0b8b42724048
Bug 1163082 - Part 1: Add --with-android-distribution-directory. r=gps

https://hg.mozilla.org/integration/fx-team/rev/baf25be8d4917e6dcc52eede79e61e1837328c86
Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Comment 37

[Nick Alexander :nalexander [he/him]]

Developer documentation and sample repository updated.  Mailing list post is at https://mail.mozilla.org/pipermail/mobile-firefox-dev/2016-February/001739.html.

Comment 38

[Nick Alexander :nalexander [he/him]]

We might want to uplift this to make it easier to get distributions out with partners.  We can do it as needed.

Comment 39

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/03297f8c28a08d2b39a252c7b368524d9e69da69
Backed out 2 changesets (bug 1163082) for Robocop rc1 failures.

Comment 40

[Nick Alexander :nalexander [he/him]]

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

Comment 41

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/489b15a7dce6b8d26dddc26387ea0d02d057c298
Bug 1163082 - Part 1: Add --with-android-distribution-directory. r=gps

https://hg.mozilla.org/integration/fx-team/rev/926d1440ce98c618194236956342a627fdec4a93
Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Comment 42

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/489b15a7dce6
https://hg.mozilla.org/mozilla-central/rev/926d1440ce98

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Nick, could you fill the uplift request for this bug as it is needed for bug 1234629? Thanks

Comment 44

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman


Approval Request Comment
[Feature/regressing bug #]: needed by bouncer APK (Bug 1234629).

[User impact if declined]: delayed testing from partners.

[Describe test coverage new/current, TreeHerder]: very little automated -- it's almost impossible to automate.  I've tested it manually.

[Risks and why]: very low.  It's possible we'd see build issues in beta; they'd be immediately obvious.  Otherwise this changes no code in Fennec (or any other product) itself.  It's possible it would interact with partner distributions in the wild, but I've taken care that the existing automated tests don't change in that regard.

[String/UUID change made/needed]: none.

I'm happy to co-ordinate uplift patches myself.

There's a dependency on getting the Android partner sample builds to work so that we can actually check out the partner distribution and get it into the bouncer APK.  I'll work with jlund to make the actual builds happen.

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8620260 [details]
MozReview Request: Bug 1163082 - Part 2: Extract Android distribution from packaged assets rather than APK root. r=rnewman

Taking it.
Should be in 45 beta 8
If you can take of the uplift, it would be great.

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2d3db6c264ba
https://hg.mozilla.org/releases/mozilla-beta/rev/7619a19b83cb

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/776b5ddd5667
https://hg.mozilla.org/releases/mozilla-aurora/rev/87206efd5e93