Right now, Fennec APK packaging is in https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files-APK.mk, and it's a mess. This ticket tracks moving it to Python, and hopefully simplifying some things along the way.

This allows to interrogate the data in a Jarrer. Review commit: https://reviewboard.mozilla.org/r/42853/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42853/

I couldn't think of a better way to pass arguments into the Jarrer for a single file. While I was here, I expanded mozjar just a little. Review commit: https://reviewboard.mozilla.org/r/42855/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42855/

A few notes: * This doesn't accommodate general OMNIJAR_NAME definitions. The current name (assets/omni.ja) is baked into the product in a few places, and is very unlikely to change, so let's not address that right now. * This makes the package-manifest.in file authoritative for what goes into assets/, libs/, and the APK root. Previously, package-manifest.in wrote into assets/ and libs/ but upload-files-APK.mk also had a convoluted DIST_FILES filtering process to work through before a file actually made it into the APK. * This is intentional about repackaging. It simplifies the repackage step rather than trying to make unpackage-then-repackage the same as just package. I pretty much never get repackaging correct the first time; this should help. (I've manually tested it.) * This doesn't yet store (rather than deflate) szipped libraries in assets/. Review commit: https://reviewboard.mozilla.org/r/42857/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42857/

The ALREADY_SZIPPED during repackaging is subsumed by the previous check if UNPACKAGE is set. The custom linker expects stored, not deflated, libraries, so there's some small legwork to accommodate that in mozjar. Review commit: https://reviewboard.mozilla.org/r/42859/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42859/

glandium: I gave the reviews to you, for old time's sake, but they could have as easily gone to mshal. Feel free to re-assign. I am pretty sure this will handle repackaging, but the Official Plan is to break it and then fix it :)

Comment on attachment 8735568 [details] MozReview Request: Bug 1260241 - Pre: Allow to read from DeflatedFile instances. r?glandium https://reviewboard.mozilla.org/r/42853/#review39369 ::: python/mozbuild/mozpack/files.py:508 (Diff revision 1) > + def read(self): > + '''Return the contents of the file.''' > + self.file.seek(0) > + return self.file.read() Why make DeflateFiles different from BaseFile in this regard? Note that so far, the code using the mozpack.file classes are just doing .open().read() when they need to read the files. Why not just do that?

https://reviewboard.mozilla.org/r/42857/#review39373 ::: python/mozbuild/mozbuild/test/action/test_package_fennec_apk.py:51 (Diff revision 1) > + # Everything else is in place. > + for name in ('classes.dex', > + 'assets/asset.txt', > + 'lib/lib.txt', > + 'root_file.txt'): > + self.assertEquals(jarrer[name].read().strip(), name) glandium: I implemented the part 1 read() to be able to do this. Should this be `.open().read()`?

(In reply to Nick Alexander :nalexander from comment #7) > https://reviewboard.mozilla.org/r/42857/#review39373 > > ::: python/mozbuild/mozbuild/test/action/test_package_fennec_apk.py:51 > (Diff revision 1) > > + # Everything else is in place. > > + for name in ('classes.dex', > > + 'assets/asset.txt', > > + 'lib/lib.txt', > > + 'root_file.txt'): > > + self.assertEquals(jarrer[name].read().strip(), name) > > glandium: I implemented the part 1 read() to be able to do this. Should > this be `.open().read()`? .open().read() would do the same thing, yes. If you feel strongly about adding a read() method as a shortcut, please add it to the base class.

(In reply to Mike Hommey [:glandium] from comment #8) > (In reply to Nick Alexander :nalexander from comment #7) > > https://reviewboard.mozilla.org/r/42857/#review39373 > > > > ::: python/mozbuild/mozbuild/test/action/test_package_fennec_apk.py:51 > > (Diff revision 1) > > > + # Everything else is in place. > > > + for name in ('classes.dex', > > > + 'assets/asset.txt', > > > + 'lib/lib.txt', > > > + 'root_file.txt'): > > > + self.assertEquals(jarrer[name].read().strip(), name) > > > > glandium: I implemented the part 1 read() to be able to do this. Should > > this be `.open().read()`? > > .open().read() would do the same thing, yes. If you feel strongly about > adding a read() method as a shortcut, please add it to the base class. I do not; I'll bump the tests for the next iteration. (BTW, I observe locally that I forgot to set default=[] for my nargs='*' variables. I'll address for the next iteration.)

Comment on attachment 8735569 [details] MozReview Request: Bug 1260241 - Pre: Allow to store (rather than deflate) files into Jarrers. r?glandium https://reviewboard.mozilla.org/r/42855/#review39377 ::: python/mozbuild/mozpack/files.py:514 (Diff revision 1) > '''Return the contents of the file.''' > self.file.seek(0) > return self.file.read() > > > +class StoredFile(File): Confusingly, in the context of Zips, stored means "non compressed". Attaching a compress and compress_level to such a class rubs me the wrong way. I don't see much value for the compress_level, too. What you really want is a way for some files to be non deflated in the archive. Now, with all that being said, I don't think the information of whether a file should be stored or deflated in a Jarrer is something that should be done with a completely separate type. For the sake of argument, let's say we want to not compress XPTFiles in the future. How would the StoredFile class help with that? It seems to me a better approach would be to "overload" Jarrer.add with def add(self, path, content, compress=True) and keep track of which files are requested not to be compressed. Then in Jarrer.copy, use that information instead of self.compress when creating the DeflaterDests (you don't need to manually jar.add) This would also, I believe, be simpler.

Comment on attachment 8735570 [details] MozReview Request: Bug 1260241 - Part 1: Implement Fennec packaging in Python. r?glandium https://reviewboard.mozilla.org/r/42857/#review39379 ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:33 (Diff revision 1) > + > + # First, take input files. The contents of the later files overwrites the > + # content of earlier files. > + for input in inputs: > + jar = JarReader(input) > + for path in jar.entries.iterkeys(): You can do: for file in jar: path = jar.filename ... ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:49 (Diff revision 1) > + for assets_dir in assets_dirs: > + finder = FileFinder(assets_dir) > + for p, f in finder.find('**'): > + add(os.path.join('assets', p), f) While this makes the review easier, I'm not a big fan of broken intermediate state, so it would be better to land part 1 and 2 folded. ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:52 (Diff revision 1) > + jarrer.add(path, file) > + > + for assets_dir in assets_dirs: > + finder = FileFinder(assets_dir) > + for p, f in finder.find('**'): > + add(os.path.join('assets', p), f) Note that the mozpack APIs want mozpaths, not os.paths. While using os.path will work in practice, using it will make things harder to the people trying to get Android builds working on Windows. ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:62 (Diff revision 1) > + add(os.path.join('lib', p), f) > + > + for root_file in root_files: > + add(os.path.basename(root_file), File(root_file)) > + > + # TODO: verify OMNIJAR_NAME is assets/omni.ja. Why TODO? just import buildconfig, and check it :) ::: python/mozbuild/mozbuild/test/action/test_package_fennec_apk.py:34 (Diff revision 1) > + # Language repacks take updated resources from an ap_ and pack them > + # into an apk. Make sure the second input overrides the first. doesn't seem to be what this test is about. ::: toolkit/mozapps/installer/upload-files-APK.mk (Diff revision 1) > - $(RELEASE_JARSIGNER) $(ABS_DIST)/gecko.apk && \ > - $(ZIPALIGN) -f -v 4 $(ABS_DIST)/gecko.apk $(PACKAGE) What happens to jar signing and zipalign?

Comment on attachment 8735571 [details] MozReview Request: Bug 1260241 - Implement Fennec packaging in Python. r?glandium https://reviewboard.mozilla.org/r/42859/#review39381 ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:55 (Diff revision 1) > if jarrer.contains(path): > jarrer.remove(path) > jarrer.add(path, file) > > for assets_dir in assets_dirs: > - finder = FileFinder(assets_dir) > + finder = FileFinder(assets_dir, find_executables=False) Not sure why you need find_executables=False here and below. ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:103 (Diff revision 1) > + parser.add_argument('--store-assets-libs', default=False, > + action='store_true', > + help='Store (rather than deflate) assets/**/*.so ' > + 'when packing into APK file.') > + parser.add_argument('--szip-assets-libs', default=False, > + action='store_true', > + help='IN PLACE szip assets/**/*.so ' > + 'BEFORE packing into APK file.') > + parser.add_argument('--szip', default=None, > + help='Path to szip host binary.') All these options seem redundant: - You can't --szip-assets-libs without --szip - You can't --szip-assets-libs without --store-assets-libs - I don't seen any use for --store-assets-libs without --szip-assets-libs. ::: toolkit/mozapps/installer/upload-files-APK.mk:153 (Diff revision 1) > + --store-assets-libs \ > + $(if $(COMPILE_ENVIRONMENT),$(if $(MOZ_ENABLE_SZIP),--szip-assets-libs --szip $(ABS_DIST)/host/bin/szip)) \ By unconditionally adding --store-assets-libs, you're effectively changing how libraries are added to the apk in the case MOZ_ENABLE_SZIP is not set. Specifically, currently, they *are* compressed in the case MOZ_ENABLE_SZIP is not set.

https://reviewboard.mozilla.org/r/42859/#review39385 ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:79 (Diff revision 1) > for root_file in root_files: > add(os.path.basename(root_file), File(root_file)) > > # TODO: verify OMNIJAR_NAME is assets/omni.ja. > if omni_ja: > add(os.path.join('assets', 'omni.ja'), File(omni_ja)) omni.ja ought to be stored uncompressed in the apk.

This required handling Deflater instances with different compression options in JarWriter, which in turn required exposing the uncompressed data in Deflater. Review commit: https://reviewboard.mozilla.org/r/42909/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/42909/

Comment on attachment 8735571 [details] MozReview Request: Bug 1260241 - Implement Fennec packaging in Python. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42859/diff/1-2/

Comment on attachment 8735571 [details] MozReview Request: Bug 1260241 - Implement Fennec packaging in Python. r?glandium Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42859/diff/2-3/

Comment on attachment 8735673 [details] MozReview Request: Bug 1260241 - Pre: Add compress option to Jarrer.add. r?glandium https://reviewboard.mozilla.org/r/42909/#review39715 ::: python/mozbuild/mozpack/copier.py:487 (Diff revision 1) > for details on the compress and optimize arguments. > ''' > self.compress = compress > self.optimize = optimize > self._preload = [] > + self._compress_options = {} # Map path to compress boolean option. PEP8 wants two spaces before inline comments. ::: python/mozbuild/mozpack/copier.py:549 (Diff revision 1) > + compress = self._compress_options.get(path, None) > + if compress is None: > + compress = self.compress You're not storing None in self._compress_options, so you could replace this with compress = self._compress_options.get(path, self.compress) ::: python/mozbuild/mozpack/mozjar.py:602 (Diff revision 1) > + elif isinstance(data, Deflater): > + # A Deflator, but with a different compression option. > + deflater.write(data.uncompressed_data) You shouldn't be hitting this case, since you're passing an explicit compress in Jarrer.copy. I'd just remote this. ::: python/mozbuild/mozpack/mozjar.py:762 (Diff revision 1) > + @property > + def uncompressed_data(self): > + ''' > + Return the uncompressed data. > + ''' > + return self._data.getvalue() > + Likewise ::: python/mozbuild/mozpack/test/test_copier.py:526 (Diff revision 1) > + > + dest.seek(0) > + jar = JarReader(fileobj=dest) > + self.assertTrue(jar['foo/bar'].compressed) > + self.assertFalse(jar['foo/qux'].compressed) > + trailing whitespace

Comment on attachment 8735571 [details] MozReview Request: Bug 1260241 - Implement Fennec packaging in Python. r?glandium https://reviewboard.mozilla.org/r/42859/#review39713 ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:59 (Diff revision 3) > + jarrer.add(path, file, compress=compress) > + > + for assets_dir in assets_dirs: > + finder = FileFinder(assets_dir, find_executables=False) > + for p, f in finder.find('**'): > + compress = True set it to None to make it use whatever the default is? ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:69 (Diff revision 3) > + # returned by the finder is not yet opened. When it is > + # opened, it will "see" the content updated by szip. > + subprocess.check_output([szip_assets_libs_with, > + mozpath.join(finder.base, p)]) > + > + if open(mozpath.join(finder.base, p)).read(4) == 'SeZz': you can do f.open().read(4) ::: python/mozbuild/mozbuild/action/package_fennec_apk.py:115 (Diff revision 3) > + 'into APK file using the given szip executable.') > + parser.add_argument('--root-files', nargs='*', default=[], > + help='Optional files to pack into APK file root.') > + args = parser.parse_args(args) > + > + if buildconfig.substs.get('OMNIJAR_NAME', '') != 'assets/omni.ja': You can skip the `, ''`

https://reviewboard.mozilla.org/r/42909/#review39715 > You shouldn't be hitting this case, since you're passing an explicit compress in Jarrer.copy. I'd just remote this. I tried to comment to explain this, but apparently it still isn't clear. I am definitely hitting this -- my tests fail without it! `Deflater` is handled... as long as the compression is the same. When it's not, `Deflater` doesn't expose any of the other instance methods. Hence this and `uncompressed_data` below.

https://reviewboard.mozilla.org/r/42909/#review39715 > I tried to comment to explain this, but apparently it still isn't clear. I am definitely hitting this -- my tests fail without it! `Deflater` is handled... as long as the compression is the same. When it's not, `Deflater` doesn't expose any of the other instance methods. Hence this and `uncompressed_data` below. If I remove your changes from python/mozbuild/mozpack/mozjar.py, your test still passes for me...

(and I see no reason why it shouldn't)

Because the Jarrer code is always creating a Deflater with the same compression as the one given to JarWriter.add.

https://hg.mozilla.org/integration/fx-team/rev/bc439c65ecdcabcf039b9a4c6074380088d8530a Bug 1260241 - Pre: Add compress option to Jarrer.add. r=glandium https://hg.mozilla.org/integration/fx-team/rev/99f8eb253c8769c6752a98be0306249ed05d7731 Bug 1260241 - Implement Fennec packaging in Python. r=glandium https://hg.mozilla.org/integration/fx-team/rev/69f271e79c56618f9a36cd0649e02785e7fde82c Bug 1260241 - Post: Document how to run Android single-locale repacks. r=me

mshal, others: I decided to just push my documentation and handle follow-up as needed. Please give https://hg.mozilla.org/integration/fx-team/rev/69f271e79c56618f9a36cd0649e02785e7fde82c a read.

What is missing for it to work with an artifact build? Isn't the package that gets pulled down for an artifact build just a regular en-US build? It seems odd to say both: l10n repacks do not work with artifact builds. and: You must have ... or a pre-built ``en-US`` package if in fact the artifact build gets you a pre-built en-US package. Otherwise, looks pretty reasonable to document the existing process. Hopefully we can move all of those commands inside a './mach repackage AB_CD' or some such command soon instead of listing the steps in a doc.

(In reply to Michael Shal [:mshal] from comment #26) > What is missing for it to work with an artifact build? There was a discussion in #build about this. Artifact builds use multiple build backends, including the FasterMake build backend. That build backend doesn't expose JAR_MANIFEST to the RecursiveMake backend, so we'd don't process any repack stuff at all. Isn't the package > that gets pulled down for an artifact build just a regular en-US build? It > seems odd to say both: > > l10n repacks do not work with artifact builds. Could say "build configurations" or "mozconfigs". > > and: > > You must have ... or a pre-built ``en-US`` package > > if in fact the artifact build gets you a pre-built en-US package. > > Otherwise, looks pretty reasonable to document the existing process. > Hopefully we can move all of those commands inside a './mach repackage > AB_CD' or some such command soon instead of listing the steps in a doc. I agree. Pretty happy to make this how you *actually* repackage.

You can actually make l10n repacks work with artifact builds instead of rejecting them. That's the part of the conversation that went missing because the meeting was starting. You just need to change the default backend when a l10n base is given. There is a complication, though, in that --with-l10n-base is defined in toolkit/moz.configure making it not always defined (standalone js doesn't have it), and build backend is in /moz.configure, and is everywhere. So there would need to be the same kind of trick done for gonkdir.

(In reply to Mike Hommey [:glandium] from comment #28) > You can actually make l10n repacks work with artifact builds instead of > rejecting them. That's the part of the conversation that went missing > because the meeting was starting. You just need to change the default > backend when a l10n base is given. There is a complication, though, in that > --with-l10n-base is defined in toolkit/moz.configure making it not always > defined (standalone js doesn't have it), and build backend is in > /moz.configure, and is everywhere. So there would need to be the same kind > of trick done for gonkdir. Neat, thanks for sharing. However, the whole idea of baking l10n base into mozconfig seems wrong to me: it should be repack specific. That is, since we're not going to make "everything a build" (which is most sensible to Android), we should get out of the half-build-half-repack camp and go full repack, meaning that locale-specific things are command arguments. That is, I'd be quite happy if L10NBASEDIR disappeared in the grand packaging improvement push. Do y'all have opinions on that?

https://hg.mozilla.org/mozilla-central/rev/bc439c65ecdc https://hg.mozilla.org/mozilla-central/rev/99f8eb253c87 https://hg.mozilla.org/mozilla-central/rev/69f271e79c56

How would L10NBASEDIR disappear? You always need to tell where your locale files are in some way.

(In reply to Mike Hommey [:glandium] from comment #31) > How would L10NBASEDIR disappear? You always need to tell where your locale > files are in some way. Why does the build need to know? If we double down on "repack after the fact" and move the l10n-processing out of Makefile and into Python, we can stop re-configuring the tree entirely. The way I see it, there are two things going on: repacking omni.ja (or the flat packaging version of omni.ja) and, for some targets, some rebuilding. I don't see why the omni.ja repack needs L10NBASEDIR. I don't think Desktop does any rebuilding, and the l10n rebuilding that Fennec does could be much more easily accomplished in a targeted script.

> I don't see why the omni.ja repack needs L10NBASEDIR. I don't see how you can do a locale repack without knowing the directory where the locales are.

And I don't know about fennec, but I don't envision desktop firefox locale repacks without at least a minimal configure.

(In reply to Mike Hommey [:glandium] from comment #33) > > I don't see why the omni.ja repack needs L10NBASEDIR. > > I don't see how you can do a locale repack without knowing the directory > where the locales are. I simply don't see why we have to configure and bake it into the build configuration. We can process JAR manifests and whatever else outside of "the build system" if we choose to.

(In reply to Nick Alexander :nalexander from comment #35) > (In reply to Mike Hommey [:glandium] from comment #33) > > > I don't see why the omni.ja repack needs L10NBASEDIR. > > > > I don't see how you can do a locale repack without knowing the directory > > where the locales are. > > I simply don't see why we have to configure and bake it into the build > configuration. We can process JAR manifests and whatever else outside of > "the build system" if we choose to. And you process all of them? Even those for b2g when doing desktop firefox? So you have to read moz.build files, and have a minimalistic ConfigEnvironment. At that point, you might as well use configure to get that ConfigEnvironment. And the thing is, what we currently have in moz.configure (modulo skipping old-configure) is probably enough as a configure for l10n repacks. Which makes me think I should try that on top of my l10n-specific build backend attempt. I can't wait to have l10n on try...

I guesss there are a few things in that doc that ask for mods. Is there a follow-up bug for that yet? We also have https://developer.mozilla.org/en-US/docs/Mozilla/Creating_a_language_pack#L10n_binary_repack, which *should* work for fennec as well as desktop. Re configure, mozconfig, l10n-base: I wish the l10n configure was faster, so yay. I think that l10n-base belongs into mozconfig (and it is for my local configs). The thing is that it's used as a code switch in packaging between packs and repacks, and if repacks get a different signal, that'd help a lot. Re l10n-on-try and anything that touches mozharness ... maybe get a bunch of build, releng, l10n in a room and create a plan in London?

(In reply to Axel Hecht [:Pike] from comment #37) > Re l10n-on-try and anything that touches mozharness ... maybe get a bunch of > build, releng, l10n in a room and create a plan in London? nthomas is currently working on l10n-on-try, so hopefully by London we'll be able to experiment with alternative strategies (and maybe I'll break it less). nalexander is planning to fix the current android l10n bustage today.