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

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[Mark Banner (:standard8)]

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

mailnews/base
mailnews/base/public
mailnews/base/src
mailnews/base/util

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>/build.mk 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

[[e-mail]]

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

base::
	$(MAKE) -C base

Comment 2

[Mark Banner (:standard8)]

(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/Makefile.in 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.

Comment 3

[Philippe M. Chiasson (:gozer)]

How about ?

default all alldep check:: base
.PHONY: base

base:
    $(MAKE) -C base

Comment 4

[Mark Banner (:standard8)]

(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

[[e-mail]]

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

check::
    $(MAKE) -C base $@

Comment 6

[Justin Wood (:Callek)]

Mark, still critical severity? wontfix per c#1's workaround, or what?

Comment 7

[Mark Banner (:standard8)]

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

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=67a290dd4140

Comment 8

[Mark Banner (:standard8)]

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

Comment 9

[[e-mail]]

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

Comment 10

[Mark Banner (:standard8)]

Checked in with the additional include:

https://hg.mozilla.org/comm-central/rev/5fb3a7d5847c

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 681094 [details] [diff] [review]
The fix

Agreed on irc we didn't need the additional review.