Get non-static builds of mailnews working when MOZ_XUL_APP set

RESOLVED FIXED

Status

defect
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: standard8, Assigned: neil)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Reporter

Description

14 years ago
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).
Reporter

Comment 1

14 years ago
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.
Reporter

Comment 2

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

Comment 3

14 years ago
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?
Reporter

Comment 4

14 years ago
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)
Reporter

Comment 5

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

Comment 6

13 years ago
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)
Reporter

Comment 8

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

Comment 9

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

Comment 10

12 years ago
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)
Reporter

Comment 11

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

Comment 12

12 years ago
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+
Reporter

Comment 13

12 years ago
(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.
Reporter

Comment 14

12 years ago
(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.
Reporter

Updated

12 years ago
Attachment #294736 - Attachment description: Add nsAbLDAPDirectoryQuery to AB components → [checked in] Add nsAbLDAPDirectoryQuery to AB components
Assignee

Comment 15

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

Updated

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

Comment 17

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

Comment 18

12 years ago
In theory once this patch lands --disable-static-mail should just work.
Assignee

Comment 19

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

Comment 20

12 years ago
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)
Reporter

Comment 21

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

Updated

12 years ago
Attachment #301880 - Flags: review?(neil)
Reporter

Comment 23

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

Comment 24

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

Comment 26

12 years ago
(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*
Assignee

Comment 27

12 years ago
(In reply to comment #26)
>*shrug*

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