824330 - Enable services in xulrunner and don't build under services/ when building against a libxul SDK

Status: RESOLVED FIXED

(Toolkit Graveyard :: XULRunner, defect)

Description

[Mike Hommey [:glandium]]

When building Firefox as a xulrunner application, services are built and installed in the browser part of the build, which, since bug 817076, is wrong since services are now meant to be in the GRE.
They should thus be built in xulrunner, and not be built when building an app against a libxul SDK.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Enable services in xulrunner and don't build under services/ when building against a libxul SDK

Comment 2

[Mike Hommey [:glandium]]

Attachment: Build services/ during the platform tier and enable services in xulrunner

r? blassey for the mobile/android part. Interestingly, while some MOZ_SERVICES_* variables are set in confvars.sh, we were never building under services/ for mobile, so I'm disabling them to match this behavior. However, if the intent of the variables in confvars.sh was to actually enable these services components, then it failed to do so, and we should keep them enabled.

Comment 3

[Benjamin Smedberg]

Comment on attachment 695328 [details] [diff] [review]
Build services/ during the platform tier and enable services in xulrunner

gps should verify that this won't have ill effects on other apps (that the services code is all off-by-default and won't tbird, for example, if it's not enabled by default already).

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 695328 [details] [diff] [review]
Build services/ during the platform tier and enable services in xulrunner

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

I /think/ this should be safe.

/services is weird. The higher-level components/services really are kinda half platform half app. Sync in particular is definitely tightly coupled with browser. Getting it to work on other apps would likely be difficult in its current form. Other things like services/common and services/metrics are definitely more platform-y.

::: services/crypto/Makefile.in
@@ +21,5 @@
>  INSTALL_TARGETS += CRYPTO_MODULE
>  
>  TEST_DIRS += tests
>  
> +DIRS += component

component is already included as part of toolkit-tiers.mk. Why, I have no clue. I suspect it has to do with build order and requiring NSS. But, that doesn't make much sense since app is always built after platform. Who knows.

::: xulrunner/confvars.sh
@@ +15,5 @@
> +MOZ_SERVICES_AITC=1
> +MOZ_SERVICES_COMMON=1
> +MOZ_SERVICES_CRYPTO=1
> +MOZ_SERVICES_METRICS=1
> +MOZ_SERVICES_NOTIFICATIONS=1

MOZ_SERVICES_NOTIFICATIONS is deprecated and should not be carried forward. The only reason it is still enabled in other apps is laziness.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Comment on attachment 695328 [details] [diff] [review]
> Build services/ during the platform tier and enable services in xulrunner
> 
> Review of attachment 695328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I /think/ this should be safe.
> 
> /services is weird. The higher-level components/services really are kinda
> half platform half app. Sync in particular is definitely tightly coupled
> with browser. Getting it to work on other apps would likely be difficult in
> its current form. Other things like services/common and services/metrics are
> definitely more platform-y.
> 
> ::: services/crypto/Makefile.in
> @@ +21,5 @@
> >  INSTALL_TARGETS += CRYPTO_MODULE
> >  
> >  TEST_DIRS += tests
> >  
> > +DIRS += component
> 
> component is already included as part of toolkit-tiers.mk. Why, I have no
> clue. I suspect it has to do with build order and requiring NSS. But, that
> doesn't make much sense since app is always built after platform. Who knows.

The patch changes that, because there's no reason not to
-tier_platform_dirs += services/crypto/component
+tier_platform_dirs += services

As for why, it's because the component was already part of the GRE, contrary to the other modules. But now everything is in the GRE, so it's kind of common-sense that everything is built in the platform tier.

> ::: xulrunner/confvars.sh
> @@ +15,5 @@
> > +MOZ_SERVICES_AITC=1
> > +MOZ_SERVICES_COMMON=1
> > +MOZ_SERVICES_CRYPTO=1
> > +MOZ_SERVICES_METRICS=1
> > +MOZ_SERVICES_NOTIFICATIONS=1
> 
> MOZ_SERVICES_NOTIFICATIONS is deprecated and should not be carried forward.
> The only reason it is still enabled in other apps is laziness.

Let's keep that to a followup, then.

Comment 6

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6656cdc8b20d

Comment 7

[Mike Hommey [:glandium]]

Backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1a01e16ce7

So in fact, the problem was that the services-crypto component is added unconditionally in toolkit/library, and used to be built unconditionally too. The patch changed that by making it built from services/crypto, and it became conditional to MOZ_SERVICES_CRYPTO. Now, the question is, is there any reason to build and link the services-crypto component unconditionally?

Comment 8

[Gregory Szorc [:gps]]

services-crypto is only used as part of Sync AFAIK. That being said, it does provide some generic crypto primitives (J-PAKE and boxed encryption) that others may find useful and thus have started to rely on. I'd be fine with nuking the MOZ_SERVICES_CRYPTO variable and building it unconditionally. The same goes for MOZ_SERVICES_COMMON.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Build services/ during the platform tier and enable services in xulrunner.

This does this on top of the previous patch:

diff --git a/services/Makefile.in b/services/Makefile.in
--- a/services/Makefile.in
+++ b/services/Makefile.in
@@ -4,23 +4,19 @@
 
 DEPTH     = @DEPTH@
 topsrcdir = @top_srcdir@
 srcdir    = @srcdir@
 VPATH     = @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
-ifdef MOZ_SERVICES_COMMON
 PARALLEL_DIRS += common
-endif
 
-ifdef MOZ_SERVICES_CRYPTO
 PARALLEL_DIRS += crypto
-endif
 
 ifdef MOZ_SERVICES_AITC
 PARALLEL_DIRS += aitc
 endif
 
 ifdef MOZ_SERVICES_HEALTHREPORT
 PARALLEL_DIRS += healthreport
 endif

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 698016 [details] [diff] [review]
Build services/ during the platform tier and enable services in xulrunner.

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

r+ conditional on not removing MOZ_SERVICES_METRICS. Maybe it was splinter. Meh.

::: mobile/android/confvars.sh
@@ -19,5 @@
>  # Enable getUserMedia
>  MOZ_MEDIA_NAVIGATOR=1
>  
> -MOZ_SERVICES_COMMON=1
> -MOZ_SERVICES_METRICS=1

Why did you remove MOZ_SERVICES_METRICS?

Comment 11

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Why did you remove MOZ_SERVICES_METRICS?

see comment 2

Comment 12

[Gregory Szorc [:gps]]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Gregory Szorc [:gps] from comment #10)
> > Why did you remove MOZ_SERVICES_METRICS?
> 
> see comment 2

Do what you need to do in this bug and I'll sort it out later (possibly as early as this weekend).

Comment 13

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27860b6b264d

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/27860b6b264d

Comment 15

[Zane U. Ji]

I can't compile SeaMonkey on windows 7 after I pulled the latest code. I'm not sure if anybody else has such problem:
...
make.py[4]: Entering directory 'd:\mozilla-build\objsmdbg\mozilla'
export_tier_platform
make.py[5]: Entering directory 'd:\mozilla-build\objsmdbg\mozilla'
Target 'services/Makefile' has multiple rules with commands.
d:\mozilla-build\src\mozilla\config\makefiles\target_export.mk:17:0: command 'd:/mozilla-build/python/python.exe d:/mozilla-build/src/mozilla/build/pymake/pymake/../make.py xpcom/Makefile modules/libpref/Makefile intl/Makefile netwerk/Makefile extensions/auth/Makefile media/libjpeg/Makefile modules/libbz2/Makefile other-licenses/bsdiff/Makefile gfx/qcms/Makefile ipc/Makefile js/ipc/Makefile hal/Makefile js/xpconnect/Makefile intl/chardet/Makefile modules/libjar/Makefile storage/Makefile extensions/cookie/Makefile extensions/permissions/Makefile rdf/Makefile js/jsd/Makefile media/libvorbis/Makefile media/libopus/Makefile media/libnestegg/Makefile media/libvpx/Makefile media/libogg/Makefile media/libtheora/Makefile media/libsydneyaudio/Makefile security/build/Makefile media/webrtc/Makefile media/mtransport/third_party/Makefile media/mtransport/build/Makefile media/mtransport/standalone/Makefile media/libspeex_resampler/Makefile media/libsoundtouch/Makefile media/libcubeb/Makefile media/libpng/Makefile testing/specialpowers/Makefile uriloader/Makefile caps/Makefile parser/Makefile gfx/Makefile image/Makefile dom/Makefile view/Makefile widget/Makefile content/Makefile editor/Makefile layout/Makefile docshell/Makefile embedding/Makefile xpfe/appshell/Makefile extensions/universalchardet/Makefile accessible/Makefile profile/Makefile tools/profiler/Makefile xpfe/components/Makefile extensions/spellcheck/Makefile security/manager/Makefile modules/libmar/Makefile toolkit/Makefile extensions/pref/Makefile services/Makefile startupcache/Makefile js/ductwork/debugger/Makefile other-licenses/snappy/Makefile ./../mozilla/xpfe/components/autocomplete/Makefile ./../ldap/xpcom/Makefile ./../db/mork/Makefile ./../mailnews/Makefile toolkit/library/Makefile xpcom/stub/Makefile testing/mochitest/Makefile testing/xpcshell/Makefile testing/tools/screenshot/Makefile testing/peptest/Makefile testing/mozbase/Makefile media/webrtc/signaling/test/Makefile media/mtransport/test/Makefile' failed, return code 2
d:\mozilla-build\src\mozilla\config\rules.mk:608:0: command 'd:/mozilla-build/python/python.exe d:/mozilla-build/src/mozilla/build/pymake/pymake/../make.py export_tier_platform' failed, return code 2

Comment 16

[Gregory Szorc [:gps]]

(In reply to Zane U. Ji from comment #15)

Apply the patch in bug 827303. That should be making its way to m-c and aurora shortly.

Comment 17

[Zane U. Ji]

(In reply to Gregory Szorc [:gps] from comment #16)
> (In reply to Zane U. Ji from comment #15)
> 
> Apply the patch in bug 827303. That should be making its way to m-c and
> aurora shortly.

I'm sorry to say that it didn't help. Yet I have no problem building ThunderBird and everything seems to work on Ubuntu 12.04.

Comment 18

[Mike Hommey [:glandium]]

Remove "tier_app_dirs += services" from suite/build.mk.

Comment 19

[Zane U. Ji]

(In reply to Mike Hommey [:glandium] from comment #18)
> Remove "tier_app_dirs += services" from suite/build.mk.

Works like a charm.