Closed Bug 575751 Opened 9 years ago Closed 9 years ago

Add package manifest support to Fennec

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(blocking2.0 -, fennec2.0b4+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- -
fennec 2.0b4+ ---

People

(Reporter: mfinkle, Assigned: mwu)

References

(Depends on 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

Depends on: 526333
How can we determine witch packages that should go into the package manifest for mobile?
Look at a current build of fennec. Compare that to the toolkit manifest. What's left should probably go into the mobile manifest. We can fine-tune it a bit too, I think.
Working on this.
Assignee: nobody → mwu
You should be aware of Bug 575894 and Bug 526333
Attached patch Switch to packaging manifest (obsolete) — Splinter Review
This is a little messy. I copied firefox's packaging manifest and adjusted it till it grabbed all the files we need. Should be ok til some of the manifest handling gets cleaned up so we can do this right.

Haven't tested Maemo too much either so that might break, but I'm not sure how I'd get try server to test a "frontend" patch.
Attachment #479643 - Flags: review?(mark.finkle)
For reference, switching to the packaging manifest saves us a little less than 300kb on the fennec APK. It probably improves Ts a little bit too.
blocking2.0: --- → ?
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk
Blocks: 600957
Comment on attachment 479643 [details] [diff] [review]
Switch to packaging manifest

Overall, this looks good. I still worry about toolkit changes breaking mobile because we get out of sync (see bug 526333).

How much has this package manifest bitrotted since you made it?
(In reply to comment #7)
> How much has this package manifest bitrotted since you made it?

Not much, surprisingly. Either that or I'm missing a few things in my builds.
The attached patch fix bug 605411

The only things we have done is remove this line: 
MOZ_PKG_REMOVALS = $(srcdir)/removed-files.in
from mobile/installer/Makefile.in because the file does not exist. Not sure what this file is used for
Blocks: 605411
Comment on attachment 479643 [details] [diff] [review]
Switch to packaging manifest

Let's do this. It's the direction we want to go.
Attachment #479643 - Flags: review?(mark.finkle) → review+
Slightly updated version landed. Lemme know if there's any problems.

http://hg.mozilla.org/mobile-browser/rev/c937d764d3b6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch Add page loader files (obsolete) — Splinter Review
More files
Attachment #484572 - Flags: review?(mark.finkle)
Attachment #484572 - Attachment is obsolete: true
Attachment #484573 - Flags: review?(mark.finkle)
Attachment #484572 - Flags: review?(mark.finkle)
Attached patch Add more files. (obsolete) — Splinter Review
Attachment #484573 - Attachment is obsolete: true
Attachment #484573 - Flags: review?(mark.finkle)
Attachment #484576 - Flags: review?(mark.finkle)
Comment on attachment 484576 [details] [diff] [review]
Add more files.

fix manifest spelling
Attachment #484576 - Flags: review?(mark.finkle) → review+
Ok.

Last night, I was able to:

a) make -f client.mk build
b) make package in objdir
c) make package-tests in objdir
d) move the packages out of objdir/dist (we'd upload them as en-US normally)
e) run compare-locales for de
f) make chrome-de in objdir/mobile/locales
g) clean up mergedir
h) run compare-locales for fr
i) make chrome-fr in objdir/mobile/locales
j) make package in objdir
k) unzip new apk; it had en-US, fr, de locales

Now that this has landed, this is no longer the case.

Would this be the culprit?
http://hg.mozilla.org/mobile-browser/rev/4fe2a42601ff

The patch has been backouted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
blocking2.0: ? → final+
blocking2.0: final+ → -
Blocks: 609430
Copied from firefox. Very useful for l10n testing.
Attachment #491690 - Flags: review?(mark.finkle)
Attachment #479643 - Attachment is obsolete: true
Attachment #484576 - Attachment is obsolete: true
Attachment #491692 - Flags: review?(mark.finkle)
Attachment #491690 - Flags: review?(mark.finkle)
Attachment #491690 - Flags: feedback+
(In reply to comment #21)
> Created attachment 491692 [details] [diff] [review]
> 3. Add MOZ_CHROME_MULTILOCALE

Some explanation: This adds support for doing multilocale repacks. It checks for MOZ_CHROME_MULTILOCALE, which should be set to a list of locales we want to ship. Setting it in the mozconfig should be sufficient, I think. We should be able to set the list from a file so maybe something like export MOZ_CHROME_MULTILOCALE=`cat mobile/locales/maemo-locales` in the mozconfig would work. In the future, we should probably consider having MOZ_CHROME_MULTILOCALE also run the chrome-* commands automatically.
Attachment #491692 - Flags: feedback?(aki)
maemo-locales is a superset of the shipping locales, so that won't work.

There's no way to make the chrome-% target append something smart to a file?
(In reply to comment #23)
> maemo-locales is a superset of the shipping locales, so that won't work.
> 
> There's no way to make the chrome-% target append something smart to a file?

Nah it's fine. As long as it's a superset the missing locales won't affect anything.
Comment on attachment 491691 [details] [diff] [review]
2. Switch to packaging manifest

+[@AB_CD@]
+@BINPATH@/chrome/@AB_CD@@JAREXT@
+@BINPATH@/chrome/@AB_CD@.manifest
+@BINPATH@/@PREF_DIR@/mobile-l10n.js
+@BINPATH@/searchplugins/*
+@BINPATH@/defaults/profile/bookmarks.html
+@BINPATH@/defaults/profile/localstore.rdf
+@BINPATH@/defaults/profile/mimeTypes.rdf
+@BINPATH@/defaults/profile/chrome/*
+@BINPATH@/update.locale
+@BINPATH@/updater.ini
+@BINPATH@/dictionaries/*
+#ifdef XP_WIN32
+#ifndef WINCE
+@BINPATH@/uninstall/helper.exe
+#endif
+#endif

is this a verbatim copy of Firefox? I'm pretty sure the searchplugins are in chrome for fennec, not sure if we need to pay attention to windows uninstall anymore, and neither sure if we're shipping dictionaries?

I also think the defaults stuff looks different in fennec.


Regarding the MOZ_CHROME_MULTILOCALE, I'd at least would want that variable name to indicate that it's a plural. I'm still hoping that we don't need to keep track of passing that list twice.

Could a regular build just overwrite a file with 

[multilocale]

and each chrome-% step added to that, and then the make package target just preprocesses both? I'm still looking for something that doesn't make bug 525327 harder still.

Not doing any in-depth review here, ftr, just mentioning things that pop up.
Btw, if we can use a superset of locales for the package-manifest, we should go for the full monty and just use all-locales. That sounds like a really safe bet to catch any locale that could be there.
Comment on attachment 491691 [details] [diff] [review]
2. Switch to packaging manifest

>+@BINPATH@/dictionaries/*

drop dictionaries from package

I assume this has otherwise been checked against current firefox manifest
Attachment #491691 - Flags: review+
(In reply to comment #25)
> Comment on attachment 491691 [details] [diff] [review]
> 2. Switch to packaging manifest
> 
> +[@AB_CD@]
> +@BINPATH@/chrome/@AB_CD@@JAREXT@
> +@BINPATH@/chrome/@AB_CD@.manifest
> +@BINPATH@/@PREF_DIR@/mobile-l10n.js
> +@BINPATH@/searchplugins/*
> +@BINPATH@/defaults/profile/bookmarks.html
> +@BINPATH@/defaults/profile/localstore.rdf
> +@BINPATH@/defaults/profile/mimeTypes.rdf
> +@BINPATH@/defaults/profile/chrome/*
> +@BINPATH@/update.locale
> +@BINPATH@/updater.ini
> +@BINPATH@/dictionaries/*
> +#ifdef XP_WIN32
> +#ifndef WINCE
> +@BINPATH@/uninstall/helper.exe
> +#endif
> +#endif
> 
> is this a verbatim copy of Firefox? I'm pretty sure the searchplugins are in
> chrome for fennec, not sure if we need to pay attention to windows uninstall
> anymore, and neither sure if we're shipping dictionaries?
> 
> I also think the defaults stuff looks different in fennec.

This is in fact a copy of the firefox package-manifest.in. It's intentional, as minimizing the differences between firefox's package-manifest and ours will let us sync up more easily until the day we land a fix for bug 526333 and factor out manifest entries common between firefox and fennec.
Comment on attachment 491692 [details] [diff] [review]
3. Add MOZ_CHROME_MULTILOCALE

As much as I can follow this part of the makefile, it looks OK to me.
Attachment #491692 - Flags: review?(mark.finkle) → review+
Landed the merge locale patch.

http://hg.mozilla.org/mobile-browser/rev/a2b0ba182a5b
Once bug 613411 is done, feel free to land this ASAP. I may not be around.
That is, patch 2 and patch 3. Patch 1 was already landed.
mwu, can you respond to comment 26 of just using all-locales instead of an explicit list? I'm concerned that we're making this harder for the integration piece than it has to be.

Maybe do a test if the locale is actually generated in bin to keep the startup impact low, assuming that we try to open all manifests on startup.
(In reply to comment #33)
> mwu, can you respond to comment 26 of just using all-locales instead of an
> explicit list? I'm concerned that we're making this harder for the integration
> piece than it has to be.
> 
all-locales should be fine.
The package manifest should always updated now since it changes based on MOZ_CHROME_MULTILOCALE
Attachment #494186 - Flags: review?(mark.finkle)
Attachment #494186 - Flags: review?(mark.finkle) → review+
tracking-fennec: 2.0b3+ → 2.0b4+
Duplicate of this bug: 446035
Aki, see if this patch helps.
Attachment #496415 - Flags: feedback?(aki)
Comment on attachment 496415 [details] [diff] [review]
5. Account for locales in jars

This fixed Maemo multilocale. And the patch looks like it shouldn't regress Android.

I'll clean up my patches and get them reviewed. Thanks mwu!
Attachment #496415 - Flags: feedback?(aki) → feedback+
Comment on attachment 491692 [details] [diff] [review]
3. Add MOZ_CHROME_MULTILOCALE

Worked in my testing.
Attachment #491692 - Flags: feedback?(aki) → feedback+
Attachment #496415 - Flags: review?(mark.finkle)
Attachment #496415 - Flags: review?(mark.finkle) → review+
bug 613411 is live; android should be packaging with MOZ_CHROME_MULTILOCALE at this point.
Patch 2:
http://hg.mozilla.org/mobile-browser/rev/415f748b171b

Combined patch 3+4+5:
http://hg.mozilla.org/mobile-browser/rev/488dd2c9bdbd
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.