Closed Bug 63083 Opened 22 years ago Closed 8 years ago

mailnews makefiles use EXPORTS instead of INCLUDES

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: dbaron, Unassigned)

References

Details

Attachments

(7 files)

This is a Mailnews/Build Config bug, really, so filing on Browser/Build Config
and assigning to owner of Mailnews/Networking-IMAP since I happen to be looking
in the mailnews/imap/src/ makefiles, although the problem seems to extend
through other directories in mailnews.

Mailnews makefiles in, for example, mailnews/imap/src/ differ strongly between
platforms in a way that makes build bustage easy to create.  The MANIFEST in
that directory exports only one file (should it export any?), but the windows
and linux makefiles there export lots of files.  These files are, presumably,
used in other directories within the same module.  It is better to use INCLUDES
in the directories where those files need to be included (see makefiles in
layout for examples).  This will avoid accidentally creating inter-module
dependencies on a Windowss or Linux build that cause Mac builds to break when
checked in.

On a somewhat related note, the MANIFEST in mailnews/base/src/ seems to export
lots of implementation header files.  Should these really be completely public?
Sorry about the spam. An unset priority should be less than P5.  
Priority: -- → P5
Product: Browser → Seamonkey
Following up on the work done so far in the address book in bug 131849 to not
export the internal implementation files, I'd now like to look at progressing
this on mailnews as this will help to tidy up the mailnews build system. Dan has
already said that the idea in principle is good, and as this bug is here it was
commented on a long time ago.

The only possible issue I can currently see is that the Thunderbird build system
currently includes everything in nsMailModule.cpp to build up the components
lists. However that can be solved by the addition of relevant directories to
LOCAL_INCLUDES in the same way address book is already in there.
Assignee: mscott → nobody
Priority: P5 → --
QA Contact: granrosebugs → build-config
Opps, sorry for the spam, selected the wrong option, reassigning to me.
Assignee: nobody → bugzilla
This is a proof of concept type patch to show what would be needed to move a
large number of unnecessary EXPORTS from the makefiles. I haven't tested with a
full clobber build, but a dep (which trashes the dist/includes directory) works
fine so far on both SeaMonkey & Thunderbird.

From the way mailnews is currently structured, all the headers in
mailnews/base/util are currently exported, they are used in various places. We
will either need to look at rehoming this, or just agree to leave them where
they are.

Also, there are many *CID.h files in the src/ directories which are required to
obtain classes from the factory. Either we leave them as they are & export
them, or we move them to public/ directories (FWIW most of them in mailnews
were originally in build/).
Looks reasonable to me.
Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

I'm going with this patch as-is. I've tested Thunderbird & SeaMonkey with dep &
clobber builds on linux & no problems. There is a mac specific change but I've
examined that in lxr & against the patch and I don't think it will cause any
problems.

Requesting r from benjamin for the build config side and sr from dmose for the
mailnews side.

Further patches will be necessary to finish tidying this all up.
Attachment #192962 - Flags: superreview?(dmose)
Attachment #192962 - Flags: review?(benjamin)
Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

beautiful
Attachment #192962 - Flags: review?(benjamin) → review+
Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

sr=dmose
Attachment #192962 - Flags: superreview?(dmose) → superreview+
Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

I've just checked this first patch in.
Attachment #192962 - Attachment description: Patch v1 → Part v1 (checked in)
Two changes I've done to fix windows bustage:

1) Change #include "nsMsgCompFields.h" to #include "nsIMsgCompFields.h"        
                                          in msgMapiImp.cpp

2) Add the export of nsMsgComposeStringBundle.h back into
mailnews/compose/src/Makefile.in

The first of these isn't an issue, but the second one I'll address in a future
patch. 
Status: NEW → ASSIGNED
This patch removes all the EXPORTS from mailnews/import directories & below,
and does LOCAL_INCLUDES where they are needed (actually just in
mailnews/import/build). This one is a Thunderbird-only patch (effectively) as
the EXPORTS were set up when we did the static build options for mailnews.

Tested with a clean build on Linux & it works fine. Checked through via lxr and
can't see any reason why Windows/Mac shouldn't work. Will try and get someone
to test it on one of those platforms before checkin.
Attachment #196426 - Flags: superreview?(dmose)
Attachment #196426 - Flags: review?(benjamin)
Comment on attachment 196426 [details] [diff] [review]
Part 2 (mailnews/import) v1 (checked in)

sr=dmose
Attachment #196426 - Flags: superreview?(dmose) → superreview+
Attachment #196426 - Flags: review?(benjamin) → review+
Comment on attachment 196426 [details] [diff] [review]
Part 2 (mailnews/import) v1 (checked in)

I checked this in with one follow up build bustage fix to
mailnews/import/build/Makefile.in

where I'd managed to put -I$(NULL) instead of just $(NULL).
Attachment #196426 - Attachment description: Part 2 (mailnews/import) v1 → Part 2 (mailnews/import) v1 (checked in)
This one sorts out most of mailnews/news (except for the CID header file).
Attachment #202005 - Flags: superreview?(dmose)
Attachment #202005 - Flags: review?(dmose)
Comment on attachment 202005 [details] [diff] [review]
Part 3 (mailnews/news) (checked in)

r+sr=dmose; thanks for the patch!
Attachment #202005 - Flags: superreview?(dmose)
Attachment #202005 - Flags: superreview+
Attachment #202005 - Flags: review?(dmose)
Attachment #202005 - Flags: review+
Comment on attachment 202005 [details] [diff] [review]
Part 3 (mailnews/news) (checked in)

I checked this in earlier.
Attachment #202005 - Attachment description: Part 3 (mailnews/news) → Part 3 (mailnews/news) (checked in)
Attached file Part 4 (cvs moves)
This is the cvs moves for the part 4 patch (I will break this off into another bug once part 4 is attached & reviewed). Added to this bug for clarity.
This patch (and the associated cvsmoves file) moves the mailnews/.../src/*CID.h to mailnews/.../public/*.CID.h where the .h file is required by other modules. Where it isn't (e.g. some of the ones under mailnews/extensions) then the file is left where it is and LOCAL_INCLUDES set up instead.

This is slightly different from what I said to Dan on IRC (I said I would move all of them to public).
Attachment #204071 - Flags: superreview?(dmose)
Attachment #204071 - Flags: review?(dmose)
Comment on attachment 204071 [details] [diff] [review]
Part 4 (move *CID.h) (checked in)

r+sr=dmose; the changes you made seem like a good choice to me.
Attachment #204071 - Flags: superreview?(dmose)
Attachment #204071 - Flags: superreview+
Attachment #204071 - Flags: review?(dmose)
Attachment #204071 - Flags: review+
Depends on: 317849
Comment on attachment 204071 [details] [diff] [review]
Part 4 (move *CID.h) (checked in)

Patch checked in and all header files moved ok.
Attachment #204071 - Attachment description: Part 4 (move *CID.h) → Part 4 (move *CID.h) (checked in)
Stop exporting internal headers from mailnews/local/src and mailnews/mapi/... also remove some unused includes.

David, from what I can tell in lxr this should still compile on Windows, but could you check it for me please? I've tested the mailnews/local/src bit and that's fine.
Attachment #211905 - Flags: superreview?(bienvenu)
Attachment #211905 - Flags: review?(bienvenu)
Comment on attachment 211905 [details] [diff] [review]
Part 5 (mailnews/local and mailnews/mapi) (checked in, compilation item addressed)

Mark, your patch compiles on Windows with this one additional change, nsnull -> NULL in mapi/mapiDLL/MapiDll.cpp


@@ -115,17 +114,17 @@ BOOL InitMozillaReference(nsIMapi **aRet
     *aRetValue = (nsIMapi *)TlsGetValue(tId);

     // Check whether the pointer actually resolves to
     // a valid method call; otherwise mozilla is not running

     if ((*aRetValue) && (*aRetValue)->IsValid() == S_OK)
          return TRUE;

-    HRESULT hRes = ::CoInitialize(nsnull) ;
+    HRESULT hRes = ::CoInitialize(NULL) ;

     hRes = ::CoCreateInstance(CLSID_CMapiImp, NULL, CLSCTX_LOCAL_SERVER,
                                          IID_nsIMapi, (LPVOID *)aRetValue);

     if (hRes == S_OK && (*aRetValue)->Initialize() == S_OK)
         if (TlsSetValue(tId, (LPVOID)(*aRetValue)))
             return TRUE;
Attachment #211905 - Flags: superreview?(bienvenu)
Attachment #211905 - Flags: superreview+
Attachment #211905 - Flags: review?(bienvenu)
Attachment #211905 - Flags: review+
Attachment #211905 - Attachment description: Part 5 (mailnews/local and mailnews/mapi) → Part 5 (mailnews/local and mailnews/mapi) (checked in, compilation item addressed)
Attachment #213235 - Flags: superreview?(bienvenu)
Attachment #213235 - Flags: superreview-
Attachment #213235 - Flags: review?(bienvenu)
Attachment #213235 - Flags: review+
Comment on attachment 213235 [details] [diff] [review]
Part 6 (some of mailnews/compose and mailnews/mime) (checked in)

oops
Attachment #213235 - Flags: superreview- → superreview+
Attachment #213235 - Attachment description: Part 6 (some of mailnews/compose and mailnews/mime) → Part 6 (some of mailnews/compose and mailnews/mime) (checked in)
There are a few more makefiles that need correcting, but I haven't got time/inclination to change them at the moment. Reassigning to default owner for someone else to pick up.
Assignee: bugzilla → nobody
Status: ASSIGNED → NEW
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
Attachment #204064 - Attachment is patch: false
I support this bug, but I'm not going to do it.
EXPORTS died a while ago.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.