Provide --with-system-icu build option

RESOLVED FIXED in mozilla26

Status

enhancement
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: mozillabugs, Assigned: m_kato)

Tracking

(Blocks 1 bug)

Trunk
mozilla26
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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?
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.
(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.
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.
(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.
(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...
Blocks: es-intl
Assignee: nobody → m_kato
Posted patch fix (obsolete) — Splinter Review
Comment on attachment 768224 [details] [diff] [review]
fix

using pkconfig
Attachment #768224 - Flags: review?(mh+mozilla)
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.
Attachment #768224 - Flags: review?(mh+mozilla) → review+
(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 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.
Attachment #768224 - Flags: feedback+
Should this land or not ? Build is broken on BSDs now after bug 853301, and that bug could temporarly help, if unbitrotted..
Posted patch fix (unbitrotten) (obsolete) — Splinter Review
I still need this, at least until bug 899722.
Attachment #768224 - Attachment is obsolete: true
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.
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.
Flags: needinfo?(jbeich)
(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!
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
Attachment #791058 - Flags: review?(mh+mozilla)
Try is green:  https://tbpl.mozilla.org/?tree=Try&rev=903973d4ca18  -- if glandium approves the unbitrotten patch this should be good to land?
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.
Attachment #791058 - Flags: review?(mh+mozilla) → review+
Posted patch fix, v2Splinter Review
(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.
Attachment #791058 - Attachment is obsolete: true
Attachment #795103 - Flags: review?(mh+mozilla)
Flags: needinfo?(jbeich)
(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)
Attachment #795103 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/6bd5bc19306c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Attachment #830524 - Flags: review?(mh+mozilla)
Depends on: 966559
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.
Attachment #830524 - Flags: review?(mh+mozilla)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.