556644 - Omnijar generation support for desktop builds (and also enable it for Firefox)

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(not currently active) Ted Mielczarek]

I'll look into this once the omnijar bug is fixed.

Comment 2

[Johnathan Nightingale [:johnath]]

(In reply to comment #1)
> I'll look into this once the omnijar bug is fixed.

mwu hasn't resolved the bug, but he pushed a couple pieces last week - are we blocked on all of his patches here, or just certain among them?

Comment 3

[(not currently active) Ted Mielczarek]

I don't think there's enough landed in that bug to do anything with yet.

Comment 4

[(dormant account)]

Once this lands I think we can do stat()s to invalidate caches in bug 531886, it should be cheap.

Comment 5

[Johnathan Nightingale [:johnath]]

The android pieces have now landed, but Ted's not sure when he can get to the remaining desktop bits. I'm secretly hoping that "soon" is the answer; I'd suggest someone else if I could think of someone else to pick it up. bsmedberg's out this week, he'd be my obvious alternate, I think?

Comment 6

[(not currently active) Ted Mielczarek]

I suspect bsmedberg has too much other stuff on his plate as well. Once I finish up some Breakpad work I'll be free to work on this. I was planning on working on bug 561842, which is also a startup-related bug, but taras says he'd rather have me work on this. So I can probably get on this in a few days.

Comment 7

[Michael Wu [:mwu]]

BTW, there's one more thing that needs to be done to get omnijar builds parity with normal builds - support for loading window icons from bitmaps instead of files. I'm also not sure how l10n and tests fit into the picture with omnijar - that needs to be investigated.

Beyond that, there are things that are needed to cram absolutely everything into the omnijar/app binary:

1. Compiling application.ini into the binary. (platform.ini already got this treatment)
2. Convert spellcheck dictionary loading to be uri based so it can load things from the omnijar.
3. Read the default blocklist from the omnijar.
4. Statically link libxul, application binary components, and everything else into the app binary.
5. (Firefox specific) Move search plugins into the jar. Mobile already does this.
6. (Firefox specific) Unpack default profile files from the omnijar.
7. (maybe optional) Support autoconfig
8. Deal with plugin-container, possibly by making the main app do the job with the right flags. Not familiar enough with e10s to know if this is possible.
9. Deal with the updater - no idea how to handle this though we can probably avoid a ton of the complexity in the current updater.

If we can fix these issues, we can have a single executable firefox. I might work on 1-3 since mobile needs it.

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to comment #7)
> 4. Statically link libxul, application binary components, and everything else
> into the app binary.

Ow. Do we really need to do this? This makes life harder for us in a number of other ways. Our current plan was instead to just get everything into libxul (bug 561842), which doesn't give us quite as many problems.

Comment 9

[Michael Wu [:mwu]]

(In reply to comment #8)
> (In reply to comment #7)
> > 4. Statically link libxul, application binary components, and everything else
> > into the app binary.
> 
> Ow. Do we really need to do this? This makes life harder for us in a number of
> other ways. Our current plan was instead to just get everything into libxul
> (bug 561842), which doesn't give us quite as many problems.
If we want firefox in a single executable. I don't know if that's where we want to end up - just listed the requirements for getting there.

Comment 10

[(not currently active) Ted Mielczarek]

In the interest of keeping this bug reasonably scoped, I propose that the end result of fixing this bug should be that we produce one single jar file containing all the non-code files for Firefox. bug 561842 will cover reducing the number of binaries we ship down to a firefox executable + libxul. If we want to go beyond that (single static binary or append the omnijar to libxul or something) we can file followup bugs.

Comment 11

[Benjamin Smedberg]

Was there ever a plan for repackaging the localized parts of the omnijar? Unless that work has already been planned out, I think this is way out of scope for Firefox 4.

Comment 12

[Michael Wu [:mwu]]

Attachment: Make omnijar work on desktop (WIP)

Comment 13

[Michael Wu [:mwu]]

Attachment: Make omnijar work on desktop, v2 (WIP)

Now compatible with locale repacks.

Remaining issues (known to me):
1. Only works on unix right now.
2. Locale specific prefs aren't concatenated to the omnipref yet.

Comment 14

[Michael Wu [:mwu]]

Attachment: 1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default

This change allows xpcshell to work with omnijar. It also moves the omnijar setup to after locale setup. Setting the omnijar too early initialized the native charset conversion before the locale was set properly. This made the code think ascii was the native codeset instead of utf8, causing a number of test failures.

Comment 15

[Michael Wu [:mwu]]

Attachment: 2. Make android work with new omnijar setup

We need to set OMNIJAR_PATH to get things setup properly since the default omnijar file name is omni.jar now, not argv[0].

Comment 16

[Michael Wu [:mwu]]

Attachment: 3. Add support for packaging omnijar

Generates a omni.jar when make package is run. Locale repacks on non-windows works.

Comment 17

[Michael Wu [:mwu]]

Attachment: 4. Move extract function from nsJAR to nsZipArchive

This moves the extract function to nsZipArchive and makes the extract function automatically generate any parent directories necessary to extract a file. This is necessary for the next patch..

Comment 18

[Michael Wu [:mwu]]

Attachment: 5. Teach nsToolkitProfileService to initialize the profile from the omnijar

This initializes profiles from defaults/profile/ in the omnijar if we're using omnijar.

Comment 19

[Michael Wu [:mwu]]

Attachment: 6. Let the browser reset bookmarks from the omnijar

Comment 20

[Michael Wu [:mwu]]

Two more patches to fix universal builds on OSX and tests on all platforms to come after they prove themselves on try.

Comment 21

[Michael Wu [:mwu]]

Attachment: 2. Make android work with new omnijar setup, v2

Move OMNIJAR_PATH setting to java wrapper.

Comment 22

[Michael Wu [:mwu]]

Attachment: 7. Fix tests

Fix all tests but Talos and tests on OSX.

Comment 23

[(dormant account)]

Comment on attachment 458860 [details] [diff] [review]
4. Move extract function from nsJAR to nsZipArchive


>-  NS_ENSURE_TRUE(item, NS_ERROR_FILE_TARGET_DOES_NOT_EXIST);

Use explicit returns instead of hiding them in macros.
>-    if (NS_FAILED(rv)) return rv;

Keep return rv on separate line.

r+ with those fixed.

The single-directory backoff is cute, but wont work for files nested 2 directories deep without corresponding directories in the zip index. Don't have to fix this since it's inherited from old code.

Comment 24

[Michael Wu [:mwu]]

Attachment: 3. Add support for packaging omnijar, v2

Works pretty well. Has two remaining issues which I'll fix in followup bugs/patches -

1. Pref combining breaks locale repackaging stuff.
2. For some reason the empty chrome directory doesn't get preserved on windows so talos fails.

Comment 25

[Benjamin Smedberg]

Please ignore talos, I'm fixing that in bug 580698.

Comment 26

[Michael Wu [:mwu]]

(In reply to comment #25)
> Please ignore talos, I'm fixing that in bug 580698.

Nice! I can drop the empty chrome directory workaround then.

Comment 27

[Michael Wu [:mwu]]

Attachment: 4. Move extract function from nsJAR to nsZipArchive, v2

Address review comments.

Comment 28

[Michael Wu [:mwu]]

Attachment: 8. Bring universal binaries into the world of omnijar

This makes flight.mk and fix-buildconfig work on flat/omnijar builds.

Comment 29

[Benjamin Smedberg]

Comment on attachment 458855 [details] [diff] [review]
1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default

I really don't like the OMNIJAR_PATH environment variable: if you need something for child processes, we should pass the omnijar path in the IPC launching code (as a commandline flag), or just figure it out automatically. Definitely not in the environment.

Everything else look ok, though. Do you want to actually check that omni.jar exists?

Comment 30

[Benjamin Smedberg]

Comment on attachment 459236 [details] [diff] [review]
7. Fix tests

For test_bug292789.html, please try to use cr.convertChromeURI, instead of forking the file.

In test_import, just remove the two lines you commented out.

runtests.py.in doesn't look like it will work on Windows, which doesn't have a command named 'cp'. What is it you're actually copying there?

And I think the installChromeFile stuff will be obsoleted by bug 579178.

Comment 31

[Michael Wu [:mwu]]

(In reply to comment #30)
> Comment on attachment 459236 [details] [diff] [review]
> 7. Fix tests
> 
> For test_bug292789.html, please try to use cr.convertChromeURI, instead of
> forking the file.
> 
Can't seem to access Components.classes. Any alternative?

> In test_import, just remove the two lines you commented out.
> 
Done.

> runtests.py.in doesn't look like it will work on Windows, which doesn't have a
> command named 'cp'. What is it you're actually copying there?
> 
We're copying the contents of bin/ to firefox/. This helps xpcshell find the omnijar and also copies components over. 'cp' seems to work fine on try, actually. Mochitests wouldn't run otherwise. Another option is to specify the omnijar on the command line.

Comment 32

[(not currently active) Ted Mielczarek]

That cp is gross. xpcshell supports a -g option to pass the GRE dir (and we use it). Can we just make that work instead?

Comment 33

[Michael Wu [:mwu]]

Attachment: 1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default, v2

Added -omnijar. Omnijar code changed to setup nsZipArchive lazily to avoid initializing the native charset code too early.

Comment 34

[Benjamin Smedberg]

Comment on attachment 459882 [details] [diff] [review]
1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default, v2

Assuming you want omnijar to work on Windows, GetNativePath is not a good choice. It is possible to install Firefox to a unicode directory that cannot be represented by a native path. It looks like you may have to either use platform ifdefs or make childArgv a CommandLine instead of a vector of std::string.

You need to remove the ifdef around mozilla::SetOmijar(nsnull): that ifdef should only be used for malloced data, not refcounted objects.

Comment 35

[Michael Wu [:mwu]]

Attachment: 1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default, v3

Avoid GetNativePath for windows, look for omnijar in gre directory instead of app directory.

Comment 36

[Michael Wu [:mwu]]

Attachment: 7. Fix tests, v2

Avoid cp and remove those two lines in test_import. Also, preprocess test_bug292789.html to avoid an omnijar fork of the file.

Comment 37

[d]

Do you have any data on how much this would improve start-up time for desktop builds?

Comment 38

[Michael Wu [:mwu]]

(In reply to comment #37)
> Do you have any data on how much this would improve start-up time for desktop
> builds?

I expect about 10% on cold start. From my runs on try server, I usually see a bit less than 10% improvement on linux and 10-20% on windows and osx. On first start, the improvement is greater (and noticeable) due to faster IO while loading js components.

There are additional optimizations available that omnijar makes possible. It also makes it possible to fix bug 531886 which I expect to cause significant extension developer frustration if not fixed.

Comment 39

[Benjamin Smedberg]

At this point I don't think this blocks, although we'll take this in a beta soon if the pieces come together. I probably wouldn't take this past beta4 (ships 20-Aug).

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I think that this must block, actually -- Fx4 is supposed to be a release focused around major improvements in performance, of which startup time is one component.  We know this has a significant impact on that, and so we should prioritize getting it in.

Comment 41

[Shawn Wilsher :sdwilsh]

Comment on attachment 458864 [details] [diff] [review]
6. Let the browser reset bookmarks from the omnijar

For more detailed review comments with context, please see http://reviews.visophyte.org/r/458864/.

on file: browser/components/nsBrowserGlue.js line 823
>         bookmarksURI = Services.io.newURI("resource:///defaults/profile/bookmarks.html", null, null);

We should just import (locally in this method for now) NetUtil.jsm, and then
use NetUtil.newURI.  You don't have to pass the two null params.


on file: browser/components/nsBrowserGlue.js line 825
>       else
>       {

nit: brace goes after the else


on file: browser/components/nsBrowserGlue.js line 829
>           bookmarksURI = Services.io.newFileURI(bookmarksFile);

...and then just use NetUtil.newURI(bookmarksFile) here.


on file: toolkit/components/places/public/nsIPlacesImportExportService.idl line 71
>   void importHTMLFromUri(in nsIURI aURI, in boolean aIsInitialImport);

nit: URI


on file: toolkit/components/places/src/nsPlacesImportExportService.h line 53
>     nsresult ImportHTMLFromUriInternal(nsIURI* aURI, PRBool aAllowRootChanges,

nit: URI


on file: toolkit/components/places/src/nsPlacesImportExportService.cpp line 2185
> nsPlacesImportExportService::ImportHTMLFromUri(nsIURI* aURI,
>                                                 PRBool aIsInitialImport)

nit: spacing is off here


r=sdwilsh

This is an API change, so it's going to need sr too.

Comment 42

[Benjamin Smedberg]

Comment on attachment 459579 [details] [diff] [review]
8. Bring universal binaries into the world of omnijar

I'm sure I don't understand how this works. By the time we get around to flight.mk, haven't we already created the omnijar? Why are we modifying the flat version in chrome/toolkit? I really don't understand how flight.mk would get MOZ_CHROME_FILE_FORMAT_JAR set.

I wish I weren't the reviewer for this patch: I didn't write the code in question, nor do I understand how it works.

Comment 43

[(not currently active) Ted Mielczarek]

Comment on attachment 459579 [details] [diff] [review]
8. Bring universal binaries into the world of omnijar

>diff --git a/build/macosx/universal/flight.mk b/build/macosx/universal/flight.mk
>--- a/build/macosx/universal/flight.mk
>+++ b/build/macosx/universal/flight.mk
>@@ -67,7 +67,7 @@ ifeq ($(MOZ_BUILD_APP),camino) # {
>+ifdef MOZ_CHROME_FILE_FORMAT_JAR
>+BUILDCONFIG = $(BUILDCONFIG_BASE)/toolkit.jar
>+FIX_MODE = jar
>+else
>+BUILDCONFIG = $(BUILDCONFIG_BASE)/toolkit/
>+FIX_MODE = file
>+endif

I'm pretty sure this won't work, because flight.mk is invoked from client.mk, which doesn't know much about the build (it doesn't know any of the stuff from autoconf.mk, for example).

Comment 44

[(not currently active) Ted Mielczarek]

Comment on attachment 459579 [details] [diff] [review]
8. Bring universal binaries into the world of omnijar

Didn't really mean to clear that review flag, but let's just do this.

Comment 45

[Benjamin Smedberg]

Comment on attachment 459965 [details] [diff] [review]
7. Fix tests, v2

runtests.py.in should just use

if not os.path.isdir(chromeDir):
    os.mkdir(chromeDir)

I don't quite understand the nsComponentManager.cpp change. Under what condition would we have a MOZ_OMNIJAR build with no mozilla::OmnijarPath? Isn't that an error condition that should fail early, instead of getting all the way here?

Or are you trying to support MOZ_OMNIJAR without actually building the omnijar?

Comment 46

[Michael Wu [:mwu]]

(In reply to comment #45)
> Comment on attachment 459965 [details] [diff] [review]
> 7. Fix tests, v2
> 
> runtests.py.in should just use
> 
> if not os.path.isdir(chromeDir):
>     os.mkdir(chromeDir)
> 
Ok.

> I don't quite understand the nsComponentManager.cpp change. Under what
> condition would we have a MOZ_OMNIJAR build with no mozilla::OmnijarPath? Isn't
> that an error condition that should fail early, instead of getting all the way
> here?
> 
> Or are you trying to support MOZ_OMNIJAR without actually building the omnijar?
Yeah, this is for running xpcshell tests without packaging.

Comment 47

[Michael Wu [:mwu]]

Attachment: 7. Fix tests, v3

Address review comment about runtests.py.in .

Comment 48

[Michael Wu [:mwu]]

Comment on attachment 458862 [details] [diff] [review]
5. Teach nsToolkitProfileService to initialize the profile from the omnijar

Hate to pile another review on, but dietrich doesn't seem to be around.

Comment 49

[Michael Wu [:mwu]]

Attachment: 1. Move omnijar setup to NS_InitXPCOM and use omni.jar by default, v4

Added a few #ifdef MOZ_OMNIJAR to make this build on non-omnijar.

Comment 50

[(not currently active) Ted Mielczarek]

Comment on attachment 459566 [details] [diff] [review]
3. Add support for packaging omnijar, v2

># HG changeset patch
># Parent c4c18d1a7840ce97f9a3216d9f3088463f096470
>
>diff --git a/browser/installer/package-manifest.in b/browser/installer/package-manifest.in
>--- a/browser/installer/package-manifest.in
>+++ b/browser/installer/package-manifest.in
>@@ -10,6 +10,12 @@
> 
> #filter substitution
> 
>+#ifdef MOZ_CHROME_FILE_FORMAT_JAR
>+#define JAREXT .jar
>+#else
>+#define JAREXT  
>+#endif

Can you do this in the Makefile instead? I'd rather keep all the DEFINES out in the Makefile in the same place.

>diff --git a/toolkit/locales/l10n.mk b/toolkit/locales/l10n.mk
>--- a/toolkit/locales/l10n.mk
>+++ b/toolkit/locales/l10n.mk
>@@ -95,8 +95,6 @@ clobber-%:
> 
> 
> PACKAGER_NO_LIBS = 1
>-include $(topsrcdir)/toolkit/mozapps/installer/packager.mk
>-
> 
> ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> STAGEDIST = $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_APPNAME)/$(_APPNAME)/Contents/MacOS
>@@ -104,6 +102,8 @@ else
> STAGEDIST = $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_DIR)
> endif
> 
>+include $(topsrcdir)/toolkit/mozapps/installer/packager.mk

What's this change for?

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -107,32 +107,32 @@ UNPACK_TAR       = tar -xf-
> 
> ifeq ($(MOZ_PKG_FORMAT),TAR)
> PKG_SUFFIX	= .tar
>-MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) > $(PACKAGE)
>-UNMAKE_PACKAGE	= $(UNPACK_TAR) < $(UNPACKAGE)
>+_MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) > $(PACKAGE)
>+_UNMAKE_PACKAGE	= $(UNPACK_TAR) < $(UNPACKAGE)
> MAKE_SDK = $(CREATE_FINAL_TAR) - $(MOZ_APP_NAME)-sdk > $(SDK)
> endif
> ifeq ($(MOZ_PKG_FORMAT),TGZ)
> PKG_SUFFIX	= .tar.gz
>-MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) | gzip -vf9 > $(PACKAGE)
>-UNMAKE_PACKAGE	= gunzip -c $(UNPACKAGE) | $(UNPACK_TAR)
>+_MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) | gzip -vf9 > $(PACKAGE)
>+_UNMAKE_PACKAGE	= gunzip -c $(UNPACKAGE) | $(UNPACK_TAR)
> MAKE_SDK = $(CREATE_FINAL_TAR) - $(MOZ_APP_NAME)-sdk | gzip -vf9 > $(SDK)
> endif
> ifeq ($(MOZ_PKG_FORMAT),BZ2)
> PKG_SUFFIX	= .tar.bz2
>-MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) | bzip2 -vf > $(PACKAGE)
>-UNMAKE_PACKAGE	= bunzip2 -c $(UNPACKAGE) | $(UNPACK_TAR)
>+_MAKE_PACKAGE 	= $(CREATE_FINAL_TAR) - $(MOZ_PKG_DIR) | bzip2 -vf > $(PACKAGE)
>+_UNMAKE_PACKAGE	= bunzip2 -c $(UNPACKAGE) | $(UNPACK_TAR)
> MAKE_SDK = $(CREATE_FINAL_TAR) - $(MOZ_APP_NAME)-sdk | bzip2 -vf > $(SDK)
> endif
> ifeq ($(MOZ_PKG_FORMAT),ZIP)
> PKG_SUFFIX	= .zip
>-MAKE_PACKAGE	= $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR)
>-UNMAKE_PACKAGE	= $(UNZIP) $(UNPACKAGE)
>+_MAKE_PACKAGE	= $(ZIP) -r9D $(PACKAGE) $(MOZ_PKG_DIR)
>+_UNMAKE_PACKAGE	= $(UNZIP) $(UNPACKAGE)
> MAKE_SDK = $(ZIP) -r9D $(SDK) $(MOZ_APP_NAME)-sdk
> endif
> ifeq ($(MOZ_PKG_FORMAT),CAB)
> PKG_SUFFIX	= .cab
>-MAKE_PACKAGE	= $(MAKE_CAB)
>-UNMAKE_PACKAGE	= $(error Unpacking CAB files is not supported)
>+_MAKE_PACKAGE	= $(MAKE_CAB)
>+_UNMAKE_PACKAGE	= $(error Unpacking CAB files is not supported)
> endif
> ifeq ($(MOZ_PKG_FORMAT),DMG)
> ifndef _APPNAME
>@@ -169,11 +169,11 @@ endif
> ifndef PKG_DMG_SOURCE
> PKG_DMG_SOURCE = $(STAGEPATH)$(MOZ_PKG_DIR)
> endif
>-MAKE_PACKAGE	= $(_ABS_MOZSRCDIR)/build/package/mac_osx/pkg-dmg \
>+_MAKE_PACKAGE	= $(_ABS_MOZSRCDIR)/build/package/mac_osx/pkg-dmg \
>   --source "$(PKG_DMG_SOURCE)" --target "$(PACKAGE)" \
>   --volname "$(MOZ_APP_DISPLAYNAME)" $(PKG_DMG_FLAGS)
> _ABS_DIST = $(call core_abspath,$(DIST))
>-UNMAKE_PACKAGE	= \
>+_UNMAKE_PACKAGE	= \
>   set -ex; \
>   rm -rf $(_ABS_DIST)/unpack.tmp; \
>   mkdir -p $(_ABS_DIST)/unpack.tmp; \
>@@ -203,6 +203,36 @@ endif
> MAKE_SDK = $(CREATE_FINAL_TAR) - $(MOZ_APP_NAME)-sdk | bzip2 -vf > $(SDK)
> endif
> 
>+ifdef MOZ_OMNIJAR
>+OMNIJAR_FILES	= \
>+  chrome \
>+  components/*.js \
>+  components/*.xpt \
>+  components/omnijar.manifest \
>+  modules \
>+  res \
>+  defaults \
>+  greprefs.js \
>+  $(NULL)
>+
>+NON_OMNIJAR_FILES = \
>+  chrome/icons/\* \
>+  res/cursors/\* \
>+  res/MainMenu.nib/\* \
>+  $(NULL)

It feels a little icky to have this list here. I get that it makes your MAKE_PACKAGE bit easier, but it doesn't feel very good.
>+ifdef MOZ_OMNIJAR
>+	@echo "Preparing files for omnijar"
>+# create manifest for components containing only omnijarable files
>+# also creates a new manifest for remaining binary components
>+	@(for MANIFEST in $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/*.manifest ; do grep -v '^binary-component' "$$MANIFEST" >> omnijar.manifest ; grep '^binary-component' "$$MANIFEST" >> binary.manifest ; echo >> omnijar.manifest ; echo >> binary.manifest ; rm "$$MANIFEST" ; done )
>+	mv omnijar.manifest $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/
>+	mv binary.manifest $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/
>+# combine prefs into the omnipref
>+	@(for PREF in $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/defaults/pref/*.js ; do cat "$$PREF" >> prefs.js ; echo >> prefs.js ; rm "$$PREF" ; done )
>+	mv prefs.js $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/defaults/
>+endif # MOZ_OMNIJAR

Seriously, can you make this a little Python script? This is super hard to read, and I don't want to have to maintain this much shell goop. If you did that, you could have the Python script generate omni.jar instead of hiding it in MAKE_PACKAGE, right? (Which has the nice side benefit that you can `make stage-package` and get an omnijar build in dist/firefox.)

Although I guess you need the MAKE_PACKAGE / UNMAKE_PACKAGE to work for the l10n repackaging bit, eh? Hrm.

Comment 51

[Michael Wu [:mwu]]

Attachment: 2. Make android work with new omnijar setup, v3

Use -omnijar instead of OMNIJAR_PATH.

Comment 52

[Benjamin Smedberg]

Comment on attachment 458862 [details] [diff] [review]
5. Teach nsToolkitProfileService to initialize the profile from the omnijar

This might be ok (although I hate it, and wish that we didn't have defaults/profile at all!). But I need a couple things in order to review:

1) move this extracting bit into a static function on its own: ::CreateProfile is already long and indented enough.
2) give the patch lots more context: -u12 or more.
3) Is there no better way to do the string-fu? RFindChar, substring-comparisons, Last() all come to mind so we can avoid doing low-level char* magic.

Comment 53

[Michael Wu [:mwu]]

(In reply to comment #50)
> Can you do this in the Makefile instead? I'd rather keep all the DEFINES out in
> the Makefile in the same place.
> 
Ok.

> >diff --git a/toolkit/locales/l10n.mk b/toolkit/locales/l10n.mk
> >--- a/toolkit/locales/l10n.mk
> >+++ b/toolkit/locales/l10n.mk
> >@@ -95,8 +95,6 @@ clobber-%:
> > 
> > 
> > PACKAGER_NO_LIBS = 1
> >-include $(topsrcdir)/toolkit/mozapps/installer/packager.mk
> >-
> > 
> > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> > STAGEDIST = $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_APPNAME)/$(_APPNAME)/Contents/MacOS
> >@@ -104,6 +102,8 @@ else
> > STAGEDIST = $(_ABS_DIST)/l10n-stage/$(MOZ_PKG_DIR)
> > endif
> > 
> >+include $(topsrcdir)/toolkit/mozapps/installer/packager.mk
> 
> What's this change for?
> 
Hmm.. I thought I had a reason for this.. Guess not. I'll move it back.

> >+ifdef MOZ_OMNIJAR
> >+OMNIJAR_FILES	= \
> >+  chrome \
> >+  components/*.js \
> >+  components/*.xpt \
> >+  components/omnijar.manifest \
> >+  modules \
> >+  res \
> >+  defaults \
> >+  greprefs.js \
> >+  $(NULL)
> >+
> >+NON_OMNIJAR_FILES = \
> >+  chrome/icons/\* \
> >+  res/cursors/\* \
> >+  res/MainMenu.nib/\* \
> >+  $(NULL)
> 
> It feels a little icky to have this list here. I get that it makes your
> MAKE_PACKAGE bit easier, but it doesn't feel very good.
I'm not sure where else to put this..

> >+ifdef MOZ_OMNIJAR
> >+	@echo "Preparing files for omnijar"
> >+# create manifest for components containing only omnijarable files
> >+# also creates a new manifest for remaining binary components
> >+	@(for MANIFEST in $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/*.manifest ; do grep -v '^binary-component' "$$MANIFEST" >> omnijar.manifest ; grep '^binary-component' "$$MANIFEST" >> binary.manifest ; echo >> omnijar.manifest ; echo >> binary.manifest ; rm "$$MANIFEST" ; done )
> >+	mv omnijar.manifest $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/
> >+	mv binary.manifest $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/components/
> >+# combine prefs into the omnipref
> >+	@(for PREF in $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/defaults/pref/*.js ; do cat "$$PREF" >> prefs.js ; echo >> prefs.js ; rm "$$PREF" ; done )
> >+	mv prefs.js $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/defaults/
> >+endif # MOZ_OMNIJAR
> 
> Seriously, can you make this a little Python script? This is super hard to
> read, and I don't want to have to maintain this much shell goop. If you did
> that, you could have the Python script generate omni.jar instead of hiding it
> in MAKE_PACKAGE, right? (Which has the nice side benefit that you can `make
> stage-package` and get an omnijar build in dist/firefox.)
> 
> Although I guess you need the MAKE_PACKAGE / UNMAKE_PACKAGE to work for the
> l10n repackaging bit, eh? Hrm.
Yeah I can put this part in a python script. I usually prefer shell scripting since it's more straightforward than python for simple things like this.

Comment 54

[Michael Wu [:mwu]]

Attachment: 8. Bring universal binaries into the world of omnijar, v2

This one also builds on non-omnijar.

Comment 55

[Michael Wu [:mwu]]

Comment on attachment 458862 [details] [diff] [review]
5. Teach nsToolkitProfileService to initialize the profile from the omnijar

Tests seem to pass fine without this patch. Goodbye defaults/profile.

Comment 56

[Michael Wu [:mwu]]

Attachment: 3. Add support for packaging omnijar, v3

I took out the messy parts that are gonna end up changing after bug 579178.

Comment 57

[Benjamin Smedberg]

Yeah, bug 579178 should land first: I'll mark approvals for this afterwards.

Comment 58

[Michael Wu [:mwu]]

Attachment: 9. Make windows installer locale repacks use the standard repacking path

This is the last piece.

 browser/installer/Makefile.in                     |    5 --
 browser/installer/windows/nsis/installer.nsi      |   26 ++++--------
 browser/locales/Makefile.in                       |   26 +-----------
 testing/release/common/unpack.sh                  |    7 +--
 toolkit/mozapps/installer/packager.mk             |   46 +++++++++-------------
 toolkit/mozapps/installer/windows/nsis/common.nsh |    3 -
 6 files changed, 39 insertions(+), 74 deletions(-)

It eliminates the nonlocalized/localized distinction in the win32 installer and makes the l10n repacking for the win32 installer use the same code that osx(dmg)/linux(tgz)/windows(zip) use.

Comment 59

[Michael Wu [:mwu]]

Looks like I accidentally eliminated the use of MOZ_(LOCALIZED|NONLOCALIZED|OPTIONAL)_PKG_LIST. Can be put back or eliminated completely.

Comment 60

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 463765 [details] [diff] [review]
9. Make windows installer locale repacks use the standard repacking path

If I read this patch correctly, if I take a completely clean tree and make repackage-win32-installer it will fail because you've removed the branding export which is necessary to build setup.exe.  r-ing for this, though everything else looks pretty good.

Can we ditch the core subdirectory?  The optional components can live in the optional/ subdirectory, but it'd be nice if for the common case no subdirectory was needed at all.

Since the browser doesn't use optional components http://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/nsis/installer.nsi#238 can change to just "Installing Files".  Other than that I didn't look at the NSIS bits too closely.

When you upload a new version please use a diff with 8 lines of context.

Comment 61

[Benjamin Smedberg]

It has come to my attention that we're talking about repacking locales in FF4 again. I really don't think we should be taking patches at this point which muck with repacking locales. It's extremely fragile, impossible to test on tryserver, and regressions aren't going to show up until very late in the cycle. We should either leave locales out of the omnijar, or punt on omnijar.

Comment 62

[Michael Wu [:mwu]]

(In reply to comment #61)
> It has come to my attention that we're talking about repacking locales in FF4
> again. I really don't think we should be taking patches at this point which
> muck with repacking locales. It's extremely fragile, impossible to test on
> tryserver, and regressions aren't going to show up until very late in the
> cycle. We should either leave locales out of the omnijar, or punt on omnijar.

Leaving locales out of the omnijar is a pain because it's not what it's designed to do. These patches are designed to be compatible as possible by mimicking flat packaged locale repacks. It's compatible with our current locale repacking code to the point that nearly all but the last patch do not actually touch any locale related code. To special case locale repacks would actually require more mucking with locale repacking code.

These patches also do not turn omnijar on by default. We can land these patches, have releng see how well omnijar builds/repacks/etc. work, and have it on or off depending on this data. No patches need to be backed out to turn off omnijar.

Comment 63

[Benjamin Smedberg]

a=me to land reviewed patches here which do not change the default behavior

Comment 64

[Benjamin Smedberg]

Per today's meeting, we're going to push to get this in beta4, which codefreezes on Monday. Ted, can you do or delegate the remaining two reviews?

Comment 65

[Axel Hecht [:Pike]]

FWIW, here's a doc that describes pretty exactly how we do repacks, https://developer.mozilla.org/en/Creating_a_Language_Pack.

Other than that, I'd suggest to get someone from releng to give you the exact list of commands we do for depends, nightlies, and release builds.

Testing should work fine with, say, french, and l10n-merge on, which is an option to https://developer.mozilla.org/En/Compare-locales, a python tool we use in our build system.

Comment 66

[Michael Wu [:mwu]]

Attachment: 10. Enumerate prefs in the omnijar instead of hardcoding

It would be better not to enumerate, but trying to combine all the prefs causes more trouble than it's worth right now.

Comment 67

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/f80832cefeb5
http://hg.mozilla.org/mozilla-central/rev/f2bffbc6fee7
http://hg.mozilla.org/mozilla-central/rev/b57d52fc217e
http://hg.mozilla.org/mozilla-central/rev/d0d46d0fc5d9

Comment 68

[Michael Wu [:mwu]]

Not actually fixed, but a bunch of patches did land. I think there's about 4 more patches to go.

Comment 69

[Michael Wu [:mwu]]

Attachment: 9. Make windows installer locale repacks use the standard repacking path, v2

I dropped the MOZ_CORE_PKG_LIST variable change since the localized/nonlocalized separation is necessary after the manifest enumeration changes. Also made an attempt to support optional but it'll need more work if someone wants to use it.

"Installing main files" wasn't changed.. not too comfortable with changing strings here..

Comment 70

[Robert Strong (they/them - no direct email)]

Have you tested the Windows installer, l10n repackaging for the Windows installer, and / or update repackaging? Are all of the patches to get this working locally attached to this bug? Will the only option be omnijar?

SeaMonkey uses optional components and I see that callek and kairo are both cc'd.

Comment 71

[Michael Wu [:mwu]]

(In reply to comment #70)
> Have you tested the Windows installer, l10n repackaging for the Windows
> installer, and / or update repackaging?
Windows installer and l10n repackaging are both tested. Update repackaging not tested - need to check that.

> Are all of the patches to get this
> working locally attached to this bug?
I think so, though it's a pain. I have a patch queue available at http://hg.mozilla.org/users/mwu_mozilla.com/patchpile/ and can also provide rolled up patches.

> Will the only option be omnijar?
> 
Nope - this patch makes the windows installer code use more of the same code that's used by other platforms for packaging. We'll be able to test these changes independently of omnijar.

> SeaMonkey uses optional components and I see that callek and kairo are both
> cc'd.
Ah ok. I'll test seamonkey then.

Comment 72

[Axel Hecht [:Pike]]

I recall that that I tried to factor as much as I could when I revamped the l10n repacks not too long ago. Doing the windows installer differently broke something rather subtle. Not sure if it was mar files, or zips? Something like that.

Comment 73

[Michael Wu [:mwu]]

Comment on attachment 464724 [details] [diff] [review]
9. Make windows installer locale repacks use the standard repacking path, v2

Er, attached the wrong patch.

Comment 74

[Michael Wu [:mwu]]

Attachment: 11. Don't do slash fixup for omnijar

Fallout from bug 579178. Omnijar code doesn't need the / -> \ fix since zips always use forward slash. Also adds a check and logging so missing manifest problems are easier to debug.

Comment 75

[Michael Wu [:mwu]]

Attachment: 3. Add support for packaging omnijar, v4

Updated for the changes from bug 579178.

Comment 76

[Michael Wu [:mwu]]

Attachment: 12. Add 60+ files to the removed-files.in

We get to remove a little over half the files we'd normally install.

Comment 77

[Michael Wu [:mwu]]

Attachment: 12. Add 60+ files to the removed-files.in, v2

I'm not very good at attaching patches.

Comment 78

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/11a41ea3cb79 Slash fixup fix. (patch 11)
http://hg.mozilla.org/mozilla-central/rev/a186d256fbef Pref enum. (patch 10)

Comment 79

[Michael Wu [:mwu]]

Attachment: 9. Make windows installer locale repacks use the standard repacking path, v3

Comment 80

[Michael Wu [:mwu]]

Attachment: 13. Fix leak in child process

This ensures the child process always frees the omnijar data. Tried doing it in XRE_DeinitCommandLine but the leak keeps getting reported, possibly due to the NS_LogTerm.

Comment 81

[Michael Wu [:mwu]]

Attachment: 9. Make windows installer locale repacks use the standard repacking path, v4

Added ifdef MOZ_OMNIJAR to a line that needed it. Managed to build seamonkey okay with a port of the firefox packaging changes, though further fixes will be needed to get its optional package installation working properly again.

Comment 82

[Robert Strong (they/them - no direct email)]

Couple of quick comments from testing. When repackaging l10n the installer is placed in obj-dir/dist instead of obj-dir/dist/install/sea and the installer name has changed as well (French example)
From:
firefox-4.0b4pre.fr.win32.installer.exe

To:
firefox-4.0b4pre.fr.win32.exe

Comment 83

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #78)
> http://hg.mozilla.org/mozilla-central/rev/11a41ea3cb79 Slash fixup fix. (patch
> 11)
> http://hg.mozilla.org/mozilla-central/rev/a186d256fbef Pref enum. (patch 10)

These were caught up in a mass backout.

Comment 84

[Robert Strong (they/them - no direct email)]

Kev, heads up that this bug will move the distribution directory in the installer self extractive archive from the nonlocalized directory to a directory named core.

Comment 85

[Robert Strong (they/them - no direct email)]

Kev ^^

Comment 86

[Michael Wu [:mwu]]

(In reply to comment #82)
> Couple of quick comments from testing. When repackaging l10n the installer is
> placed in obj-dir/dist instead of obj-dir/dist/install/sea and the installer
> name has changed as well (French example)
> From:
> firefox-4.0b4pre.fr.win32.installer.exe
> 
> To:
> firefox-4.0b4pre.fr.win32.exe
Hm, odd. Looks like package-name.mk isn't being included properly, though I can't tell how it wouldn't be included.

Comment 87

[Robert Strong (they/them - no direct email)]

Relevent output

make[2]: Leaving directory `/d/moz/_omnijar_test/ff-opt/browser/installer/windows'
make repackage-zip AB_CD=fr MOZ_PKG_FORMAT=SFX7Z ZIP_IN=/d/moz/_omnijar_test/ff-opt/dist/install/sea/firefox-4.0b4pre.en-US.win32.installer.exe ZIP_OUT="/d/moz/_omnijar_test/ff-opt/dist/install/sea/firefox-4.0b4pre.fr.win32.installer.exe" SFX_HEADER="/d/moz/_omnijar_test/ff-opt/browser/locales/../installer/windows/l10ngen/7zSD.sfx /d/moz/_omnijar_test/mozilla/browser/installer/windows/app.tag"
make[2]: Entering directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
rm -f -r /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/uninstall; d:/moz/_omnijar_test/ff-opt/config/nsinstall.exe -D /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/uninstall; cp ../installer/windows/l10ngen/helper.exe /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/uninstall; rm -f /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/setup.exe; cp ../installer/windows/l10ngen/setup.exe /d/moz/_omnijar_test/ff-opt/dist/l10n-stage; 
cd ../../dist/xpi-stage/locale-fr && \
	  tar --exclude=install.rdf --exclude=chrome.manifest -cvhf - * | ( cd /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox && tar -xf - )
mv /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/chrome/fr.manifest /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/chrome/localized.manifest
d:/moz/_omnijar_test/ff-opt/config/nsinstall.exe -D ../../dist/l10n-stage/
cd ../../dist/l10n-stage; \
	  (cd firefox && rm -f omni.jar components/binary.manifest && grep -h '^binary-component' components/*.manifest > binary.manifest ; zip -r9m omni.jar chrome chrome.manifest components/*.js components/*.xpt components/*.manifest modules res defaults greprefs.js  -x chrome/icons/\* res/cursors/\* res/MainMenu.nib/\*  && mv binary.manifest components && echo "manifest components/binary.manifest" > chrome.manifest) && rm -f app.7z && mv firefox core &&  7z a -r -t7z app.7z -mx -m0=BCJ2 -m1=LZMA:d24 -m2=LZMA:d19 -m3=LZMA:d19 -mb0:1 -mb0s1:2 -mb0s2:3 && mv core firefox && cat /d/moz/_omnijar_test/ff-opt/browser/locales/../installer/windows/l10ngen/7zSD.sfx /d/moz/_omnijar_test/mozilla/browser/installer/windows/app.tag app.7z > firefox-4.0b4pre.fr.win32.exe && chmod 0755 firefox-4.0b4pre.fr.win32.exe

<snip>

make[3]: Leaving directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
d:/moz/_omnijar_test/ff-opt/config/nsinstall.exe -D ../../dist/
mv -f "../../dist/l10n-stage/firefox-4.0b4pre.fr.win32.exe" "../../dist/firefox-4.0b4pre.fr.win32.exe"

Comment 88

[Robert Strong (they/them - no direct email)]

Comment on attachment 465132 [details] [diff] [review]
9. Make windows installer locale repacks use the standard repacking path, v4

>diff --git a/browser/installer/windows/nsis/installer.nsi b/browser/installer/windows/nsis/installer.nsi
>--- a/browser/installer/windows/nsis/installer.nsi
>+++ b/browser/installer/windows/nsis/installer.nsi
>@@ -231,17 +231,17 @@ SectionEnd
> Section "-Application" APP_IDX
>   ${StartUninstallLog}
> 
>   SetDetailsPrint both
>   DetailPrint $(STATUS_INSTALL_APP)
>   SetDetailsPrint none
> 
>   ${LogHeader} "Installing Main Files"
>-  ${CopyFilesFromDir} "$EXEDIR\nonlocalized" "$INSTDIR" \
>+  ${CopyFilesFromDir} "$EXEDIR\core" "$INSTDIR" \
>                       "$(ERROR_CREATE_DIRECTORY_PREFIX)" \
>                       "$(ERROR_CREATE_DIRECTORY_SUFFIX)"
> 
>   ; Register DLLs
>   ; XXXrstrong - AccessibleMarshal.dll can be used by multiple applications but
>   ; is only registered for the last application installed. When the last
>   ; application installed is uninstalled AccessibleMarshal.dll will no longer be
>   ; registered. bug 338878
>@@ -266,21 +266,16 @@ Section "-Application" APP_IDX
>   ${LogUninstall} "File: \install_status.log"
>   ${LogUninstall} "File: \install_wizard.log"
>   ${LogUninstall} "File: \updates.xml"
> 
>   SetDetailsPrint both
>   DetailPrint $(STATUS_INSTALL_LANG)
>   SetDetailsPrint none
> 
Remove the previous four lines

>-  ${LogHeader} "Installing Localized Files"
>-  ${CopyFilesFromDir} "$EXEDIR\localized" "$INSTDIR" \
>-                      "$(ERROR_CREATE_DIRECTORY_PREFIX)" \
>-                      "$(ERROR_CREATE_DIRECTORY_SUFFIX)"
>-

I'm going to go check a few more things but overall it looks good... would like to see the patch that fixes the name of the l10n installer and where it ends up as well.

Comment 89

[Michael Wu [:mwu]]

Attachment: 9a. Move the repacked installer to the right path

This does the trick. The repacking code resists moving the file so we just move the file after it's done.

Comment 90

[Robert Strong (they/them - no direct email)]

(In reply to comment #89)
> Created attachment 465166 [details] [diff] [review]
> 9a. Move the repacked installer to the right path
> 
> This does the trick. The repacking code resists moving the file so we just move
> the file after it's done.
This fails for me.

make[3]: Entering directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
rm -f /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/chrome/fr.jar \
          /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/chrome/fr.manifest
 \
          /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/defaults/pref/fire
fox-l10n.js
rm -f -rf  /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/searchplugins \
          /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/dictionaries \
          /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/defaults/profile \

          /d/moz/_omnijar_test/ff-opt/dist/l10n-stage/firefox/chrome/fr
make[3]: Leaving directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
d:/moz/_omnijar_test/ff-opt/config/nsinstall.exe -D ../../dist/
mv -f "../../dist/l10n-stage/firefox-4.0b4pre.fr.win32.exe" "../../dist/firefox-
4.0b4pre.fr.win32.exe"
make[2]: Leaving directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
mv -f "/d/moz/_omnijar_test/ff-opt/dist/firefox-4.0b4pre.fr.win32.zip" ""/d/moz/
_omnijar_test/ff-opt/dist/install/sea/firefox-4.0b4pre.fr.win32.installer.exe""
mv: cannot stat `/d/moz/_omnijar_test/ff-opt/dist/firefox-4.0b4pre.fr.win32.zip'
: No such file or directory
make[1]: *** [repackage-win32-installer] Error 1
make[1]: Leaving directory `/d/moz/_omnijar_test/ff-opt/browser/locales'
make: *** [repackage-win32-installer-fr] Error 2

Comment 91

[Benjamin Smedberg]

Comment on attachment 464921 [details] [diff] [review]
3. Add support for packaging omnijar, v4

* Please rename _MAKE_PACKAGE and _UNMAKE_PACKAGE to INNER_MAKE_PACKAGE... leading underscores are hard to read.

* please use printf instead of echo and add a trailing newline

* is zip -m portable?

Comment 92

[Michael Wu [:mwu]]

Relanded those two patches:

http://hg.mozilla.org/mozilla-central/rev/48dfa2f5d62e
http://hg.mozilla.org/mozilla-central/rev/3d9f5f454c1a

Comment 93

[Michael Wu [:mwu]]

(In reply to comment #91)
> Comment on attachment 464921 [details] [diff] [review]
> 3. Add support for packaging omnijar, v4
> 
> * Please rename _MAKE_PACKAGE and _UNMAKE_PACKAGE to INNER_MAKE_PACKAGE...
> leading underscores are hard to read.
> 
Ok.

> * please use printf instead of echo and add a trailing newline
> 
Ok. (curious to know the reasoning..)

> * is zip -m portable?
It works on windows and manpages indicate it exists on OSX. At the very least, the builds work on try.

Comment 94

[Axel Hecht [:Pike]]

echo proved itself to be a portability pain, fwiw.

Comment 95

[Benjamin Smedberg]

Yeah, echo doesn't behave the same way in various shells, and printf is highly portable.

Comment 96

[Michael Wu [:mwu]]

Attachment: 9a. Move the repacked installer to the right path, v2

I have greater confidence in this one. Started the installer to be sure.

Comment 97

[(not currently active) Ted Mielczarek]

Comment on attachment 461065 [details] [diff] [review]
8. Bring universal binaries into the world of omnijar, v2

># HG changeset patch
># Parent f37600e6f77e1738b9fc15d4655c9a9e80951f4e
>
>diff --git a/build/macosx/universal/fix-buildconfig b/build/macosx/universal/fix-buildconfig
>--- a/build/macosx/universal/fix-buildconfig
>+++ b/build/macosx/universal/fix-buildconfig
>@@ -42,46 +42,66 @@ use Archive::Zip(':ERROR_CODES');
> 
> my ($BUILDCONFIG);
> 
>-sub fixBuildconfig($$);
>+sub fixBuildconfig($$$);
> 
> $BUILDCONFIG = 'content/global/buildconfig.html';
> 
>-if (scalar(@ARGV) != 2) {
>-  print STDERR ("usage: fix-buildconfig <zipfile1> <zipfile2>\n");
>+if (scalar(@ARGV) != 3) {
>+  print STDERR ("usage: fix-buildconfig <jar or file mode> <zipfile1> <zipfile2>\n");

I'd write that as "usage: fix-buildconfig <jar|file> <file1> <file2>\n"

>   exit(1);
> }
> 
>-if (!fixBuildconfig($ARGV[0], $ARGV[1])) {
>+if (!fixBuildconfig($ARGV[0], $ARGV[1], $ARGV[2])) {
>   exit(1);
> }
> 
> exit(0);
> 
>-sub fixBuildconfig($$) {
>-  my ($zipPath1, $zipPath2);
>-  ($zipPath1, $zipPath2) = @_;
>+sub fixBuildconfig($$$) {
>+  my ($mode, $path1, $path2);
>+  ($mode, $path1, $path2) = @_;

You should probably just check that $mode == 'jar' or 'file' up top here and return, so you don't have to check it each time below.

r=me with those two nits fixed.

Comment 98

[Michael Wu [:mwu]]

Attachment: 3. Add support for packaging omnijar, v5

All review comments addressed.

Comment 99

[Michael Wu [:mwu]]

Attachment: 9. Make windows installer locale repacks use the standard repacking path, v5

Comment 100

[Kev Needham [:kev]]

wrt comment #85: thanks for the heads-up. ccooper: we'll need to make sure we track for the repack script(s).

Comment 101

[Axel Hecht [:Pike]]

Comment on attachment 465366 [details] [diff] [review]
9. Make windows installer locale repacks use the standard repacking path, v5

Did you test all three results, that is, exe, zip, and mar, to yield the right thing, on a virgin build dir? That is, create a new build dir, copy the zip and exe over from the old into dist..., build mar, and then repack.

I'm really sorry that I don't remember why I needed this, I only remember that I figured out something broke way late last time around.

Also, did you get info from releng how they're calling this code in release factories to get the prettified binary names? Again, sadly I only know it's different, and it was too cumbersome for me to remember. IIRC, I had to have someone from releng jump in and fix my regressions.

Comment 102

[Michael Wu [:mwu]]

(In reply to comment #101)
> Comment on attachment 465366 [details] [diff] [review]
> 9. Make windows installer locale repacks use the standard repacking path, v5
> 
> Did you test all three results, that is, exe, zip, and mar, to yield the right
> thing, on a virgin build dir? That is, create a new build dir, copy the zip and
> exe over from the old into dist..., build mar, and then repack.
> 
I did copy to a new build dir and ensure repacking with a --disable-compile-environment objdir works correctly. Some fixes were applied after that. I didn't test mars though I think robstrong may have. (mostly because I don't know how to test mars)

> I'm really sorry that I don't remember why I needed this, I only remember that
> I figured out something broke way late last time around.
> 
Understandable. It's pretty complicated.

> Also, did you get info from releng how they're calling this code in release
> factories to get the prettified binary names? Again, sadly I only know it's
> different, and it was too cumbersome for me to remember. IIRC, I had to have
> someone from releng jump in and fix my regressions.
I saw the code that makes the pretty release build names. As far as I can tell, this correctly accounts for that. The final repacked filenames and locations should be the same as before. This code still uses the logic from package-name.mk to name files.

Comment 103

[Michael Wu [:mwu]]

Attachment: 14. Turn on omnijar via confvars.sh

This is the scariest one line patch I've written all year.

Comment 104

[Robert Strong (they/them - no direct email)]

mwu, I haven't checked the installer end result when MOZ_OMNIJAR isn't defined and unless I'm mistaken (though I haven't checked) you haven't made it a requirement when building with the installer. Could you provide details to this affect?

Comment 105

[Michael Wu [:mwu]]

(In reply to comment #104)
> mwu, I haven't checked the installer end result when MOZ_OMNIJAR isn't defined
> and unless I'm mistaken (though I haven't checked) you haven't made it a
> requirement when building with the installer. Could you provide details to this
> affect?
The omnijar repacking is handled somewhere inside unpack/repackage-zip and is automatically turned off when MOZ_OMNIJAR isn't defined. I've successfully done a regular jar build for seamonkey to test this case.

See patch 3 for details. Basically, UNMAKE/MAKE_PACKAGE is modified when MOZ_OMNIJAR is defined to pack up the omnijar before packing everything else up, and to unpack the omnijar after unpacking the main package.

Comment 106

[Benjamin Smedberg]

Comment on attachment 465452 [details] [diff] [review]
14. Turn on omnijar via confvars.sh

r=me, but please don't land this part until I coordinate some l10n testing

Comment 107

[Michael Wu [:mwu]]

Attachment: 12. Add 60+ files to the removed-files.in, v3

en_US.manifest -> @AB_CD@.manifest and remove all the manifest files that are already specified. Also remove classic.jar. Also adds omni.jar to the list when MOZ_OMNIJAR isn't defined to allow switching back to normal packaging if we ever need it.

Comment 108

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 465366 [details] [diff] [review]
9. Make windows installer locale repacks use the standard repacking path, v5

This looks great, and between :rs and the bug :bs filed I'm confident it'll get enough testing before landing.

Also, unifying code paths is awesome!

Comment 109

[Robert Strong (they/them - no direct email)]

Comment on attachment 465498 [details] [diff] [review]
12. Add 60+ files to the removed-files.in, v3

diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in
--- a/browser/installer/removed-files.in
>+>+>+ b/browser/installer/removed-files.in
@@ -21,7 >+21,7 @@ chrome/classic.manifest
 chrome/comm.jar
 chrome/comm.manifest
 chrome/en-win.jar
-chrome/en-US.manifest
>+chrome/@AB_CD@.manifest
Thanks!

 chrome/help.jar
 chrome/installed-chrome.txt
 chrome/m3ffxtbr.jar
@@ -545,6 >+545,171 @@ uninstall/UninstallFirefox.exe
 uninstall/uninst.exe
 uninstall/uninstall.exe
 xpicleanup@BIN_SUFFIX@
>+#ifdef MOZ_OMNIJAR
>+chrome/@AB_CD@.jar
these should be indented just like the contents of the other ifdefs to make it obvious where they start and end

>...
>+modules/AddonLogging.jsm
>+modules/AddonManager.jsm
>+modules/AddonRepository.jsm
>+modules/AddonUpdateChecker.jsm
>+modules/CertUtils.jsm
>+modules/CrashSubmit.jsm
>+modules/CSPUtils.jsm
>+modules/ctypes.jsm
>+modules/debug.js
>+modules/distribution.js
>+modules/DownloadLastDir.jsm
>+modules/DownloadPaths.jsm
>+modules/DownloadUtils.jsm
>+modules/FileUtils.jsm
Looks like Geometry.jsm is missing

>+modules/HUDService.jsm
>+modules/InlineSpellChecker.jsm
>+modules/ISO8601DateUtils.jsm
>+modules/LightweightThemeConsumer.jsm
>+modules/LightweightThemeManager.jsm
>+modules/Microformats.js
>+modules/NetUtil.jsm
>+modules/NetworkPrioritizer.jsm
>+modules/openLocationLastURL.jsm
>+modules/PerfMeasurement.jsm
>+modules/PlacesDBUtils.jsm
>+modules/PlacesUIUtils.jsm
>+modules/PlacesUtils.jsm
>+modules/PluginProvider.jsm
>+modules/PluralForm.jsm
>+modules/PopupNotifications.jsm
>+modules/Services.jsm
>+modules/SpatialNavigation.js
>+modules/stylePanel.jsm
>+modules/utils.js
>+modules/WindowDraggingUtils.jsm
Looks like WindowsJumpLists.jsm and WindowsPreviewPerTab.jsm are missing. Should be #ifdef XP_WIN


Still need to go through components

Comment 110

[Robert Strong (they/them - no direct email)]

Comment on attachment 465498 [details] [diff] [review]
12. Add 60+ files to the removed-files.in, v3

>+components/nsDefaultCLH.js
>+components/nsDownloadManagerUI.js
>+components/nsFilePicker.js
I believe this is XP_UNIX only and should be
#ifdef XP_UNIX
  #ifndef XP_MACOSX
    components/nsFilePicker.js
  #endif
#endif

Comment 111

[Robert Strong (they/them - no direct email)]

All new files will need to be added to removed-files.in unless / until we make the decision that we won't allow updating from omnijar to jar or jar to omnijar builds. I think we should make the decision that we won't support anything besides going from jar to omnijar.

Comment 112

[Michael Wu [:mwu]]

Attachment: 8. Bring universal binaries into the world of omnijar, v3

Review comments addressed and build verified working on try server.

Comment 113

[Michael Wu [:mwu]]

Attachment: 12. Add 60+ files to the removed-files.in, v4

Review comments addressed.

Comment 114

[Michael Wu [:mwu]]

(In reply to comment #111)
> All new files will need to be added to removed-files.in unless / until we make
> the decision that we won't allow updating from omnijar to jar or jar to omnijar
> builds. I think we should make the decision that we won't support anything
> besides going from jar to omnijar.
Once we're omnijar has stuck, I think we can stop caring about adding new files to removed-files.in. (and if there's an emergency need to switch back to jar, we could add the files then..)

Comment 115

[Michael Wu [:mwu]]

Universal binaries patch: (8)
http://hg.mozilla.org/mozilla-central/rev/810105bfc749

Fix leak in child process: (13)
http://hg.mozilla.org/mozilla-central/rev/1bdb6586b0a0

Add support for packaging omnijar: (3)
http://hg.mozilla.org/mozilla-central/rev/d37b6b337227

Make windows installer locale repacks use the standard repacking path: (9)
http://hg.mozilla.org/mozilla-central/rev/7cf1fba2029a

Add omnijar removed files: (12)
http://hg.mozilla.org/mozilla-central/rev/b799524f7810

Comment 116

[Michael Wu [:mwu]]

Attachment: 9b. Update SFX7Z to use INNER_MAKE/UNMAKE_PACKAGE

Opps. Forgot to update the windows locale repacks patch for the changes in the latest omnijar packaging patch.

Comment 117

[Michael Wu [:mwu]]

jst checked in this fix for me.

http://hg.mozilla.org/mozilla-central/rev/e63e4c634f75

Comment 118

[Benjamin Smedberg]

At this point, we're planning on turning on desktop omnijar by default right after beta4 branches, so that we can get a full two weeks of bake time and test a dry-run localized beta.

Comment 119

[[e-mail]]

(In reply to comment #47)
> Created attachment 460956 [details] [diff] [review]
> 7. Fix tests, v3
Nice to see the tests actually pass with flat chrome :-)

Comment 120

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/5ca9e053ee98

Comment 121

[Bobby Holley (:bholley)]

I believe this broke non-libxul builds on mac. Thoughts?

Comment 122

[Bobby Holley (:bholley)]

(In reply to comment #121)
> I believe this broke non-libxul builds on mac. Thoughts?

Filed bug 588075 for this.

Comment 123

[Michael Wu [:mwu]]

Attachment: 12a. Another update to removed-files.in

Comment 124

[Peter Henkel [:Terepin]]

After landing this in trunk, I can't no longer start Firefox.

Comment 125

[Ben Bucksch (:BenB)]

Peter Henkel, please file a bug, with sufficient information (mozconfig, platform, anything else relevant for your build) to reproduce, and make it block this bug.

Comment 126

[Axel Hecht [:Pike]]

Filed bug 588386 on broken l10n repacks on the mac.

Comment 127

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out because it blew up update packaging on win32.

Comment 128

[Peter Henkel [:Terepin]]

That might be the reason why I can't start Firefox at all.

Comment 129

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It appears to be just a packaging issue.

Comment 130

[Benjamin Smedberg]

/e/builds/moz2_slave/mozilla-central-win32-nightly/build/tools/update-packaging/make_full_update.sh: line 75: e:/builds/moz2_slave/mozilla-central-win32-nightly/build/obj-firefox/dist/host/bin/mar.exe: Bad file number mv: cannot stat `../../dist/firefox.work/output.mar': No such file or directory make: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32-nightly/build/obj-firefox/tools/update-packaging' program finished with exit code 0

khuey, is that from the nightly build log?

Comment 131

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes.

Comment 133

[Michael Wu [:mwu]]

Attachment: 9c. Do omnijar packing in dist/firefox instead of installer staging

The updater code expects that dist/firefox contains what we ship. make package does this but make installer does not. The updater code runs right after make installer, so mar generation for windows fails. This patch ensures dist/firefox is left omnijar'd after make installer.

Comment 134

[Robert Strong (they/them - no direct email)]

bah... I was able to create a full mar but didn't try to after running make installer. Sorry for not catching that. :(

Comment 135

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 467247 [details] [diff] [review]
9c. Do omnijar packing in dist/firefox instead of installer staging

Nice.

Comment 136

[pal-moz]

cannot start.
with new/clean profile.

http://forums.mozillazine.org/viewtopic.php?p=9773093#p9773093

Comment 137

[Robert Strong (they/them - no direct email)]

mwu, looks like Kyle also backed out the updated removed-files.in patch
http://hg.mozilla.org/mozilla-central/rev/5d36870125a7

Comment 138

[Michael Wu [:mwu]]

(In reply to comment #137)
> mwu, looks like Kyle also backed out the updated removed-files.in patch
> http://hg.mozilla.org/mozilla-central/rev/5d36870125a7

Thanks for the heads up. I've asked cjones to check that patch back in with his push.

Comment 139

[pal-moz]

seems to be fine with next released Hourly,
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1282197742/

changeset, http://hg.mozilla.org/mozilla-central/rev/9897ac78e6e3

Comment 140

[Axel Hecht [:Pike]]

At least one bustage in the l10n repacks is 

/tools/buildbot/bin/python2.6: can't open file '../../config/optimizejars.py': [Errno 2] No such file or directory

I've also seen 

mv /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/MacOS/chrome/cy.manifest /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/MacOS/chrome/localized.manifest
mv /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/Resources/en.lproj /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/Resources/cy.lproj
mv: rename /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/Resources/en.lproj to /builds/slave/mozilla-central-macosx64-l10n-nightly/build/mozilla-central/browser/locales/../../dist/l10n-stage/firefox/Minefield.app/Contents/Resources/cy.lproj: No such file or directory

Comment 141

[Robert Kaiser]

(In reply to comment #140)
> At least one bustage in the l10n repacks is 
> 
> /tools/buildbot/bin/python2.6: can't open file '../../config/optimizejars.py':
> [Errno 2] No such file or directory

That's from bug 559961, actually.

Comment 142

[Robert Strong (they/them - no direct email)]

Looks like DownloadTaskbarProgress.jsm needs to be added to removed-files.in

Comment 143

[Michael Wu [:mwu]]

Attachment: 12b. Yet another removed-files.in update

I just noticed that we're leaving directories behind. Does adding directories to removed-files.in remove them? Would be nice to clean those up too.

Comment 144

[Robert Strong (they/them - no direct email)]

Comment on attachment 467470 [details] [diff] [review]
12b. Yet another removed-files.in update

That should be windows only. Fix that and r=me.

Would be nice but app update doesn't support directory removal - bug 386760.

Comment 145

[Michael Wu [:mwu]]

Attachment: 12b. Yet another removed-files.in update, v2

Comment 146

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/6eead86e13dd

(Leaving RESO->FIXED for mwu; not sure if this is the last.)

Comment 147

[Michael Wu [:mwu]]

Optimistically resolving as fixed. Received my OSX nightly update fine. Also seems that l10n is ok, though other things broke l10n for unrelated reasons. Will fix any other issues in followups.

Comment 148

[pal-moz]

sorry, if wrong place

[STR]
create new profile
no "chrome" folder (also userChrome-example.css etc.) in new profile

caused by omnijar ?

Comment 149

[PikeUK]

(In reply to comment #146)
> http://hg.mozilla.org/mozilla-central/rev/6eead86e13dd
> 
> (Leaving RESO->FIXED for mwu; not sure if this is the last.)

Mozilla/5.0 (Windows NT 6.1; rv:2.0b5pre) Gecko/20100822 Minefield/4.0b5pre

FWIW on the above nightly the following files are found both inside and outside omni.jar:

modules/PropertyPanel.jsm
res/fonts/mathfontSymbol.properties

Comment 150

[Robert Strong (they/them - no direct email)]

(In reply to comment #149)
filed bug 589738

Comment 151

[fxtech]

same for this file

Warning: Cannot load binary components from the omnijar.
Source File: C:\Program Files\Firefox\omni.jar:components/components.manifest
Line: 109

the file is outside of the jar

Comment 152

[Frank Wein [:mcsmurf]]

I think this bug here might have broken MOZ_OPTIONAL_PKG_LIST, see Bug 590575 (non-omnijar build). It looks like everything from optional\ is also packaged in the core\ folder.

Comment 153

[Frank Wein [:mcsmurf]]

(In reply to comment #152)
> I think this bug here might have broken MOZ_OPTIONAL_PKG_LIST, see Bug 590575
> (non-omnijar build). It looks like everything from optional\ is also packaged
> in the core\ folder.

If I see this correctly, Attachment 465366 [details] [diff] is to blame for this. Now everything from dist/bin/ just gets copied into the core/ folder:
@cp -av $(DIST)/$(STAGEPATH)$(MOZ_PKG_DIR)$(_BINPATH)/. $(DEPTH)/installer-stage/core

Comment 154

[Mike Beltzner [:beltzner, not reading bugmail]]

FTR, we should have had a separate bug for turning omnijar support on for desktop builds so that it could have more clearly tracked its dependency on bug 586837

Comment 155

[Benjamin Smedberg]

I'm not sure what the last comment means. I really don't think another bug would have bought us extra clarity, overall.

Comment 156

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, then I suppose my issue is that bug 586837 didn't actually block this bug, and should have done, since it's marked that way. If we didn't believe that issue to be serious enough to stop omnijar from being turned on on desktops, then it should have been filed as dependent on this one, not blocking.

Comment 157

[Benjamin Smedberg]

For lack of better "related to" relationships in bugzilla, I don't think nitpicking the depends-on relationships is helpful. We had hoped to get it done first, but I decided that it was more important to get it landed early in the b5 cycle than to wait on dry-run testing.

Comment 158

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #155)
> I really don't think another bug would have bought us extra clarity, overall.

Landing multiple independent patches days apart (including across a beta freeze) and trying to track them all in one bug is painful and leads to all sorts of coordination issues. This work should have been split across more than one bug.

Comment 160

[dickvl]

Moving all file and directories from defaults/pref to omni.jar has made it impossible or unclear on how to specify and use mozilla.cfg to configure and lock prefs.

Specifying a mozilla.cfg via a pref in a file in omni.jar gives "Failed to read the configuration file. Please contact your system administrator."

I've created bug 595522 about that problem.
Can someone look at it and make changes if needed to make sure that this gets attention in one of the coming beta releases. This needs to be fixed in a beta so mozilla.cfg will work in the release candidate and the final release.

Works:      2010-08-17 : SourceStamp=116f2046b9ef
Don't work: 2010-08-18 : SourceStamp=f4c97baa0e51

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=f4c97baa0e51