575751 - Add package manifest support to Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Use package manifest in build system, like Firefox does:

http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in

http://mxr.mozilla.org/mozilla-central/source/browser/installer/Makefile.in

Comment 1

[Mike Kristoffersen (:MikeK)]

How can we determine witch packages that should go into the package manifest for mobile?

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Michael Wu [:mwu]]

Working on this.

Comment 4

[Mike Kristoffersen (:MikeK)]

You should be aware of Bug 575894 and Bug 526333

Comment 5

[Michael Wu [:mwu]]

Attachment: Switch to packaging manifest

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.

Comment 6

[Michael Wu [:mwu]]

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.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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?

Comment 8

[Michael Wu [:mwu]]

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

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 479643 [details] [diff] [review]
Switch to packaging manifest

Let's do this. It's the direction we want to go.

Comment 11

[Michael Wu [:mwu]]

Slightly updated version landed. Lemme know if there's any problems.

http://hg.mozilla.org/mobile-browser/rev/c937d764d3b6

Comment 12

[Michael Wu [:mwu]]

Attachment: Add page loader files

More files

Comment 13

[Michael Wu [:mwu]]

Attachment: Add page loader files and crashreporter.crt

Comment 14

[Michael Wu [:mwu]]

Attachment: Add more files.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 484576 [details] [diff] [review]
Add more files.

fix manifest spelling

Comment 16

[Michael Wu [:mwu]]

http://hg.mozilla.org/mobile-browser/rev/ffaf2c62e01a

Comment 17

[Aki Sasaki (not active)]

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?

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

http://hg.mozilla.org/mobile-browser/rev/4fe2a42601ff

The patch has been backouted.

Comment 19

[Michael Wu [:mwu]]

Attachment: 1. Add merge locale support

Copied from firefox. Very useful for l10n testing.

Comment 20

[Michael Wu [:mwu]]

Attachment: 2. Switch to packaging manifest

Comment 21

[Michael Wu [:mwu]]

Attachment: 3. Add MOZ_CHROME_MULTILOCALE

Comment 22

[Michael Wu [:mwu]]

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

Comment 23

[Axel Hecht [:Pike]]

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?

Comment 24

[Michael Wu [:mwu]]

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

[Axel Hecht [:Pike]]

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.

Comment 26

[Axel Hecht [:Pike]]

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 27

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 28

[Michael Wu [:mwu]]

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

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 30

[Michael Wu [:mwu]]

Landed the merge locale patch.

http://hg.mozilla.org/mobile-browser/rev/a2b0ba182a5b

Comment 31

[Michael Wu [:mwu]]

Once bug 613411 is done, feel free to land this ASAP. I may not be around.

Comment 32

[Michael Wu [:mwu]]

That is, patch 2 and patch 3. Patch 1 was already landed.

Comment 33

[Axel Hecht [:Pike]]

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.

Comment 34

[Michael Wu [:mwu]]

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

Comment 35

[Michael Wu [:mwu]]

Attachment: 4. Force package manifest to always be updated

The package manifest should always updated now since it changes based on MOZ_CHROME_MULTILOCALE

Comment 37

[Aki Sasaki (not active)]

This seems to be broken for Maemo.
https://bugzilla.mozilla.org/show_bug.cgi?id=613411#c13

Comment 38

[Michael Wu [:mwu]]

Attachment: 5. Account for locales in jars

Aki, see if this patch helps.

Comment 39

[Aki Sasaki (not active)]

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!

Comment 40

[Aki Sasaki (not active)]

Comment on attachment 491692 [details] [diff] [review]
3. Add MOZ_CHROME_MULTILOCALE

Worked in my testing.

Comment 41

[Aki Sasaki (not active)]

bug 613411 is live; android should be packaging with MOZ_CHROME_MULTILOCALE at this point.

Comment 42

[Michael Wu [:mwu]]

Patch 2:
http://hg.mozilla.org/mobile-browser/rev/415f748b171b

Combined patch 3+4+5:
http://hg.mozilla.org/mobile-browser/rev/488dd2c9bdbd