Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Build Config
--
critical
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({regression})

Trunk
Thunderbird 19.0
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

9 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

base::
	$(MAKE) -C base
(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.
How about ?

default all alldep check:: base
.PHONY: base

base:
    $(MAKE) -C base
(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

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

check::
    $(MAKE) -C base $@
Mark, still critical severity? wontfix per c#1's workaround, or what?
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:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=67a290dd4140
Assignee: nobody → mbanner
Status: NEW → ASSIGNED
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

5 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+
Checked in with the additional include:

https://hg.mozilla.org/comm-central/rev/5fb3a7d5847c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
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.