299324 - Enable the optional use of ICU in Mozilla

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Samphan Raruenrom]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

ICU (http://icu.sourceforge.net/) is an internationalization library developed
by IBM. A Thai localized build require ICU for line-breaking, see bug #7969.
Other languages or i18n tasks may find uses of ICU in Mozilla.
Since Mozilla doesn't has plan to include ICU as an internal component. There
should be an interim solution to allow optional use of external ICU (using
#ifdef ENABLE_ICU) in the source.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Samphan Raruenrom]

Attachment: Patch to add --with-icu[=prefix] to configure.in to enable optional use of external ICU

This patch add --with-icu[=prefix] to configure.in. Add a line like this :-
ac_add_options --with-icu=/usr/local
or
ac_add_options --with-icu=c:/opt/icu
To .mozconfig to enable compiling/linking with ICU at the location. Prefix is
optional, if missing, ICU must be in system include/library path (e.g.
/usr/include, /usr/lib).
It will link statically with ICU if --enable-static and dynamically if
--enable-shared. For dynamic linking, LD_LIBRARY_PATH or PATH must be set
appropriately for configure to pass.
The patch add 4 Makefile symbols :- ICU_CFLAGS, ICU_CXXFLAGS, ICU_LIBS and
ENABLE_ICU. ICU_CCLAGS/ICU_CXXCLAGS must be (optionally) added to
CFLAGS/CXXFLAGS for the source directory that use ICU. ICU_LIBS must be
(optionally) added to EXTRA_LIBS for the source directory that produce DLL that
use ICU. ENABLE_ICU can be used in Makefiles and source files to detect the use
of --with-icu flag. #ifdef ENABLE_ICU in source files allow optional use of ICU
depend on the presence of --with-icu flag. See Thai patch in bug #7969.

Comment 2

[Samphan Raruenrom]

This is tested for Firefox on Windows/MSVC and Linux. You have to run autoconf
after applying the patch.

Comment 3

[Jo Hermans]

looks like a BSD-style license

Comment 4

[Benjamin Smedberg]

How would this work linkage-wise on windows?

Comment 5

[cls]

Comment on attachment 187898 [details] [diff] [review]
Patch to add --with-icu[=prefix] to configure.in to enable optional use of external ICU

A bunch of minor nits:
* Put the description for --with-icu on the same line a sthe option
* win32 msys builds don't have cygpath.  you'll need an explicit $host_os check
for mingw.
* use - instead of / for command options (msys)
* Afaict, there's no need to wrap the test args in ()s
* --disable-shared/ --enable-static applies to internal Mozilla libs, not
external dependencies so we shouldn't add the NODEFAULTLIB:libc.lib for win32
* Cross-compile builds will fail with TRY_RUN and it doesn't look like it's a
run-time test at all.  Use TRY_LINK instead.

Comment 6

[Samphan Raruenrom]

Attachment: New patch to add --with-icu[=prefix]

- work with msys
- use TRY_LINK
- I don't understand why but I can't get rid of the -NODEFAULTLIB:libc.lib
option for win32 static link test or the link will fail like this :-

configure:16191: cl -o conftest -Ic:/opt/iculite/include
-DU_STATIC_IMPLEMENTATION -Ic:/opt/iculite/include -DU_STATIC_IMPLEMENTATION 
-TC -nologo -W3 -nologo -Gy -Fd$(PDBFILE)   conftest.C -link
-LIBPATH:c:/opt/iculite/lib sicuin.lib sicuio.lib sicule.lib siculx.lib
sicuuc.lib sicudt.lib  kernel32.lib user32.lib gdi32.lib winmm.lib wsock32.lib
advapi32.lib 1>&5
conftest.C
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _strncpy already defined in
LIBC.lib(strncpy.obj)
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _memmove already defined in
LIBC.lib(memmove.obj)
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _strncmp already defined in
LIBC.lib(strncmp.obj)
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _malloc already defined in
LIBC.lib(malloc.obj)
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _realloc already defined in
LIBC.lib(realloc.obj)
MSVCRT.lib(MSVCR71.dll) : error LNK2005: _free already defined in
LIBC.lib(free.obj)
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _strncpy already defined in
LIBC.lib(strncpy.obj); second definition ignored
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _memmove already defined in
LIBC.lib(memmove.obj); second definition ignored
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _strncmp already defined in
LIBC.lib(strncmp.obj); second definition ignored
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _malloc already defined in
LIBC.lib(malloc.obj); second definition ignored
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _realloc already defined in
LIBC.lib(realloc.obj); second definition ignored
MSVCRT.lib(MSVCR71.dll) : warning LNK4006: _free already defined in
LIBC.lib(free.obj); second definition ignored
   Creating library conftest.lib and object conftest.exp
LINK : warning LNK4098: defaultlib 'MSVCRT' conflicts with use of other libs;
use /NODEFAULTLIB:library
conftest.exe : fatal error LNK1169: one or more multiply defined symbols found
configure: failed program was:
#line 16184 "configure"
#include "confdefs.h"
#include <unicode/uloc.h>
int main() {
const char *loc = uloc_getDefault();
; return 0; }

Comment 7

[Benjamin Smedberg]

Samphan, before posting lots of patches could you please answer my questions
about how ICU is going to be linked on various platforms? Are you using a stable
C API or a C++ API (which by nature of the language and linking cannot be
considered stable for our purposes). Do you plan to link dynamically or
statically against ICU? Do you plan to ship the ICU binaries with any or all of
our platform binaries?

Comment 8

[Samphan Raruenrom]

(In reply to comment #7)
> Samphan, before posting lots of patches could you please answer my questions
> about how ICU is going to be linked on various platforms? 

It depends on how the Mozilla app is built. If it is built with --enable-shared
then ICU will be dynamically linked. If it is built with --enable-static then
ICU will be statically linked.

> Are you using a stable
> C API or a C++ API (which by nature of the language and linking cannot be
> considered stable for our purposes).

I myself use C++ API in the patch in bug #7969.

> Do you plan to link dynamically or
> statically against ICU? Do you plan to ship the ICU binaries with any or all of
> our platform binaries?

I think statically link ICU with Mozilla apps should be easiest for packaging. I
did so in the Thai Firefox Community Edition.

Comment 9

[Benjamin Smedberg]

ok. I suppose I am ok with static linking for the time being, though I think we
should really see what the codesize affects are before committing to anything.
You don't mean to use the --enable-static/--enable-shared flags, those are very
different and affect how mozilla links its own internal component libraries.
Probably we should just use whatever kind of lib is available at the --with-icu
location (.a if linking statically, .so if linking dynamically).

Comment 10

[Samphan Raruenrom]

I reduce the size of ICU to only allow the use of DictionaryBasedBreakIterator
that I use for Thai line-breaking. When statically-linked, the size increase for
firefox.exe on Win32 is 528k.

Comment 11

[Jean-Marc Desperrier]

Thai line-breaking is now done using Pango under linux, Uniscribe under windows, and ATSUI under Mac OS. There is no plan to include ICU anymore.

Shall we close this as WONTFIX ?

Comment 12

[Samphan Raruenrom]

Agree