Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Switch SeaMonkey to use omnijar

RESOLVED FIXED in seamonkey2.1b2

Status

SeaMonkey
Build Config
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
seamonkey2.1b2
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.1 wanted)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
SeaMonkey should switch to omnijar at least for nightly/release builds, as they should cause a significant perf increase.
(Assignee)

Updated

7 years ago
status-seamonkey2.1: --- → ?
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).
(Assignee)

Comment 2

7 years ago
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.
status-seamonkey2.1: ? → wanted
(Assignee)

Updated

7 years ago
Depends on: 601571

Comment 4

7 years ago
q.v. Bug 601573 - Support omnijar in Thunderbird
Depends on: 601573
(Assignee)

Comment 5

7 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Attachment #497196 - Flags: review?(bugspam.Callek)

Comment 6

7 years ago
I like to add the -D and -X flags to any zip operation ;-)

Comment 7

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

Comment 8

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

Comment 9

7 years ago
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.
Attachment #497196 - Attachment is obsolete: true
Attachment #498609 - Flags: review?(bugspam.Callek)
Attachment #497196 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 10

7 years ago
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.
Attachment #498611 - Flags: review?(bugspam.Callek)
(Assignee)

Comment 11

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

Updated

7 years ago
Attachment #498611 - Flags: review?(bugspam.Callek) → review+

Updated

7 years ago
Attachment #498614 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Comment 13

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

Comment 15

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1b2

Comment 16

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

Updated

7 years ago
Depends on: 626970
(In reply to comment #16)
> Windows builds are burning:

Moved to bug 626970.
(Assignee)

Comment 18

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

Updated

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

Updated

7 years ago
Depends on: 627417
Depends on: 628079
Depends on: 629037

Updated

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

Comment 22

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

Comment 23

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

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

6 years ago
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, ...
Blocks: 716392
You need to log in before you can comment on or make changes to this bug.