Closed Bug 351715 Opened 18 years ago Closed 17 years ago

Build Chatzilla as extension for suiterunner (toolkit/ style seamonkey)

Categories

(Other Applications :: ChatZilla, defect)

Product:
Other Applications
Component:
ChatZilla
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mcsmurf, Assigned: standard8)

References

(
URL
)

Details

Attachments

(4 obsolete files) 

Currently SeaMonkey is in the process to convert from xpfe/ to toolkit/. To make things easier it would be nice if the Chatzilla build process could be changed to build Chatzilla as extension for SeaMonkey when MOZ_XUL_APP is enabled. It should just copy itself to dist/bin/extensions/ like other extensions in-tree do.
I've put some of the main issues related to this on the wiki. I've had it working here, but I need decisions on how exactly to do the build stuff, whether to be appManaged, etc..
URL: http://wiki.mozilla.org/ChatZilla:Sui...
Attached patch patch v3 (obsolete) — — Splinter Review 
This patch builds ChatZilla as an extension inside the build process, in the same way as DOMI. I still haven't fully convinced myself this is the best way, but it's the best I can figure out at the moment.
Attachment #239185 - Flags: review?(silver)
Comment on attachment 239185 [details] [diff] [review]
patch v3

I've been doing some work on the suiterunner build config, and I've just seen this, so here are some comments (note I haven't applied this patch myself, so please excuse me if I have interpreted things wrongly):

+#	USE_EXTENSION_MANIFEST = 1

I don't see why this is commented out, when building chatzilla in tree as an extension, I'd expect to need this.

-ifdef MOZ_PHOENIX
-	DIRS += ff
...
+ifdef MOZ_XUL_APP
...
 else
 	DIRS += sm
 endif

Does the jar.mn preprocessor still go into sm & ff even if you haven't got those directories in DIRS? If it doesn't I think we're going to be missing a couple of overlays.

+DEFINES += -DCHATZILLA_VERSION=$(CHATZILLA_VERSION)
+
+export::
+	$(NSINSTALL) -D $(FINAL_TARGET)/chrome/icons/default
+	$(INSTALL) $(srcdir)/xpi/resources/chatzilla-window.* $(FINAL_TARGET)/chrome/icons/default
+

Interesting that we also have routines to install these in the dist dir from suite/branding. Suggest we'd remove the suite/branding icons also(possibly in another bug).

+CHATZILLA_VERSION=$(shell cat $(srcdir)/version.txt)

If you're going to do that, then you could also preprocess the xul/content/contents.rdf I believe and then you'd only have to update one version number for a CZ update.
(In reply to comment #3)
> Does the jar.mn preprocessor still go into sm & ff even if you haven't got
> those directories in DIRS? If it doesn't I think we're going to be missing a
> couple of overlays.

I didn't address overlays in this patch. The right way to fix it probably depends on what happens in bug 335155.

> Interesting that we also have routines to install these in the dist dir from
> suite/branding. Suggest we'd remove the suite/branding icons also(possibly in
> another bug).

I guess this depends on which of the icons SeaMonkey wants.

> If you're going to do that, then you could also preprocess the
> xul/content/contents.rdf I believe and then you'd only have to update one
> version number for a CZ update.

I'm not sure what uses the version in contents.rdf, but if it's still needed, this makes sense.
As per my comments on bug 371670, this is a summary of why we need to do/change the way we build CZ into SeaMonkey for "suiterunner" builds (which will pick up the new toolkit, more info: http://wiki.mozilla.org/SeaMonkey:suiterunner):

If we stay as we are there are two problems

1) ChatZilla puts items into installed-chrome.txt that we're no longer going to support (bug 366673),
2) Developers/Users will be able to install new versions of chatzilla via add-ons manager so they will have two copies one in chrome one in extensions/... - which one gets used is beyond me and is probably a bad idea.

So building CZ in-tree as an extension would resolve both of these. It would also make test (developer) builds be closer to the way we would like to handle it on the release builds and therefore much better for testing.

We then have the questions of how to handle updates (the appManaged flag as per http://wiki.mozilla.org/ChatZilla:Suiterunner#appManaged) and also packaging for release. If we package the in-tree version of CZ for release, then with how branches typically go, we'll be prompting for an update on the first restart after install (not good?). I think these two questions need thrashing out on the wiki/irc/newsgroups, as they may end up affecting how SeaMonkey handles CZ, DOMI, Venkman etc.

I'll try and set up something up for discussing these in the next few days, but I think building as an extension in-tree is something we need to do anyway for suiterunner.
(In reply to comment #5)
> I'll try and set up something up for discussing these in the next few days, but
> I think building as an extension in-tree is something we need to do anyway for
> suiterunner.
> 
It took a while, but I've now spoken to Gijs in detail on irc, and I've updated the page at http://wiki.mozilla.org/ChatZilla:Suiterunner with various comments. The main SeaMonkey devs haven't raised any objections to what I have put there, so I think its safe to go ahead and do it.

In short, the headlines are:

- We'd like to build CZ as a toolkit extension in-tree for developer use and so we can package CZ as a true add-on extension for installation (which would also allow the user to upgrade it)

- appManaged should be off (for the time being at least - once we get it into the public domain we may need to change what we do for this).

If there's any questions, please ask me here or over irc (Standard8)

Rob are you happy to update the patch?
Blocks: 366673
Attached patch Updated Patch (obsolete) — — Splinter Review 
This is a possible patch for allowing ChatZilla to be built as a "proper" extension in-tree. This is based on the changes that were done with Venkman.

I have not fully tested it yet, I haven't checked for any negative effects on makexpi.sh (though I don't expect there to be any). I do know that at least the basics work for building in-tree.

So please feel free to comment/ping me on irc about it I'll probably try and get round to finishing it at the weekend.
Attachment #239185 - Attachment is obsolete: true
Attachment #239185 - Flags: review?(silver)
(In reply to comment #7)
> This is based on the changes that were done with Venkman.

Don't copy, work from scratch (and documentation) please.

The changes to DIRS in Makefile.in look utterly wrong, as does the export:: section. The locales stuff is definately not right either - I'd rather you ignored that for this bug and not try to change what it does.

Also, note that I'm really busy this week and that there's no way I'm letting this bug land without my own testing, so take your time to get it right.
(In reply to comment #8)
> The changes to DIRS in Makefile.in look utterly wrong, as does the export::
> section. The locales stuff is definately not right either - I'd rather you
> ignored that for this bug and not try to change what it does.

If you want me to keep the ifdefs for FF and SM, then that is fine. I was trying to make the in-tree version reflect as much as possible the real CZ which does include both SM and FF dirs.

Why is the locales stuff not right? Were you thinking of locale extension packages? I can exclude it for this patch, but I can't complete this bug until we fix the locales inclusion (and yes, we can do a patch in a blocking/dependent bug).

> Also, note that I'm really busy this week and that there's no way I'm letting
> this bug land without my own testing, so take your time to get it right.

I never said I wanted to get this in quick. I posted it to get initial feedback - especially considering the discussions that have been going on wrt this area. I'd prefer it if you and others do test the patch, because you may find things that I miss. 
(In reply to comment #9)
> If you want me to keep the ifdefs for FF and SM, then that is fine. I was
> trying to make the in-tree version reflect as much as possible the real CZ
> which does include both SM and FF dirs.

The XPI contains both directories, but it does not *register* both. Your patch registered everything in sight, and definately should not be doing so.

> Why is the locales stuff not right? Were you thinking of locale extension
> packages? I can exclude it for this patch, but I can't complete this bug until
> we fix the locales inclusion (and yes, we can do a patch in a
> blocking/dependent bug).

The changes in locales will build whichever locale is selected into the app-ext dir as part of the main extension (or its XPI) AFAICT. That is significantly changing the current setup and definately should not be part of this bug.

I'm annoyed with the locale mess which has landed as it is, I don't want any further changes to it (in fact, I want the current lot backed out, but that's another story).

> I never said I wanted to get this in quick.

I never said you did.
(In reply to comment #10)
> The changes in locales will build whichever locale is selected into the app-ext
> dir as part of the main extension (or its XPI) AFAICT. That is significantly
> changing the current setup and definately should not be part of this bug.

Actually, that _is_ the current (suboptimal, I admit) setup. And I requested Mark to do that patch so I can build the more correct version for the locales stuff based on this - it just wouldn't make sense to do my further work based on the non-as-extension chatzilla.

I'm not saying I need this patch checked in, but I need it as a base to figure out the technical approach to the correct way of doing the locales stuff we seem to reach an agreement on in the newsgroups.
(In reply to comment #10)
> The XPI contains both directories, but it does not *register* both. Your patch
> registered everything in sight, and definately should not be doing so.

You're wrong here, the patch does _not_ register everything in sight, it only registers the FF overlay for FF and the SeaMonkey overlays for SeaMonkey, that's what all the application= flags are about - though I guess the style override could need such a flag as well.

Mark, with my work on improving the locale stuff as discussed elsewhere, I noticed that you also should update the packages files for SeaMonkey along with that change here.
(In reply to comment #12)
> Mark, with my work on improving the locale stuff as discussed elsewhere, I
> noticed that you also should update the packages files for SeaMonkey along with
> that change here.

I was going to cover the SeaMonkey specific changes in a different bug, as there is one other change than just the packages files that we'll be wanting to make. This bug can just focus on getting Chatzilla right.

(In reply to comment #12)

So can you explain how the following lines will only be applied on certain applications (first 2, Firefox, rest, SeaMonkey):

+% content chatzilla-ff %content/chatzilla/ff/
+% overlay chrome://chatzilla/content/chatzilla.xul chrome://chatzilla/content/ff/overlay.xul
+% content chatzilla-sm %content/chatzilla/sm/
+% overlay chrome://chatzilla/content/chatzilla.xul chrome://chatzilla/content/sm/overlay.xul
+% overlay chrome://chatzilla/content/chatzilla.xul chrome://communicator/content/utilityOverlay.xul
+% overlay chrome://chatzilla/content/menus.xul chrome://communicator/content/tasksOverlay.xul
True, if those get applied everywhere, they probably also need the application= flags. I am/was not sure if those jar.mn files are even parsed (and those lines written to chrome.manifest) in all cases or not. I thought the build process might only descend in their directories when the respective apps are built. If we always descend into them, we need to add the application= flags on them - if not, the flags won't hurt, so we could probably always add them to make the intentions clear.
The build process only decends into the correct one, based on Makefile.in logic; however, the patch removes the logic and replaces it with:

+DIRS += locales ff sm

Which means, with the patch, they will both be processed whatever the application.
Blocks: 397246
Ah, ok, somehow looked over that.
Ideally, we'd generate the same XPI in the Makefile build process as we create with makexpi - but of course we need the application flags there in this case, that's right.
(In reply to comment #16)
> The build process only decends into the correct one, based on Makefile.in
> logic; however, the patch removes the logic and replaces it with:
> 
> +DIRS += locales ff sm
> 
> Which means, with the patch, they will both be processed whatever the
> application.

Yes, I missed that.

(In reply to comment #17)
> Ah, ok, somehow looked over that.
> Ideally, we'd generate the same XPI in the Makefile build process as we create
> with makexpi - but of course we need the application flags there in this case,
> that's right.

Well we're not going to be able to generate exactly the same XPI as we're going to need to have chrome.manifest pre-generated in the in-tree case (if it isn't pre-generated then it gives us major problems with installation).

So it probably doesn't matter too much whether we process both the directories (with application restrictions) or only one via Makefile.in logic, but depends on how similar we want the offical XPI and in-tree builds to be.
While the pre-generated chrome.manifest is not strictly needed when ChatZilla is installed as a normal XPI, it doesn't harm to have it there, as older application versions should just ignore it and use contents.rdf instead, from what I know
What I'd like to see is that the XPI generated by the build system can just be taken as used just like the one generated with makexpi, so the smaller the differences, the better.
Attached patch Updated Patch v2 (obsolete) — — Splinter Review 
I'm now happy that this patch is working correctly.

The sm and ff chrome registration is now limited to be app specific based on the application id.

The locales/ changes have been dropped as agreed previously.

I have checked that makexpi.sh generates the same xpi, (bar the addition of one comment line due to variable substitution).

I've also done a difference between the makexpi.sh generated xpi and what ends up in the $(objdir)/dist/bin/extensions/{59c81df5-4b7a-477b-912d-4e0fdf64e5f2} dir for CZ:

chrome/content/chatzilla/contents.rdf - locales version is 1.9a1 in the objdir instead of "". This is caused by the trunk makefiles substituting the locale version. I don't think this matters as we'll have a pre generated chrome.manifest and therefore we won't normally use contents.rdf.

chrome.manifest only exists in the objdir version, this is what we need to ship with to avoid startup problems. Hence no problem here.

The locale directory exists in $(objdir)/dist/bin/chrome/chatzilla.jar which is what we expect as the locale changes will be covered by a different bug. Doesn't appear to cause any problems with running chatzilla with them in separate locations.

install.js is only in the xpi, which again is what I'd expect as we don't need it for trunk installs/shipments.

I've also run it through a build of a trunk version of firefox and that seems to work fine as well.
Attachment #280376 - Attachment is obsolete: true
Attachment #285404 - Flags: review?(silver)
It looks generally OK, although I remain annoyed at the whole version.txt thing.

One thing that puzzels me at the moment, though, is the make-jars.pl changes in makexpi.sh. It feels to me like the change will omit the /sm and /ff parts from the JAR.

When testing this, did you run "makexpi.sh clean" before "makexpi.sh"? By default, makexpi.sh keeps the JAR and XPI build trees around (just like a depend build of native code) and make-jars.pl is unable to know when to remove files from a JAR (just as it is when building the full products).
Comment on attachment 285404 [details] [diff] [review]
Updated Patch v2

Yes the makexpi.sh change for the sm and ff dirs is wrong. My fault for not realising/thinking about "makexpi.sh clean".

For the version.txt, I didn't realise you didn't like it before.

Would you prefer to keep it in static.js if we can and use grep/sed from Makefile.in(s)?
Attachment #285404 - Attachment is obsolete: true
Attachment #285404 - Flags: review?(silver)
We (ChatZilla) need to document our special XPI builder, too. :)

I think it must have been a different bug where I originally mentioned my dislike of the version.txt thing. I don't have a big problem with it, but it seems like needless change, introduces an LXR vs. JAR inconsistency in the code (possibly messing up line numbers - I'm not sure these days if it still does that) and having to keep no line end in it adds to the number of things anyone can accidentally get wrong (especially as diff calls it out).

If you could use the same line/command in the makefile as makexpi.sh uses, that would be great from my POV.
stealing as I seem to be doing this now ;-)
Assignee: rginda → bugzilla
Attached patch Updated Patch v3 (checked in) (obsolete) — — Splinter Review 
This one drops the use of the version.txt file, and sorts out the ff and sm dir processing in makexpi.sh.

I had to move the comments to the start of the xpi file because the preprocessor didn't like them part way down.

Now I find no differences in the xpi files when running makexpi.sh with and without this patch.

The differences between running the makexpi.sh and the objdir are the same as I listed before in comment 20.
Attachment #285473 - Flags: review?(silver)
Comment on attachment 285473 [details] [diff] [review]
Updated Patch v3 (checked in)

r=silver with one nit:

Do you still need the change to static.js in jar.mn (to add the *)? If not, please leave it without the *.
Attachment #285473 - Flags: review?(silver) → review+
Blocks: 401478
Comment on attachment 285473 [details] [diff] [review]
Updated Patch v3 (checked in)

I checked this in earlier tonight without the change in the jar.mn to process static.js.

I'm leaving this open until bug 397246 is fixed as the move to being a full extension isn't complete yet.
Attachment #285473 - Attachment description: Updated Patch v3 → Updated Patch v3 (checked in)
Why do you need the changes to extensions/irc/ff/Makefile.in and extensions/irc/sm/Makefile.in in addition to the main Makefile.in?
Are they needed to make the % content and % overlay rules in the respective jar.mn work? Can't you use "export" in the main makefile.in?
And don't the multiple INSTALL_EXTENSION_ID commands mean that the extension is installed twice, once from the main makefile and once from the submakefile?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/config/rules.mk&rev=3.575&mark=1808-1818#1808
(In reply to comment #28)
>And don't the multiple INSTALL_EXTENSION_ID commands mean that the extension is
>installed twice, once from the main makefile and once from the submakefile?
I think they were copying DOM Inspector, which makes the same mistake.
is there any special reason why the [ff|sm]/jar.mn weren't merged into the main jar.mn?
(In reply to comment #28)
> Why do you need the changes to extensions/irc/ff/Makefile.in and
> extensions/irc/sm/Makefile.in in addition to the main Makefile.in?
> Are they needed to make the % content and % overlay rules in the respective
> jar.mn work? Can't you use "export" in the main makefile.in?

The changes are necessary to make the % content and % overlay rules work. My Makefile knowledge isn't good enough to use export and no one has really suggested it before or given any examples.

> And don't the multiple INSTALL_EXTENSION_ID commands mean that the extension is
> installed twice, once from the main makefile and once from the submakefile?

Yes, however I don't see a way around that - I believe its a limitation of the build system. If you only specify INSTALL_EXTENSION_ID in the top-level makefile then the sub-directory items won't be included, because the top-level makefile installation is run first.

(In reply to comment #30)
> is there any special reason why the [ff|sm]/jar.mn weren't merged into the main
> jar.mn?

I felt it best that the patch created the minimal changes to the existing chatzilla build system. If anyone wants to propose changes to the structure, I'm sure that could be done in a different bug.
Comment on attachment 285473 [details] [diff] [review]
Updated Patch v3 (checked in)

Plop.
Attachment #285473 - Attachment is obsolete: true
Is this the bug to make it so we can check off that we don't want Chatzilla as part of the installer?
Sorry, meant to add the reference to Bug 383154 – Seamonkey (suite) NO option to install individual components only.
Blocks: 383154
Yes, fixing this bug will ultimately allow the SeaMonkey installer to give the option of installing or not installing ChatZilla to the user.
With the landing of bug 397246, the last piece of this should be fixed now.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 409490
(In reply to comment #35)
> Yes, fixing this bug will ultimately allow the SeaMonkey installer to give the
> option of installing or not installing ChatZilla to the user.
> 

(In reply to comment #36)
> With the landing of bug 397246, the last piece of this should be fixed now.
> 

Not seeing the option to not install it. Is it there yet or what? Maybe I'm missing it. Thanks.


(In reply to comment #37)
> (In reply to comment #35)
> > Yes, fixing this bug will ultimately allow the SeaMonkey installer to give the
> > option of installing or not installing ChatZilla to the user.
> > 
> 
> (In reply to comment #36)
> > With the landing of bug 397246, the last piece of this should be fixed now.
> > 
> 
> Not seeing the option to not install it. Is it there yet or what? Maybe I'm
> missing it. Thanks.

Try reading comment #35 again - fixing this bug will *ultimately* allow... I need to do some bug reorg later so will check if we have got one filed that covers CZ being optional or not.
I filed bug 409490 yesterday for exactly what Worcester12345 wants to have.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: