Closed Bug 824330 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit Graveyard :: XULRunner, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla20

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Attachment #695328 - Flags: review?(blassey.bugs)
Attachment #695328 - Flags: review?(benjamin)
Attachment #695291 - Attachment is obsolete: true
Attachment #695291 - Flags: review?(benjamin)
Attachment #695328 - Flags: review?(blassey.bugs) → review+
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).
Attachment #695328 - Flags: review?(gps)
Attachment #695328 - Flags: review?(benjamin)
Attachment #695328 - Flags: review+
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.
Attachment #695328 - Flags: review?(gps) → review+
(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.
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?
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.
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
Attachment #698016 - Flags: review?(gps)
Attachment #695328 - Attachment is obsolete: true
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?
Attachment #698016 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #10)
> Why did you remove MOZ_SERVICES_METRICS?

see comment 2
(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).
https://hg.mozilla.org/mozilla-central/rev/27860b6b264d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 827303
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
(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.
(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.
Remove "tier_app_dirs += services" from suite/build.mk.
(In reply to Mike Hommey [:glandium] from comment #18)
> Remove "tier_app_dirs += services" from suite/build.mk.

Works like a charm.
Blocks: 828957
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.