Last Comment Bug 707305 - Re-enable building with --enable-incomplete-external-linkage
: Re-enable building with --enable-incomplete-external-linkage
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 452232
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-02 13:46 PST by neil@parkwaycc.co.uk
Modified: 2012-06-15 13:51 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (6.34 KB, patch)
2011-12-02 13:59 PST, neil@parkwaycc.co.uk
bugspam.Callek: review-
Details | Diff | Review
m-c patch to build autocomplete with external linkage (700 bytes, patch)
2011-12-02 15:17 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Review
Alternative patch (7.06 KB, patch)
2012-01-22 09:18 PST, neil@parkwaycc.co.uk
bugspam.Callek: review+
Details | Diff | Review
Fix linking and enable building components for external linkages (3.97 KB, patch)
2012-05-03 08:20 PDT, Jan Horak
standard8: review+
neil: review-
Details | Diff | Review
Fix linking and enable building components for external linkages v2 (7.00 KB, patch)
2012-05-10 05:06 PDT, Jan Horak
neil: review+
Details | Diff | Review
Fix linking and enable building components for external linkages v3 (7.13 KB, patch)
2012-05-15 05:26 PDT, Jan Horak
no flags Details | Diff | Review
Fix linking and enable building components for external linkages v4 (7.18 KB, patch)
2012-05-16 06:12 PDT, Jan Horak
jhorak: review+
Details | Diff | Review

Description neil@parkwaycc.co.uk 2011-12-02 13:46:35 PST
Although it's possible (or at least, nearly possible) to build with both --enable-incomplete-external-linkage and --with-libxul-sdk it's not currently possible to build with just --enable-incomplete-external-linkage.
Comment 1 neil@parkwaycc.co.uk 2011-12-02 13:59:10 PST
Created attachment 578712 [details] [diff] [review]
Proposed patch

* Makefile.in change needed because pymake (dunno about gmake, but it tends to be more lenient about this sort of thing) doesn't like the same Makefiles being listed twice in SUBMAKEFILES
* bridge.mk change not strictly necessary
* configure change exports the variable so that confvars.sh can find it
* build.mk now only sets APP_LIBXUL_*DIRS for fat libxul builds
* build.mk adds APP_LIBXUL_*DIRS to top-level tier_app_*dirs for external API builds so that it builds after libxul (not a problem with libxul-sdk builds).
* Leading ./ removed from dirs because pymake wouldn't make makefiles...
* confvars.sh doesn't set the libxul variables for external API builds.
Comment 2 neil@parkwaycc.co.uk 2011-12-02 15:17:25 PST
Created attachment 578744 [details] [diff] [review]
m-c patch to build autocomplete with external linkage

In case you don't want to try out the patch that switches ldap autocomplete.
Comment 3 Justin Wood (:Callek) 2012-01-17 04:57:22 PST
Comment on attachment 578712 [details] [diff] [review]
Proposed patch

Review of attachment 578712 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +65,4 @@
>  
>  # workaround Bug 599809 by making these makefiles be generated here
>  SUBMAKEFILES += $(addsuffix /Makefile, $(APP_LIBXUL_DIRS) $(APP_LIBXUL_STATICDIRS))
> +endif

This change worries me in and of itself, I don't see easily how these values get added to SUBMAKEFILES twice here. Before I review the rest or r+ this I want that answer.
Comment 4 neil@parkwaycc.co.uk 2012-01-17 07:19:55 PST
(In reply to Justin Wood from comment #3)
> (From update of attachment 578712 [details] [diff] [review])
> > -endif
> >  
> >  # workaround Bug 599809 by making these makefiles be generated here
> >  SUBMAKEFILES += $(addsuffix /Makefile, $(APP_LIBXUL_DIRS) $(APP_LIBXUL_STATICDIRS))
> > +endif
> This change worries me in and of itself, I don't see easily how these values
> get added to SUBMAKEFILES twice here. Before I review the rest or r+ this I
> want that answer.
--enable-incomplete-external-linkage disables the libxul frankenbuild system, so that the app libxul dirs get built in the normal way via tier_app_dirs, which config.mk copies into DIRS, which rules.mk copies into SUBMAKEFILES.
Comment 5 Justin Wood (:Callek) 2012-01-17 07:48:24 PST
Comment on attachment 578712 [details] [diff] [review]
Proposed patch

Review of attachment 578712 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile.in
@@ +65,4 @@
>  
>  # workaround Bug 599809 by making these makefiles be generated here
>  SUBMAKEFILES += $(addsuffix /Makefile, $(APP_LIBXUL_DIRS) $(APP_LIBXUL_STATICDIRS))
> +endif

Given what you said in your response, lets do a separate wrapping of ifndef MOZ_INCOMPLETE_EXTERNAL_LINKAGE here

ifndef INCLUDED_BRIDGE_MK alone is not the solution.

::: bridge/bridge.mk
@@ +47,2 @@
>  APP_LIBXUL_DIRS += $(DEPTH)$(SUBDIR)/db/mork
> +endif

Not a fan of these bridge.mk changes, both us and TB won't have either of these in MOZ_APP_COMPONENT_LIBS if the related MOZ_* var is not present.

::: mail/confvars.sh
@@ +48,5 @@
>  MOZ_COMPOSER=1
>  MOZ_SAFE_BROWSING=1
>  MOZ_MORK=1
>  MOZ_STATIC_BUILD_UNSUPPORTED=1
> +if test -z "$MOZ_INCOMPLETE_EXTERNAL_LINKAGE"; then

This won't do it right here, we source confvars.sh before we set MOZ_INCOMPLETE_EXTERNAL_LINKAGE. and thus your test will always be true.

::: suite/build.mk
@@ +36,5 @@
>  # ***** END LICENSE BLOCK *****
>  
>  ifndef COMM_BUILD # Mozilla Makefile
>  
> +ifdef MOZ_APP_COMPONENT_LIBS

Which means this in the build.mk's will not do what you think it does.
Comment 6 neil@parkwaycc.co.uk 2012-01-17 08:07:03 PST
(In reply to Justin Wood from comment #5)
> (From update of attachment 578712 [details] [diff] [review])
> > -endif
> >  
> >  # workaround Bug 599809 by making these makefiles be generated here
> >  SUBMAKEFILES += $(addsuffix /Makefile, $(APP_LIBXUL_DIRS) $(APP_LIBXUL_STATICDIRS))
> > +endif
> Given what you said in your response, lets do a separate wrapping of ifndef
> MOZ_INCOMPLETE_EXTERNAL_LINKAGE here
> 
> ifndef INCLUDED_BRIDGE_MK alone is not the solution.
It's actually a leftover from when --disable-libxul used to work, which also needed this change to allow building with pymake ;-)

> > -#ifdef MOZ_LDAP_XPCOM
> > +ifneq (,$(MOZ_LDAP_XPCOM)$(filter mozldap,$(MOZ_APP_COMPONENT_LIBS)))
> >  APP_LIBXUL_STATICDIRS += $(DEPTH)$(SUBDIR)/ldap/sdks/c-sdk
> >  APP_LIBXUL_DIRS += $(DEPTH)$(SUBDIR)/ldap/xpcom
> > -#endif
> > +endif
> Not a fan of these bridge.mk changes, both us and TB won't have either of
> these in MOZ_APP_COMPONENT_LIBS if the related MOZ_* var is not present.
In fatlibxul builds, MOZ_APP_COMPONENT_LIBS will be present, see below. In external linkage builds, this is built from comm-central's Makefile, so MOZ_LDAP_XPCOM will be present instead (assuming you didn't disable ldap.)

> This won't do it right here, we source confvars.sh before we set
> MOZ_INCOMPLETE_EXTERNAL_LINKAGE. and thus your test will always be true.
Only in comm-central's configure. In mozilla-central's configure MOZ_INCOMPLETE_EXTERNAL_LINKAGE will have been exported from comm-central and the test will then be false. (The variable is only needed for fatlibxul.)

> >  ifndef COMM_BUILD # Mozilla Makefile
> >  
> > +ifdef MOZ_APP_COMPONENT_LIBS
> Which means this in the build.mk's will not do what you think it does.
But this is the Mozilla Makefile, so it does do what I think it does.
Comment 7 neil@parkwaycc.co.uk 2012-01-22 09:18:57 PST
Created attachment 590575 [details] [diff] [review]
Alternative patch

This version does away with INCLUDED_BRIDGE_MK completely.
Comment 8 Mark Banner (:standard8) 2012-01-27 01:50:22 PST
Comment on attachment 578712 [details] [diff] [review]
Proposed patch

I think I'm happy with either, although the alternative patch does seem slightly cleaner.
Comment 9 Jan Horak 2012-04-30 06:04:56 PDT
How to deal with namespaces:
../src/nsImportMail.o: In function `mozilla::services::GetStringBundleService()':
/home/jhorak/mozilla/5comm-central/obj-x86_64-unknown-linux-gnu/mailnews/import/src/../../../mozilla/dist/include/mozilla/ServiceList.h:10: undefined reference to `mozilla::services::_external_GetStringBundleService()'
../../base/util/nsMsgUtils.o: In function `mozilla::services::GetIOService()':
/home/jhorak/mozilla/5comm-central/obj-x86_64-unknown-linux-gnu/mailnews/base/util/../../../mozilla/dist/include/mozilla/ServiceList.h:8: undefined reference to `mozilla::services::_external_GetIOService()'
?
Comment 10 neil@parkwaycc.co.uk 2012-04-30 07:06:11 PDT
These are symbols exported from libxul (rather than the xpcom glue, as was traditional) as part of bug 714967 (without which we wouldn't have been able to link to them at all). We've never imported symbols directly from libxul before but I don't think this should be a problem, just that my patch pre-dates this work so the build won't actually complete these days without some tweaks.
Comment 11 Jan Horak 2012-05-03 08:20:41 PDT
Created attachment 620709 [details] [diff] [review]
Fix linking and enable building components for external linkages

By using Neil's patches and this patch I'm able to build thunderbird with --enable-incomplete-external-linkage again. I had to also apply patch 'up to date patch' from bug 452232 to make that happen.
Not sure who to ask about review.
Comment 12 neil@parkwaycc.co.uk 2012-05-03 08:29:04 PDT
Comment on attachment 620709 [details] [diff] [review]
Fix linking and enable building components for external linkages

Picking a reviewer for mail/Makefile.in ...
Comment 13 Justin Wood (:Callek) 2012-05-06 10:17:34 PDT
Comment on attachment 590575 [details] [diff] [review]
Alternative patch

Review of attachment 590575 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like it will not regress the current build system, after detailed looking, and *should* be a big step to making external linkage an option again.

Please do a test compile in the default configuration before landing (try-comm-central is ok) and watch for any weird test breakage (especially TB tests).

I know this (currently) is not enough to make external linkage fully functional, but would be good to test a compile in that configuration anyway, and be sure that we are only failing in final linkage relating to mozilla::services.
Comment 14 Justin Wood (:Callek) 2012-05-06 10:17:49 PDT
...sorry for the very delayed review
Comment 15 neil@parkwaycc.co.uk 2012-05-08 16:06:57 PDT
Comment on attachment 590575 [details] [diff] [review]
Alternative patch

Pushed changeset b2a30f2ca5e9 to comm-central.
Comment 16 Mark Banner (:standard8) 2012-05-09 04:23:46 PDT
Comment on attachment 620709 [details] [diff] [review]
Fix linking and enable building components for external linkages

r=me for the Makefile.in changes in mailnews. The mail/ one is no longer needed.

Not sure if we want to do something like http://hg.mozilla.org/comm-central/annotate/134579274546/mailnews/import/text/src/nsTextAddress.cpp#l177 for CountChar replacement, so at least we're consistent everywhere, so redirecting r for that to Neil.
Comment 17 neil@parkwaycc.co.uk 2012-05-09 06:29:07 PDT
(In reply to Mark Banner from comment #16)
> r=me for the Makefile.in changes in mailnews. The mail/ one is no longer needed.
Yeah, I checked it in by mistake. Sorry about that!

> Not sure if we want to do something like
> http://hg.mozilla.org/comm-central/annotate/134579274546/mailnews/import/
> text/src/nsTextAddress.cpp#l177 for CountChar replacement, so at least we're
> consistent everywhere, so redirecting r for that to Neil.
The difference is that import is interested in counting quotes as it reads lines, so that it only splits on an even number of quotes, while this code is only really interested in finding a unique colon, and one of the calls actually wants to know where it is, thus duplicating effort with the old code. On the other hand, the use of CountChar is probably more readable, although this could possibly be alleviated with suitable comments. And I've now spent so much time thinking about this that I've also come up with the following option:
if (colonPos != kNotFound && colonPos == hostname.RFindChar(':'))
Comment 18 neil@parkwaycc.co.uk 2012-05-09 06:33:29 PDT
Comment on attachment 620709 [details] [diff] [review]
Fix linking and enable building components for external linkages

>+  if (colonPos != -1 && hostname.FindChar(':', colonPos+1) == -1)
I've convinced myself that this version isn't readable enough (even if you had used kNotFound instead of -1 and put spaces around the +es) so I would suggest the best course of action is to add MsgCountChar to nsMsgUtils (a simple #define would do in the internal API case) and use it here and in import.
Comment 19 Jan Horak 2012-05-10 05:06:08 PDT
Created attachment 622690 [details] [diff] [review]
Fix linking and enable building components for external linkages v2

Thanks for reviewing guys, changes:
- Added MsgCountChar function to nsMsgUtils.
- Replaced some Left(), Right() calls by Substring.
Comment 20 neil@parkwaycc.co.uk 2012-05-10 05:53:22 PDT
Comment on attachment 622690 [details] [diff] [review]
Fix linking and enable building components for external linkages v2

My external tree is a few days out of date, but I hope to get to this soon.

>   nsString acctPart;
>   if (!hostnameChanged && (atPos != kNotFound))
>   {
>     // ...if username changed and the previous username was equal to the part
>     // of the account name before @
>-    acctName.Left(acctPart, atPos);
>+    acctPart = Substring(acctName, 0, atPos);
Nit: StringHead(acctName, atPos)

>     if (acctPart.Equals(NS_ConvertASCIItoUTF16(userName)))
... and you could substitute the expression for the temporary at this point...

>-    acctName.Right(acctPart, acctName.Length() - atPos);
>+    acctPart = Substring(acctName, atPos);
>     if (acctPart.Equals(NS_ConvertASCIItoUTF16(hostName))) {
... and here too.
Comment 21 Jan Horak 2012-05-15 05:26:23 PDT
Created attachment 624002 [details] [diff] [review]
Fix linking and enable building components for external linkages v3

> ... and you could substitute the expression for the temporary at this point...
You mean something like this (in attachment).
Comment 22 neil@parkwaycc.co.uk 2012-05-15 05:29:23 PDT
Comment on attachment 624002 [details] [diff] [review]
Fix linking and enable building components for external linkages v3

Nearer, but you only use each temporary once, so you don't really need it.
Comment 23 Jan Horak 2012-05-16 06:12:55 PDT
Created attachment 624360 [details] [diff] [review]
Fix linking and enable building components for external linkages v4

Attaching patch after clarifying it on irc, setting review+ from previous positive from Neil.
Comment 24 Jan Horak 2012-05-30 02:18:20 PDT
Setting checkin-needed for attachment: https://bugzilla.mozilla.org/attachment.cgi?id=624360
Comment 25 Mike Conley (:mconley) - (Away until June 29th) 2012-06-02 11:56:24 PDT
https://hg.mozilla.org/comm-central/rev/ee459e007a9c
Comment 26 Magnus Melin 2012-06-15 04:47:07 PDT
So it should now be possible to build without relinking libxul? Only --enable-incomplete-external-linkage doesn't build for me. Do i need --with-libxul-sdk, --enable-incomplete-toolkit-ldap-autocomplete or some other options too?
Comment 27 neil@parkwaycc.co.uk 2012-06-15 05:01:47 PDT
(In reply to Magnus Melin from comment #26)
> So it should now be possible to build without relinking libxul? Only
> --enable-incomplete-external-linkage doesn't build for me. Do i need
> --with-libxul-sdk, --enable-incomplete-toolkit-ldap-autocomplete or some
> other options too?

As of this bug, --with-libxul-sdk is now optional. However, you may find that you need to use --enable-incomplete-toolkit-ldap-autocomplete or apply attachment 578744 [details] [diff] [review]. Also you can't yet build Thunderbird on Windows this way, it needs an (unfiled) mozilla-central change to export an nsLookAndFeel symbol.
Comment 28 Magnus Melin 2012-06-15 11:51:59 PDT
Doesn't seem to build for me. 

ac_add_options --enable-incomplete-external-linkage
ad_add_options --enable-incomplete-toolkit-ldap-autocomplete

../addrbook/src/nsLDAPAutoCompleteSession.o: In function `nsQueryElementAt':
/opt/moz-objdir/mail/mailnews/addrbook/src/../../../mozilla/dist/include/nsICollection.h:188: undefined reference to `vtable for nsQueryElementAt'
/usr/bin/ld: ../addrbook/src/nsLDAPAutoCompleteSession.o: relocation R_X86_64_PC32 against undefined hidden symbol `vtable for nsQueryElementAt' can not be used when making a shared object
/usr/bin/ld: final link failed: Bad value

Suggestions appreciated as i'd really like to get this building, i suspect time savings could be huge in the end...
Comment 29 neil@parkwaycc.co.uk 2012-06-15 11:58:17 PDT
(In reply to Magnus Melin from comment #28)
> Doesn't seem to build for me. 
> 
> ac_add_options --enable-incomplete-external-linkage
> ad_add_options --enable-incomplete-toolkit-ldap-autocomplete
> 
> ../addrbook/src/nsLDAPAutoCompleteSession.o
Well there's your problem. mailnews/addrbook/src/Makefile.in should be protecting that file with a MOZ_INCOMPLETE_TOOLKIT_LDAP_AUTOCOMPLETE ifndef.
Comment 30 Magnus Melin 2012-06-15 13:51:36 PDT
Mistyped ac_add_options...

Note You need to log in before you can comment on or make changes to this bug.