Last Comment Bug 588067 - Switch SeaMonkey to use omnijar
: Switch SeaMonkey to use omnijar
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1b2
Assigned To: Robert Kaiser
:
:
Mentors:
Depends on: 630453 394502 556644 601571 601573 626970 627417 627455 628079 629037 629967 630124 630253
Blocks: 716392
  Show dependency treegraph
 
Reported: 2010-08-17 09:12 PDT by Robert Kaiser
Modified: 2012-01-08 10:45 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wanted


Attachments
v1: make it work basically (24.55 KB, patch)
2010-12-12 15:56 PST, Robert Kaiser
no flags Details | Diff | Splinter Review
v1.1: moved to build stage, repacks tested (26.44 KB, patch)
2010-12-19 08:44 PST, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
ChatZilla, v1: only remove subdir when .jar exists (1022 bytes, patch)
2010-12-19 09:04 PST, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review
venkman, v1: only remove subdir when .jar exists (1016 bytes, patch)
2010-12-19 09:06 PST, Robert Kaiser
bugspam.Callek: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-17 09:12:39 PDT
SeaMonkey should switch to omnijar at least for nightly/release builds, as they should cause a significant perf increase.
Comment 1 Benjamin Smedberg [:bsmedberg] 2010-08-17 11:30:37 PDT
Have you tried it? I don't think omnijar builds without libxul (that is, xpcom/components links directly with libjar stuff).
Comment 2 Robert Kaiser 2010-08-17 11:37:21 PDT
Benjamin, it's clear to me that we are probably not there, but then, libxul is among the firm goals for SeaMonkey 2.1, and so should omnijar probably be.
Thanks for the pointer though, will mark the dependency.
Comment 3 Justin Wood (:Callek) 2010-09-07 20:23:20 PDT
IFF we get to libxul for 2.1, we should look into difficulty of getting omnijar working reliably, but the fatter goals for us and libxul are of course OOPP and libxul perf improvements themselves.
Comment 4 Philip Chee 2010-11-02 02:51:25 PDT
q.v. Bug 601573 - Support omnijar in Thunderbird
Comment 5 Robert Kaiser 2010-12-12 15:56:32 PST
Created attachment 497196 [details] [diff] [review]
v1: make it work basically

Here's the basic patch to do this. I still need to test L10n repackages, and check for files that need to be removed for other platforms than Linux, but basically, it should work.

The hack to get extensions packed up is ugly, but unfortunately, I can't find a better way to do this.
Comment 6 neil@parkwaycc.co.uk 2010-12-13 00:38:24 PST
I like to add the -D and -X flags to any zip operation ;-)
Comment 7 Philip Chee 2010-12-13 01:51:00 PST
> --- a/suite/installer/removed-files.in
> +#ifdef MOZ_OMNIJAR
> +  chrome/en-US.jar

Shouldn't this be:

chrome/@AB_CD@.jar

since earlier in /package-manifest.in you do:
> -@BINPATH@/chrome/@AB_CD@.jar
> +@BINPATH@/chrome/@AB_CD@@JAREXT@

> +  res/fonts/mathfont.properties
> +  res/fonts/mathfontStandardSymbolsL.properties
> +  res/fonts/mathfontSTIXNonUnicode.properties
> +  res/fonts/mathfontSTIXSize1.properties
> +  res/fonts/mathfontSTIXSizeOneSym.properties
> +  res/fonts/mathfontUnicode.properties

Do we package: res/fonts/mathfontSymbol.properties ?
Comment 8 Robert Kaiser 2010-12-14 10:39:15 PST
(In reply to comment #7)
> Shouldn't this be:
> chrome/@AB_CD@.jar

Er, right, probably.

> Do we package: res/fonts/mathfontSymbol.properties ?

Apparently we do, as this list comes from a diff of file lists in a package from before and after the patch.
Comment 9 Robert Kaiser 2010-12-19 08:44:17 PST
Created attachment 498609 [details] [diff] [review]
v1.1: moved to build stage, repacks tested

I have moved the extension packaging step into the build stage (IMHO, ideally, this should be done automatically and omni.jar should also be generated at build stage, but let's use the general mechanism as much as possible).

Also, this got slight changes to make it work flawlessly with L10n repackaging (which is now tested as well).

We'll need small fixups for ChatZilla and venkman in terms of L10n repack, but those are trivial and in build system code only used by SeaMonkey.
Comment 10 Robert Kaiser 2010-12-19 09:04:42 PST
Created attachment 498611 [details] [diff] [review]
ChatZilla, v1: only remove subdir when .jar exists

Callek, I'm handing those to your review as well, as it's purely build system and code only used by SeaMonkey anyhow.
The remove command in question was to remove a remnant directory that isn't used/needed when we're packaging the traditional jarred extension. Now, with omnijar, there is no .jar and all files end up in that directory, so we need to keep it. As it's a cleanup for when we have a .jar only, it's best to do the if on the existence of the .jar, IMHO.
The same patch is coming for venkman as well.
Comment 11 Robert Kaiser 2010-12-19 09:06:33 PST
Created attachment 498614 [details] [diff] [review]
venkman, v1: only remove subdir when .jar exists

Here's the venkman part. Of course, the repackage-zip target is only called in the L10n repack process, which I tested but you might not know/want to, but I think it's trivial to see what it's doing or if it's correct.
Comment 12 Justin Wood (:Callek) 2011-01-13 19:40:40 PST
Comment on attachment 498609 [details] [diff] [review]
v1.1: moved to build stage, repacks tested

>diff --git a/suite/app/Makefile.in b/suite/app/Makefile.in
>--- a/suite/app/Makefile.in
>+++ b/suite/app/Makefile.in
>@@ -290,16 +290,55 @@ ifeq ($(OS_ARCH),WINNT)
> 	$(PERL) -pe 's/(?<!\r)\n/\r\n/g;' < $(topsrcdir)/suite/installer/license.txt > $(DIST)/bin/license.txt
> else
> 	$(INSTALL) $(topsrcdir)/suite/installer/license.txt $(DIST)/bin/
> endif
> 
> libs:: blocklist.xml
> 	$(INSTALL) $(IFLAGS1) $^ $(DIST)/bin
> 
>+ifdef MOZ_OMNIJAR
>+# Make extensions end up as XPIs instead of flat chrome when doing omni.jar.
>+# NOTE: This is a hack to run this at the end of compilation, would be nicer
>+# if this was done right away for built-in extensions in omnijar mode.
>+EXTENSIONS = \
>+  {59c81df5-4b7a-477b-912d-4e0fdf64e5f2} \

Nit: can we assign these GUID's to variables so we can know what they are easier when we are peeking at this hackery of code?

>+define _PACKAGE_EXTENSIONS
>+if test -d "$(ABS_DIST)/extensions/$(dir)"; then \
>+cd $(ABS_DIST)/extensions/$(dir)/; $(ZIP) -r9mX ../$(dir).xpi *; \
>+rm -rf $(ABS_DIST)/extensions/$(dir); \
>+fi

Nit insert an echo for each $(dir) here, so if we are taking a LONG time on some of these, we can see what is going on, rather than simply "Packaging extensions..."

>+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat

I'm not sure if any other files are needed as NON* here, or even if this is; I feel safe taking your word for it though.

>--- a/suite/installer/package-manifest.in

Before landing please just double check this and removed-files to see if things changed from under you, and merge accordingly.

If anyone but you wrote this patch I'd also ask for a second-review, but the only other one probably capable [who deals with suite] is Neil, and since he is overloaded, and you and me both know more build system-fu than him, so the one review should be fine.
Comment 13 Robert Kaiser 2011-01-14 14:49:01 PST
(In reply to comment #12)
> >+EXTENSIONS = \
> >+  {59c81df5-4b7a-477b-912d-4e0fdf64e5f2} \
> 
> Nit: can we assign these GUID's to variables so we can know what they are
> easier when we are peeking at this hackery of code?

You mean variables that are passed into this variable? That sounds strange as well. I wonder if we should just add something to the comment before this that says |Listed extension GUIDs: 59c81df5-...  ChatZilla, 972ce4c6-... Classic Theme, f13b157f-... venkman|, would this be enough for documentation?

> Nit insert an echo for each $(dir) here, so if we are taking a LONG time on
> some of these, we can see what is going on, rather than simply "Packaging
> extensions..."

OK, will do, not that any of those is very large, so probably won't take that long, but if we fail somewhere, it might be helpful to know what has been started last.

> >+NON_OMNIJAR_FILES = defaults/messenger/mailViews.dat
> 
> I'm not sure if any other files are needed as NON* here, or even if this is; I
> feel safe taking your word for it though.

If we need any more, we'll see about those - but Firefox needs none from what I can tell, and this one is the only one Thunderbird needs, so I think this should work for us as well. Else our testers will report problem, I'm sure. :)

> >--- a/suite/installer/package-manifest.in
> 
> Before landing please just double check this and removed-files to see if things
> changed from under you, and merge accordingly.


I'll re-check before checking in, yes. The patch still applies flawlessly at least right now. :)
Comment 14 Justin Wood (:Callek) 2011-01-14 19:39:40 PST
(In reply to comment #13)
> (In reply to comment #12)
> > >+EXTENSIONS = \
> > >+  {59c81df5-4b7a-477b-912d-4e0fdf64e5f2} \
> > 
> > Nit: can we assign these GUID's to variables so we can know what they are
> > easier when we are peeking at this hackery of code?
> 
> You mean variables that are passed into this variable? That sounds strange as

That is what I meant, yes it won't be used anywhere else, but to me it was clearer.

> well. I wonder if we should just add something to the comment before this that
> says |Listed extension GUIDs: 59c81df5-...  ChatZilla, 972ce4c6-... Classic
> Theme, f13b157f-... venkman|, would this be enough for documentation?

I won't block on this relatively minor nit, so if you want that comment solution instead, go ahead.

> Else our testers will report problem, I'm sure. :)

Which is the reason I'm happy to take it without doing heavy investigating :-)
Comment 15 Robert Kaiser 2011-01-18 10:00:24 PST
I took a look at package-compare in my build and didn't spot anything, so I guess package-manifest and removed-files should still be at the same stage as when I created the patch. Please file new bugs if you see anything coming up from the checkin.

Pushed:
http://hg.mozilla.org/chatzilla/rev/6c302afa1b50
http://hg.mozilla.org/venkman/rev/508ba977910a
http://hg.mozilla.org/comm-central/rev/44423b415a5f
Comment 16 Philip Chee 2011-01-19 01:33:51 PST
Windows builds are burning:

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1295417854.1295428553.19418.gz
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1295417664.1295424646.518.gz

make[6]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir/suite/app'
make[5]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir/suite/app'
make[4]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir/suite'
make[3]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir'
make[2]: Leaving directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir'
rm: cannot remove directory `/e/builds/slave/comm-cen-trunk-w32/build/objdir/suite/app/../../mozilla/dist/bin/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}': Permission denied
Comment 17 Serge Gautherie (:sgautherie) 2011-01-19 04:14:45 PST
(In reply to comment #16)
> Windows builds are burning:

Moved to bug 626970.
Comment 18 Robert Kaiser 2011-01-19 05:36:38 PST
We have a lot of test orange, but that is because clobbering didn't work (bug 626164) and a clobber is needed. We had two test s on Linux that ran on builds after an incidental periodic clobber on the -01 machine and those runs were fine. I'm currently going through all machines to do manual clobbers and clear this mess up.
Comment 19 Serge Gautherie (:sgautherie) 2011-01-20 12:13:35 PST
(Maybe this is already written in this bug which I didn't fully read, but)
This bug summary targets nightly/release builds only.
Was it actually intended that opt and debug tinderbox-builds should be affected too?
Comment 20 Justin Wood (:Callek) 2011-01-20 12:20:59 PST
(In reply to comment #19)
> (Maybe this is already written in this bug which I didn't fully read, but)
> This bug summary targets nightly/release builds only.
> Was it actually intended that opt and debug tinderbox-builds should be affected
> too?

Yes, we don't want our infrastructure to produce nightlies/releases which are vastly different than what our tests run with. I adjusted summary accordingly
Comment 21 Serge Gautherie (:sgautherie) 2011-01-31 00:17:55 PST
Ftr, I so much wished this change had been pushed alone and the tree closed as needed :-/
It's a real pain to sort out "all" the regression that happened in this timeframe :-(
Comment 22 Philip Chee 2011-01-31 01:39:18 PST
Some tryserver builds wouldn't have been remiss either. But then hindsight is always 20/20.
Comment 23 andré 2011-07-14 17:09:46 PDT
I know this is off topic, but I'm trying to edit seamonkey's omni.jar on Linux without success.
Could anyone lead me to where I can find the appropriate tools ?
I found optimizejars.py, but without any indication of how to use it (nor other files that may be needed, or versions of python for it)

None of my zip utilities can extract omni.jar properly.
(Although squeeze seems to be able to read it, and do partial extractions very awkwardly.)

Can anyone give me a pointer ?  Thanks in advance.
Comment 24 andré 2011-07-14 19:41:30 PDT
Found solution for opening omni.jar (under Linux at least)

zip -FF {input file} --out {output file}

(The zip utility from info-zip.  Available on many platforms.)

This fixes non-standard zip files.
When recompressed using standard zip, the file is the same size.
(Such recompressed files are reported to work on posts to other bugs.)
I'll report back whether it works.
Comment 25 andré 2011-07-14 20:18:47 PDT
Unfortunately, omni.jar corrected to zip format doesn't work.

(Note : if corrected file is uncompressed then recompressed, it is slightly bigger.)
Now if only there were some indication how to use optimizejars.py ...
Comment 26 Serge Gautherie (:sgautherie) 2011-07-14 21:22:35 PDT
(In reply to comment #23)

> I know this is off topic,

Please, don't spam bugs: SeaMonkey is affected (too) but the "cause" is in Core.

> Can anyone give me a pointer ?  Thanks in advance.

As I've run into this "issue" myself, I finally took some time to do a quick search.
Here are some pointers:
http://blog.mozilla.com/tglek/2010/09/14/firefox-4-jar-jar-jar/
Bug 595473, bug 605524, ...

Note You need to log in before you can comment on or make changes to this bug.