Closed Bug 329021 Opened 15 years ago Closed 13 years ago

Get non-static builds of mailnews working when MOZ_XUL_APP set

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: neil)

References

Details

Attachments

(7 files)

As per the summary. SeaMonkey needs non-static builds of mailnews (i.e. those that don't define MOZ_STATIC_MAIL_BUILD) working when MOZ_XUL_APP set.

The changes shouldn't affect Thunderbird (though some MOZ_THUNDERBIRD defs may change to MOZ_XUL_APP).
This patch has the initial changes for non-static builds of mailnews. Basically we're defining the initialisation routines for the command line handlers using the toolkit version rather than the old xpfe versions.

Comments welcome, especially about where to put the static functions.
Comment on attachment 213659 [details] [diff] [review]
Main build changes for mailnews

Ok, looks like this part will be needed in early MOZ_XUL_APP enabling, so requesting review.
Attachment #213659 - Flags: review?(mnyromyr)
Comment on attachment 213659 [details] [diff] [review]
Main build changes for mailnews

How does the toolkit command line handler work? Might it be easier to #ifdef MOZ_XUL_APP nsICmdLineHandler.idl to generate the right code?
Comment on attachment 213659 [details] [diff] [review]
Main build changes for mailnews

This has brought up various issues with respect to startup and command lines that we need to resolve before we do this patch. Also there may be a better way to do it.
Attachment #213659 - Flags: review?(mnyromyr)
The current idea for this bug is to turn on the MOZ_STATIC_MAIL_BUILD flag for mailnews (for SeaMonkey) when we turn on the MOZ_XUL_APP flag. This will mean we don't have to do code changes and allow SeaMonkey to pick up some of the advantages of static mailnews builds (faster execution for one).

Mnyromyr was commenting on irc that it would be good to leave MOZ_STATIC_MAIL_BUILD OFF for debug builds, to make them faster to build. I'm going to leave that open for debate, as with a fast-ish computer, I don't mind either way, but would prefer not to do the extra work at the moment for it.
We should get rid of the nonstatic mail build completely: it is not a good idea to have to maintain two sets of component registration manifests, and it allows signifucant simplification in terms of linkage.
overall I think allowing the non-static way is a *good* thing for faster build times on any type that wants it. (eg. Debug).

I currently --disable-static for my *personal* opt builds for that reason, makes dep builds much faster to build.

I will be willing to 'accept' either option chosen of course, (mainly as I do not know enough to actually do the work involved)
(In reply to comment #7)
> I will be willing to 'accept' either option chosen of course, (mainly as I do
> not know enough to actually do the work involved)

I'm not planning to do any work on this myself, so reassigning to default owner. The work that would be required would be to examine nsMailModule.cpp (used for static builds and other files used for non-static builds (e.g. nsAbFactory.cpp) and implement the required changes - mainly the command line handling code I believe.
Assignee: bugzilla → nobody
QA Contact: backend
I would very much like to only have the static-mail build in all configurations, to reduce the maintenance headache of the mailutils exports and other crap.
Obviously you also have to delete MOZ_STATIC_MAIL_BUILD from suite/confvars.sh

Although this builds I'm not guaranteeing that it runs but it's a start :-)
Attachment #275769 - Flags: review?(mnyromyr)
Neil commented that with his patch it would build but may not run correctly... I've been playing around and I've just found that nsAbLDAPDirectoryQuery is missing from nsAbFactory.

Here's the fix.
Attachment #294736 - Flags: superreview?(neil)
Attachment #294736 - Flags: review?(neil)
Comment on attachment 294736 [details] [diff] [review]
[checked in] Add nsAbLDAPDirectoryQuery to AB components

So the rest of attachment 275769 [details] [diff] [review] is OK then?
Attachment #294736 - Flags: superreview?(neil)
Attachment #294736 - Flags: superreview+
Attachment #294736 - Flags: review?(neil)
Attachment #294736 - Flags: review+
(In reply to comment #12)
> (From update of attachment 294736 [details] [diff] [review])
> So the rest of attachment 275769 [details] [diff] [review] is OK then?

I have only really tested the address book parts and the mailnews/base/util parts. Note that the diff for addrbook/build/nsAbFactory.cpp is now obsolete. The rest of it looks good though I haven't actually tested it.
(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 294736 [details] [diff] [review] [details])
> > So the rest of attachment 275769 [details] [diff] [review] [details] is OK then?
> 
> I have only really tested the address book parts and the mailnews/base/util
> parts. Note that the diff for addrbook/build/nsAbFactory.cpp is now obsolete.
> The rest of it looks good though I haven't actually tested it.

I've just found that ./seamonkey -addressbook doesn't work. We're missing something like: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/build/nsMailModule.cpp&rev=1.48&mark=608-653#608 for the individual libs.
Attachment #294736 - Attachment description: Add nsAbLDAPDirectoryQuery to AB components → [checked in] Add nsAbLDAPDirectoryQuery to AB components
This fixes the startup error you were seeing; the shutdown service is a new component that landed after attachment 275769 [details] [diff] [review] was generated.
Attachment #296766 - Flags: review?(mnyromyr)
Attachment #296766 - Flags: review?(mnyromyr) → review+
Comment on attachment 275769 [details] [diff] [review]
Fix compile and link errors

I toyed around with this a bit and basically it looks good.

There are just two issues which need to be solved in followup patches:
- the commandline handlers "-mail", "-news", "-compose" and "-addressbook" don't work
- confvars.sh sets static as the default, but we only have --enable-static-mail, so users can't override that in their mozconfig easily. We either should change confvars.sh and do dynamic by default or replace/complement --enable-static-mail.
Attachment #275769 - Flags: review?(mnyromyr) → review+
I don't think we should have non-static by default, but it might be a good idea to make static mail set as a default and dynamic as a default when debug is enabled - or set static as default and --disable-static-mail the config option.
In theory once this patch lands --disable-static-mail should just work.
Note: This was deliberately diffed against the previous versions in order to make it clearer what's going on. I'll attach an up-to-date patch for review.
Assignee: nobody → neil
Status: NEW → ASSIGNED
The reordering of includes is to match nsMailModule.cpp (Sorry, but I didn't think to check against it when I added the include previously...)
Attachment #297431 - Flags: review?(bugzilla)
Comment on attachment 297431 [details] [diff] [review]
Implement command-line handlers

r=me, though I haven't fully tested it, it all looks ok.
Attachment #297431 - Flags: review?(bugzilla) → review+
This should fix non-static mail builds.
Attachment #301880 - Flags: review?(neil)
(In reply to comment #22)
> Created an attachment (id=301880) [details]
> Link in macmorefiles
> This should fix non-static mail builds.

So why aren't these included in mailnews/build/Makefile.in if they are required for compose?
Comment on attachment 301880 [details] [diff] [review]
Link in macmorefiles

I guess it might be more accurate to say r=Mnyromyr sr=me ;-)

Standard8, feel free to chime in with better ideas!
Attachment #301880 - Flags: review?(neil) → review+
Checking in Makefile.in;
/cvsroot/mozilla/mailnews/compose/build/Makefile.in,v  <--  Makefile.in
new revision: 1.56; previous revision: 1.55
done
(In reply to comment #24)
> (From update of attachment 301880 [details] [diff] [review])
> I guess it might be more accurate to say r=Mnyromyr sr=me ;-)
> Standard8, feel free to chime in with better ideas!

*shrug*
(In reply to comment #26)
>*shrug*

Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.