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
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?
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
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 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.
You need to log in before you can comment on or make changes to this bug.