Closed Bug 465339 Opened 12 years ago Closed 8 years ago

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

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: standard8, Assigned: standard8)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
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.
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?
Attached patch Possible fix (obsolete) — Splinter Review
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
Attached patch The fixSplinter Review
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 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
Closed: 8 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.