851992 - Provide --with-system-icu build option

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Norbert Lindenberg]

The initial implementation of the ECMAScript Internationalization API is based on a bundled ICU. Distributors will likely want a --with-system-icu option to use a copy of ICU that's already in the OS.

Comment 1

[Norbert Lindenberg]

Note from Jeff Walden, bug 724531 comment 29:

Assuming someone implements --with-system-icu=, as seems likely, they might need to deal with allocator mismatching for NumberingSystem, in the "part 4" patch in bug 837957, if I understand our allocator-hooking correctly:

<Waldo> does our malloc hooking in Gecko also properly hook new/delete?  does it hook either in system libraries, should they be used?
<Waldo> in particular, do things work right if there's a |delete foo| in Gecko, but the corresponding |new Foo| was in a system library?

Comment 2

[Jeff Walden [:Waldo]]

Further patches in bug 837957 (part 7 at least) have a minimum version requirement of ICU 50.1, so that should also be enforced when compiling against a system ICU.

Comment 3

[Norbert Lindenberg]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Further patches in bug 837957 (part 7 at least) have a minimum version
> requirement of ICU 50.1, so that should also be enforced when compiling
> against a system ICU.

Actually, that part has a comment indicating how it can be built with earlier versions of ICU. I do recommend, however, using a current version of ICU in order to get up-to-date locale data and in particular time zone data.

Comment 4

[Norbert Lindenberg]

Another issue that an implementation of --with-system-icu does have to watch out for is binary compatibility:
http://userguide.icu-project.org/design#TOC-ICU-Binary-Compatibility:-Using-ICU-as-an-Operating-System-Level-Library

At this point, Intl.cpp does make use of some C++ API in the intl_numberingSystem function. ICU bug http://bugs.icu-project.org/trac/ticket/10039 requests a C API for this functionality. Implementations that use system ICUs that don't have that C API will have to change intl_numberingSystem. One possible workaround is to format a known number (0) in the desired locale and check the resulting string against the known numbering systems:
0 -> latn
\u0660 -> arab
\u0E50 -> thai
etc.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to Norbert Lindenberg from comment #3)
> Actually, that part has a comment indicating how it can be built with
> earlier versions of ICU.

Too much trouble.  :-)  Plus there's the outdated-ness concern, and security concerns from using older ICU.  We require at-least-version for other optional system dependencies like NSPR and NSS.  It's totally reasonable to do the same here.

Comment 6

[Landry Breuil (:gaston)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> (In reply to Norbert Lindenberg from comment #3)
> > Actually, that part has a comment indicating how it can be built with
> > earlier versions of ICU.
> 
> Too much trouble.  :-)  Plus there's the outdated-ness concern, and security
> concerns from using older ICU.  We require at-least-version for other
> optional system dependencies like NSPR and NSS.  It's totally reasonable to
> do the same here.

Yes, as long as you don't update & bump the dependency version too often :) As a distributor building with systemwide nspr/nss/sqlite, it's sometimes quite hard to follow the pace of updates & dependency on the very latest sqlite...

Comment 7

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 768224 [details] [diff] [review]
fix

using pkconfig

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 768224 [details] [diff] [review]
fix

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

::: configure.in
@@ +4147,5 @@
> +                          Use system icu (located with pkgconfig)],
> +    MOZ_NATIVE_ICU=1)
> +
> +if test -n "$MOZ_NATIVE_ICU"; then
> +    PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1)

I wonder what will happen when building against icu >= 51.

::: js/src/Makefile.in
@@ +259,5 @@
>    ICU_LIB_RENAME = $(foreach libname,$(ICU_LIB_NAMES),\
>                       cp -p intl/icu/lib/s$(libname)$(ICU_LIB_SUFFIX).lib intl/icu/lib/$(libname).lib;)
>  endif
>  
> +ifndef MOZ_NATIVE_ICU

Please move this under the ifdef ENABLE_INTL_API above.

Comment 10

[Landry Breuil (:gaston)]

(In reply to Mike Hommey [:glandium] from comment #9)
> > +if test -n "$MOZ_NATIVE_ICU"; then
> > +    PKG_CHECK_MODULES(MOZ_ICU, icu-i18n >= 50.1)
> 
> I wonder what will happen when building against icu >= 51.

We have 51.1 systemwide here so i can give it a shot...

Comment 11

[Landry Breuil (:gaston)]

Comment on attachment 768224 [details] [diff] [review]
fix

Fwiw, m-c builds & runs fine here using --with-system-icu, with 51.2 as systemwide icu4c. I had to manually merge configure.in's first chunk though.

Comment 12

[Landry Breuil (:gaston)]

Should this land or not ? Build is broken on BSDs now after bug 853301, and that bug could temporarly help, if unbitrotted..

Comment 13

[Jan Beich]

Attachment: fix (unbitrotten)

I still need this, at least until bug 899722.

Comment 14

[Ian Stakenvicius]

For the upcoming spidermonkey-24 release, Gentoo Linux would really like this to both land and be backported if possible.....  

FYI, latest patch (except for js/src/configure.in, probably due to the update for de-bitrotting) applies clean to beta.

Comment 15

[Gregory Szorc [:gps]]

ICU isn't enabled in 24, right? So why would we need to backport this?

Landing on the other hand...

Why isn't this patch up for review? Please flag :glandium for it when it's ready.

Comment 16

[Ian Stakenvicius]

(In reply to Gregory Szorc [:gps] from comment #15)
> ICU isn't enabled in 24, right? So why would we need to backport this?

It's enabled by default in js/src when JS_STANDALONE (at least, this was true according to beta a couple of days ago), which makes a difference for the spidermonkey-24 package.

Will flag, thx!

Comment 17

[Landry Breuil (:gaston)]

The unbitrotten patch wasnt r? because the previous version (ie att 768244) was already r+'ed by glandium..

And it's indeed enabled by default on beta for JS_STANDALONE, cf http://mxr.mozilla.org/mozilla-beta/source/js/src/configure.in#4413

Comment 18

[Ian Stakenvicius]

Try is green:  https://tbpl.mozilla.org/?tree=Try&rev=903973d4ca18  -- if glandium approves the unbitrotten patch this should be good to land?

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 791058 [details] [diff] [review]
fix (unbitrotten)

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

::: config/system-headers
@@ +1140,5 @@
> +unicode/udat.h
> +unicode/udatpg.h
> +unicode/uenum.h
> +unicode/unum.h
> +unicode/ustring.h

missing unicode/utypes.h

::: js/src/Makefile.in
@@ +393,5 @@
>  DEFINES += -DUSE_ZLIB
>  endif
>  
> +ifdef MOZ_NATIVE_ICU
> +EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS)

EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS) should be enough for both cases.

Comment 20

[Jan Beich]

Attachment: fix, v2

(In reply to Mike Hommey [:glandium] from comment #19)
> ::: config/system-headers
> @@ +1140,5 @@
> > +unicode/udat.h
> > +unicode/udatpg.h
> > +unicode/uenum.h
> > +unicode/unum.h
> > +unicode/ustring.h
> 
> missing unicode/utypes.h

For completeness sake unicode/uclean.h should be there, too.

> ::: js/src/Makefile.in
> @@ +393,5 @@
> >  DEFINES += -DUSE_ZLIB
> >  endif
> >  
> > +ifdef MOZ_NATIVE_ICU
> > +EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS)
> 
> EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS) should be enough for both cases.

Unless you mean something else removing SHARED_LIBRARY_LIBS breaks
js/src/shell build with internal ICU due to missing symbols in
libjs_static.a.

Comment 21

[Ian Stakenvicius]

(In reply to Jan Beich from comment #20)
> > ::: js/src/Makefile.in
> > @@ +393,5 @@
> > >  DEFINES += -DUSE_ZLIB
> > >  endif
> > >  
> > > +ifdef MOZ_NATIVE_ICU
> > > +EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS)
> > 
> > EXTRA_DSO_LDOPTS += $(MOZ_ICU_LIBS) should be enough for both cases.
> 
> Unless you mean something else removing SHARED_LIBRARY_LIBS breaks
> js/src/shell build with internal ICU due to missing symbols in
> libjs_static.a.

I think he means that the 'ifdef MOZ_NATIVE_ICU' isn't needed because EXTRA_DSO_LDOPTS will just get an empty set appended to it when ifundef MOZ_NATIVE_ICU.  (i haven't checked what else happens within this ifdef or if there's an else block)

Comment 22

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd5bc19306c

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6bd5bc19306c

Comment 24

[darkxst]

Attachment: backport to esr24 branch

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 830524 [details] [diff] [review]
backport to esr24 branch

Sorry I didn't come to this in time. That's an unfortunate consequence of me using http://harthur.github.io/bugzilla-todos/ for so long, and it not showing review requests on closed bugs.