Enable the optional use of ICU in Mozilla

RESOLVED WONTFIX

Status

()

Core
Build Config
RESOLVED WONTFIX
12 years ago
10 years ago

People

(Reporter: Samphan Raruenrom, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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:
(Reporter)

Comment 1

12 years ago
Created attachment 187898 [details] [diff] [review]
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.
Attachment #187898 - Flags: review?(jshin1987)
(Reporter)

Updated

12 years ago
Blocks: 7969
(Reporter)

Comment 2

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

Comment 3

12 years ago
looks like a BSD-style license

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: PC → All
How would this work linkage-wise on windows?

Comment 5

12 years ago
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.
Attachment #187898 - Flags: review?(jshin1987) → review-
(Reporter)

Comment 6

12 years ago
Created attachment 188756 [details] [diff] [review]
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; }
(Reporter)

Updated

12 years ago
Attachment #187898 - Attachment is obsolete: true
Attachment #188756 - Flags: review?(cls)
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?
(Reporter)

Comment 8

12 years ago
(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.
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).
(Reporter)

Comment 10

12 years ago
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.

Updated

11 years ago
Attachment #188756 - Flags: review?(cls)
(Reporter)

Updated

10 years ago
No longer blocks: 7969

Comment 11

10 years ago
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 ?
(Reporter)

Comment 12

10 years ago
Agree
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.