63083 - mailnews makefiles use EXPORTS instead of INCLUDES

Status: RESOLVED WORKSFORME

(MailNews Core :: Build Config, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 1

[cls]

Sorry about the spam. An unset priority should be less than P5.  

Comment 2

[Mark Banner (:standard8)]

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.

Comment 3

[Mark Banner (:standard8)]

Opps, sorry for the spam, selected the wrong option, reassigning to me.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part v1 (checked in)

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/).

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Looks reasonable to me.

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Benjamin Smedberg]

Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

beautiful

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

sr=dmose

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 192962 [details] [diff] [review]
Part v1 (checked in)

I've just checked this first patch in.

Comment 10

[Mark Banner (:standard8)]

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. 

Comment 11

[Mark Banner (:standard8)]

Attachment: Part 2 (mailnews/import) v1 (checked in)

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.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 196426 [details] [diff] [review]
Part 2 (mailnews/import) v1 (checked in)

sr=dmose

Comment 13

[Mark Banner (:standard8)]

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).

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 3 (mailnews/news) (checked in)

This one sorts out most of mailnews/news (except for the CID header file).

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 202005 [details] [diff] [review]
Part 3 (mailnews/news) (checked in)

r+sr=dmose; thanks for the patch!

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 202005 [details] [diff] [review]
Part 3 (mailnews/news) (checked in)

I checked this in earlier.

Comment 17

[Mark Banner (:standard8)]

Attachment: 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.

Comment 18

[Mark Banner (:standard8)]

Attachment: Part 4 (move *CID.h) (checked in)

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).

Comment 19

[Dan Mosedale (:dmosedale, :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.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 204071 [details] [diff] [review]
Part 4 (move *CID.h) (checked in)

Patch checked in and all header files moved ok.

Comment 21

[Mark Banner (:standard8)]

Attachment: Part 5 (mailnews/local and mailnews/mapi) (checked in, compilation item addressed)

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.

Comment 22

[David :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;

Comment 23

[Mark Banner (:standard8)]

Attachment: Part 6 (some of mailnews/compose and mailnews/mime) (checked in)

Comment 24

[David :Bienvenu]

Comment on attachment 213235 [details] [diff] [review]
Part 6 (some of mailnews/compose and mailnews/mime) (checked in)

oops

Comment 25

[Mark Banner (:standard8)]

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.

Comment 26

[Serge Gautherie (:sgautherie)]

I support this bug, but I'm not going to do it.

Comment 27

[Mark Banner (:standard8)]

EXPORTS died a while ago.