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

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}?

(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.

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

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.

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.)

/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/

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 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.

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.

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)

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)

(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.

(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/*')]

(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.

https://reviewboard.mozilla.org/r/8629/#review7359 r=me with glandium's comment addressed. Please followup and fix ANDROID_RES_DIRS the same way.

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.

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?

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.

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.

https://reviewboard.mozilla.org/r/8635/#review7375 Why? You've introduced complexity for no documented reason.

Comment on attachment 8604454 [details] MozReview Request: bz://1163082/nalexander I think I have done enough damage with my reviews here.

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 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 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 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 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 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 on attachment 8620258 [details] MozReview Request: Bug 1163082 - Part 1: Add --with-android-distribution-directory. r?gps https://reviewboard.mozilla.org/r/8629/#review29905

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

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

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

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

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

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

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

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 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.

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

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