Add package manifest support to Fennec

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mfinkle, Assigned: mwu)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Details

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Updated

8 years ago
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.
(Assignee)

Comment 3

8 years ago
Working on this.
Assignee: nobody → mwu
You should be aware of Bug 575894 and Bug 526333
(Assignee)

Comment 5

8 years ago
Created attachment 479643 [details] [diff] [review]
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.
Attachment #479643 - Flags: review?(mark.finkle)
(Assignee)

Comment 6

8 years ago
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: --- → ?
(Assignee)

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Version: 1.9.2 Branch → Trunk

Updated

8 years ago
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?
(Assignee)

Comment 8

8 years ago
(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+
(Assignee)

Comment 11

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

http://hg.mozilla.org/mobile-browser/rev/c937d764d3b6
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

8 years ago
Created attachment 484572 [details] [diff] [review]
Add page loader files

More files
Attachment #484572 - Flags: review?(mark.finkle)
(Assignee)

Comment 13

8 years ago
Created attachment 484573 [details] [diff] [review]
Add page loader files and crashreporter.crt
Attachment #484572 - Attachment is obsolete: true
Attachment #484573 - Flags: review?(mark.finkle)
Attachment #484572 - Flags: review?(mark.finkle)
(Assignee)

Comment 14

8 years ago
Created attachment 484576 [details] [diff] [review]
Add more files.
Attachment #484573 - Attachment is obsolete: true
Attachment #484573 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
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+

Comment 17

8 years ago
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?

Updated

8 years ago
Blocks: 563382
http://hg.mozilla.org/mobile-browser/rev/4fe2a42601ff

The patch has been backouted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
tracking-fennec: --- → ?

Updated

8 years ago
tracking-fennec: ? → 2.0b3+

Updated

8 years ago
blocking2.0: ? → final+

Updated

8 years ago
blocking2.0: final+ → -
(Reporter)

Updated

8 years ago
Blocks: 609430
(Assignee)

Comment 19

8 years ago
Created attachment 491690 [details] [diff] [review]
1. Add merge locale support

Copied from firefox. Very useful for l10n testing.
Attachment #491690 - Flags: review?(mark.finkle)
(Assignee)

Comment 20

8 years ago
Created attachment 491691 [details] [diff] [review]
2. Switch to packaging manifest
Attachment #479643 - Attachment is obsolete: true
Attachment #484576 - Attachment is obsolete: true
(Assignee)

Comment 21

8 years ago
Created attachment 491692 [details] [diff] [review]
3. Add MOZ_CHROME_MULTILOCALE
Attachment #491692 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #491690 - Flags: review?(mark.finkle)

Updated

8 years ago
Attachment #491690 - Flags: feedback+
(Assignee)

Comment 22

8 years ago
(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.
(Assignee)

Updated

8 years ago
Attachment #491692 - Flags: feedback?(aki)

Comment 23

8 years ago
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?
(Assignee)

Comment 24

8 years ago
(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

8 years ago
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

8 years ago
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+
(Assignee)

Comment 28

8 years ago
(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+

Updated

8 years ago
Depends on: 613411
(Assignee)

Comment 30

8 years ago
Landed the merge locale patch.

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

Comment 31

8 years ago
Once bug 613411 is done, feel free to land this ASAP. I may not be around.
(Assignee)

Comment 32

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

Comment 33

8 years ago
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.
(Assignee)

Comment 34

8 years ago
(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.
(Assignee)

Comment 35

8 years ago
Created attachment 494186 [details] [diff] [review]
4. Force package manifest to always be updated

The package manifest should always updated now since it changes based on MOZ_CHROME_MULTILOCALE
Attachment #494186 - Flags: review?(mark.finkle)
(Reporter)

Updated

8 years ago
Attachment #494186 - Flags: review?(mark.finkle) → review+

Updated

8 years ago
tracking-fennec: 2.0b3+ → 2.0b4+
(Reporter)

Updated

8 years ago
Duplicate of this bug: 446035
(Assignee)

Comment 38

8 years ago
Created attachment 496415 [details] [diff] [review]
5. Account for locales in jars

Aki, see if this patch helps.
Attachment #496415 - Flags: feedback?(aki)

Comment 39

8 years ago
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 40

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

Worked in my testing.
Attachment #491692 - Flags: feedback?(aki) → feedback+
(Assignee)

Updated

8 years ago
Attachment #496415 - Flags: review?(mark.finkle)
(Reporter)

Updated

8 years ago
Attachment #496415 - Flags: review?(mark.finkle) → review+

Comment 41

8 years ago
bug 613411 is live; android should be packaging with MOZ_CHROME_MULTILOCALE at this point.
(Assignee)

Comment 42

8 years ago
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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.