Find a sensible way around building in mailnews when touching mailnews/base, or document it everywhere

RESOLVED FIXED in Thunderbird 19.0


10 years ago
6 years ago


(Reporter: standard8, Assigned: standard8)



Thunderbird 19.0

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



10 years ago
Bug 463385 changed our mailnews build system, when you touch items in one of:


you must now rebuild in mailnews/base *and* mailnews.

This is annoying, more so that it wasn't publicised and isn't obvious. I've rejected a couple of reviews on not knowing this change had been made.

The problem is that mailnews/base is needed to be built before the rest of the mailnews directories - due to the way our libs system works. So bug 463385 put the mailnews/base into <app>/ alongside mailnews/, hence the change in funtionality, but also allowing most of the mailnews directories to be run parallel when running make.

I've posted to the newsgroups, but that's not a long-term solution. I think we either need to fix this (maybe a pre-PARALLEL_DIRS option?), or make it known in the build documentation.

Comment 1

10 years ago
An alternative workaround before this bug is fixed is to use make -C $(OBJDIR) tier_app (you can also use libs_tier_app if you haven't changed any exports).

Would these rules help at all?

default all alldep:: base

.PHONY:: base

	$(MAKE) -C base

Comment 2

10 years ago
(In reply to comment #1)
> Would these rules help at all?
> default all alldep:: base
> .PHONY:: base
> base::
>     $(MAKE) -C base

If I put them at the start of mailnews/ and change the double colon on .PHONY and base targets to a single colon, then it works for building mailnews.

It doesn't work for make -C mailnews check.
How about ?

default all alldep check:: base
.PHONY: base

    $(MAKE) -C base

Comment 4

10 years ago
(In reply to comment #3)
> How about ?
> default all alldep check:: base
> .PHONY: base
> base:
>     $(MAKE) -C base

I tried that, but it did a "make -C base" before running the tests in the rest of mailnews. Need to pass that information onto the command somehow.

Comment 5

10 years ago
You'd probably want a separate rule for check, perhaps:

    $(MAKE) -C base $@
Mark, still critical severity? wontfix per c#1's workaround, or what?

Comment 7

6 years ago
Created attachment 680994 [details] [diff] [review]
Possible fix

Now that we have XPIDL_FLAGS in the makefiles, plus we got rid of the separate build/ directories ages ago, I think we can actually remove the requirement to build mailnews/base and then mailnews.

This patch works for me fine locally with a -j4 build. I've also done various other tests to see if each directory should work standalone for the export stage, and I think it will.

Worst case, any issues should be covered by extending XPIDL_FLAGS for the directory that's failing.

Note there's also some rework around MailNewsTypes2.idl includes - the xpidl parser isn't clever enough to recognise duplicate includes, so I just needed to simplify the includes down.

This is now on try server here:
Assignee: nobody → mbanner

Comment 8

6 years ago
Created attachment 681094 [details] [diff] [review]
The fix

This is pretty much the same as before, but has some more includes fixing madness. I've run it through try server on all platforms at least a couple of times and it seems to be green.

As I said before, in my previous comment, I think any additional issues should be easy to fix.

Of course, the advantages are that developers will just have to build mailnews/ it also means that we can have more parallelism.
Attachment #680994 - Attachment is obsolete: true
Attachment #681094 - Flags: review?(neil)
Attachment #681094 - Flags: review?(bugspam.Callek)

Comment 9

6 years ago
Comment on attachment 681094 [details] [diff] [review]
The fix

>diff --git a/mailnews/base/search/public/nsMsgSearchAdapter.h b/mailnews/base/search/public/nsMsgSearchAdapter.h
>--- a/mailnews/base/search/public/nsMsgSearchAdapter.h
>+++ b/mailnews/base/search/public/nsMsgSearchAdapter.h
>@@ -7,7 +7,7 @@
> #define _nsMsgSearchAdapter_H_
> #include "nsMsgSearchCore.h"
>+#include "nsStringGlue.h"
> #include "nsIMsgSearchAdapter.h"
> #include "nsIMsgSearchValidityTable.h"
> #include "nsIMsgSearchValidityManager.h"
I couldn't work out why but nsMsgSearchAdapter.h stopped compiling in external API builds because of a missing nsCOMPtr.h include. r=me if you add one. (I haven't had time to compile an internal API build but I expect it works.)
Attachment #681094 - Flags: review?(neil) → review+

Comment 10

6 years ago
Checked in with the additional include:
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0

Comment 11

6 years ago
Comment on attachment 681094 [details] [diff] [review]
The fix

Agreed on irc we didn't need the additional review.
Attachment #681094 - Flags: review?(bugspam.Callek)
You need to log in before you can comment on or make changes to this bug.