707305 - Re-enable building with --enable-incomplete-external-linkage

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Description

[[e-mail]]

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

[[e-mail]]

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

[[e-mail]]

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

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

[[e-mail]]

(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)]

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

[[e-mail]]

(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

[[e-mail]]

Attachment: Alternative patch

This version does away with INCLUDED_BRIDGE_MK completely.

Comment 8

[Mark Banner (:standard8)]

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 [:jhorak]]

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

[[e-mail]]

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 [:jhorak]]

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

[[e-mail]]

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

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

...sorry for the very delayed review

Comment 15

[[e-mail]]

Comment on attachment 590575 [details] [diff] [review]
Alternative patch

Pushed changeset b2a30f2ca5e9 to comm-central.

Comment 16

[Mark Banner (:standard8)]

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

[[e-mail]]

(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

[[e-mail]]

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 [:jhorak]]

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

[[e-mail]]

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 [:jhorak]]

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

[[e-mail]]

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 [:jhorak]]

Attachment: 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 [:jhorak]]

Setting checkin-needed for attachment: https://bugzilla.mozilla.org/attachment.cgi?id=624360

Comment 25

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/comm-central/rev/ee459e007a9c

Comment 26

[Magnus Melin [:mkmelin]]

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

[[e-mail]]

(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 [:mkmelin]]

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

[[e-mail]]

(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 [:mkmelin]]

Mistyped ac_add_options...