Closed Bug 588067 Opened 14 years ago Closed 13 years ago

Switch SeaMonkey to use omnijar

Categories

(SeaMonkey :: Build Config, defect)

defect
Not set
normal

Tracking

(seamonkey2.1 wanted)

RESOLVED FIXED
seamonkey2.1b2
Tracking Status
seamonkey2.1 --- wanted

People

(Reporter: kairo, Assigned: kairo)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

SeaMonkey should switch to omnijar at least for nightly/release builds, as they should cause a significant perf increase.
OS: Linux → All
Hardware: x86 → All
Have you tried it? I don't think omnijar builds without libxul (that is, xpcom/components links directly with libjar stuff).
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.
Depends on: 394502
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.
Depends on: 601571
q.v. Bug 601573 - Support omnijar in Thunderbird
Depends on: 601573
Attached patch v1: make it work basically (obsolete) — Splinter Review
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #497196 - Flags: review?(bugspam.Callek)
I like to add the -D and -X flags to any zip operation ;-)
> --- 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 ?
(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.
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.
Attachment #497196 - Attachment is obsolete: true
Attachment #498609 - Flags: review?(bugspam.Callek)
Attachment #497196 - Flags: review?(bugspam.Callek)
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.
Attachment #498611 - Flags: review?(bugspam.Callek)
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.
Attachment #498614 - Flags: review?(bugspam.Callek)
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.
Attachment #498609 - Flags: review?(bugspam.Callek) → review+
Attachment #498611 - Flags: review?(bugspam.Callek) → review+
Attachment #498614 - Flags: review?(bugspam.Callek) → review+
(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. :)
(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 :-)
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2
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
Depends on: 626970
(In reply to comment #16)
> Windows builds are burning:

Moved to bug 626970.
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.
Blocks: 627455
(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?
(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
Summary: Switch SeaMonkey to use omnijar for nightly/release builds → Switch SeaMonkey to use omnijar
Depends on: 627417
Depends on: 628079
Depends on: 629037
Depends on: 629967
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 :-(
Depends on: 630124
Some tryserver builds wouldn't have been remiss either. But then hindsight is always 20/20.
Depends on: 630253
Depends on: 630453
No longer blocks: 627455
Depends on: 627455
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.
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.
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 ...
(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, ...
You need to log in before you can comment on or make changes to this bug.