Right now we have some wildcarding and manifest generation in [1]. A light moz.build abstraction around this would not go amiss. We may also have other places where extensions are packaged into the omni.ja. [1] https://dxr.mozilla.org/mozilla-central/source/browser/extensions/Makefile.in

I created a new subcontext for extensions. Overkill? Probably, but it makes the declarations look pretty.

Found a pair of bugs, which I've fixed: * Subcontexts didn't emit the necessary directory traversal stuff, so mobile/android/extensions was getting skipped during libs. * $(FINAL_TARGET) isn't defined early enough to use it in the path of the target files, so make was trying to create the manifests in /chrome, which didn't work. (Only for android, because browser/extensions has an explicit final target.)

Comment on attachment 8597965 [details] [diff] [review] Define extension build data in moz.build This looks a bit out of my league with the amount of mozbuild changes - redirecting to gps.

Comment on attachment 8597965 [details] [diff] [review] Define extension build data in moz.build Functionality in this patch touches on packaging foo which glandium has taken an interest in. I'm going to defer this review to glandium, as he is in a much better place to assess its alignment with his grand unified theory for how this should work. glandium: if all you want to do is sign off on the new sub-context in context.py, feel free to re-assign back to me.

Comment on attachment 8597965 [details] [diff] [review] Define extension build data in moz.build Review of attachment 8597965 [details] [diff] [review]: ----------------------------------------------------------------- It bothers me to go all the way with this when shumway and pdfjs are, in fact, not even actually extensions, and could be dealt with like any other part of chrome, with a jar manifest. Albeit, painfully because jar manifests don't take wildcards. So let's just change this.

Comment on attachment 8599722 [details] [diff] [review] Use jar manifests for shumway and pdf.js Review of attachment 8599722 [details] [diff] [review]: ----------------------------------------------------------------- Gerv, we're currently shipping LICENSE files in browser/omni.ja, under chrome/pdfjs/LICENSE and chrome/shumway/LICENSE. They are the following files: https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/LICENSE https://mxr.mozilla.org/mozilla-central/source/browser/extensions/shumway/LICENSE This patch removes those files from omni.ja, but they could easily be added back. The question is whether we do need them there, considering the following: - most files under the corresponding directories in the build (as well as in the source, obviously), carry the license in their header. Files that don't are file formats that usually don't have licenses embedded (images, pdfs, videos), binary files that aren't under the same license and https://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/web/l10n.js. - the license (apache license 2.0) is mentioned in about:license (although there are no specifics about what specific files or directories in the source are under that license) What do you think?

If the license is in about:license, we don't need to ship further copies of it. Gerv

Would be nice to do this for https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/roboextender as well. It's a little tricky (possibly) since the roboextender files are packed in testing/mochitest/roboextender but located in mobile/android/base/roboextender. With your patch can we use topsrcdir relative paths in jar.mn, glandium? Could we add this? Should we?

(In reply to Nick Alexander :nalexander from comment #10) > Would be nice to do this for > https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/ > roboextender as well. It's a little tricky (possibly) since the > roboextender files are packed in testing/mochitest/roboextender but located > in mobile/android/base/roboextender. > > With your patch can we use topsrcdir relative paths in jar.mn, glandium? > Could we add this? Should we? There doesn't seem to be anything in there that can't *already* be done with jar.mn, that is, without the patch in this bug.

err... except the last line in the makefile.

(In reply to Mike Hommey [:glandium] from comment #12) > err... except the last line in the makefile. Right. That needs the (sibling directory) glob.

Comment on attachment 8599721 [details] [diff] [review] Minimalist support for wildcards in jar manifests Review of attachment 8599721 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: python/mozbuild/mozbuild/test/test_jarmaker.py @@ +281,5 @@ > + > + def test_a_wildcard_symlink(self): > + '''Test a wildcard in jar.mn with symlinks''' > + if not symlinks_supported(self.srcdir): > + return Shouldn't this `raise unittest.SkipTest('symlinks not supported')`?

Comment on attachment 8599722 [details] [diff] [review] Use jar manifests for shumway and pdf.js Review of attachment 8599722 [details] [diff] [review]: ----------------------------------------------------------------- The deletion of the Makefile.in files and their cryptic-to-non-build-peers content makes me happy. ::: browser/extensions/moz.build @@ +6,5 @@ > > +DIRS += [ > + 'pdfjs', > + 'shumway', > +] Do we really need the new moz.build files? I can't remember if multiple values for JAR_MANIFESTS actually works... (This is my standard question every time I see new moz.build created. I'd prefer we trend in the other direction.) ::: mobile/android/extensions/moz.build @@ +3,5 @@ > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +JAR_MANIFESTS += ['../../../browser/extensions/shumway/jar.mn'] I guess we don't support "'/' is relative to topsrcdir" for JAR_MANIFESTS :/

(In reply to Gregory Szorc [:gps] from comment #15) > Comment on attachment 8599722 [details] [diff] [review] > Use jar manifests for shumway and pdf.js > > Review of attachment 8599722 [details] [diff] [review]: > ----------------------------------------------------------------- > > The deletion of the Makefile.in files and their cryptic-to-non-build-peers > content makes me happy. > > ::: browser/extensions/moz.build > @@ +6,5 @@ > > > > +DIRS += [ > > + 'pdfjs', > > + 'shumway', > > +] > > Do we really need the new moz.build files? I can't remember if multiple > values for JAR_MANIFESTS actually works... https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/frontend/emitter.py#1135 I didn't want to scope-creep. > ::: mobile/android/extensions/moz.build > @@ +3,5 @@ > > # This Source Code Form is subject to the terms of the Mozilla Public > > # License, v. 2.0. If a copy of the MPL was not distributed with this > > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > > +JAR_MANIFESTS += ['../../../browser/extensions/shumway/jar.mn'] > > I guess we don't support "'/' is relative to topsrcdir" for JAR_MANIFESTS :/ We don't. Same as above, I didn't want to scope-creep.

I'm totally on board with avoiding scope creep. And the scope creep for JAR_MANIFESTS would likely be a deep rabbit hole. Land away.

https://hg.mozilla.org/integration/mozilla-inbound/rev/07c6eccd05c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/22be845b13b6

https://hg.mozilla.org/mozilla-central/rev/07c6eccd05c2 https://hg.mozilla.org/mozilla-central/rev/22be845b13b6 https://hg.mozilla.org/mozilla-central/rev/8ab257db9630

That is a great cleanup, nice work!