WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
22713
Confusing/repetitive onboard unicode sources
https://bugs.webkit.org/show_bug.cgi?id=22713
Summary
Confusing/repetitive onboard unicode sources
Daniel Macks
Reported
2008-12-06 10:51:53 PST
Building
r39007
on OS X 10.4 with fink installed for gtk dependencies, I noticed during autogen: checking the Unicode backend to use... icu and then during compiling, flags for internal icu sources are passed (-I./JavaScriptCore/icu -I./WebCore/icu). Now wait, I have libicu headers and a "libicu-config" available on my system, why is webkit using what sounds like *its own* icu lib? Looking at configure.ac, I see why, at line 237:
>if test "$os_darwin" = "yes"; then > UNICODE_CFLAGS="-I\$(srcdir)/JavaScriptCore/icu -I\$(srcdir)/WebCore/icu" > UNICODE_LIBS="-licucore"
[...]
>else > AC_PATH_PROG(icu_config, icu-config, no)
[...]
> UNICODE_CFLAGS=`$icu_config --cppflags` > UNICODE_LIBS=`$icu_config --ldflags` >fi
It hardcodes to use the internal one on darwin even if there is an actual ("normal for unixish platforms":) libicu available. Seems like the primary test should be for availablity of icu-config, and then only fall back on platform-dependent guessing if it's not found. Secondarily, while looking at the included libicu, I see three different sets of libicu headers in the webkit sources: JavaScriptCore/icu, JavaScriptGlue/icu, and WebCore/icu, all annotated that they are for OS X 10.4. So first, that's hella-redundant:) Can't all the components share *one* libicu? But also, it's not consistent with configure.ac, which appears to trigger their use on *all* OS X versions.
Attachments
Always check for icu-config first
(1.60 KB, patch)
2008-12-06 12:51 PST
,
Daniel Macks
no flags
Details
Formatted Diff
Diff
Always check for icu-config first
(1.63 KB, patch)
2008-12-06 13:18 PST
,
Daniel Macks
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Macks
Comment 1
2008-12-06 12:51:47 PST
Created
attachment 25817
[details]
Always check for icu-config first
Daniel Macks
Comment 2
2008-12-06 13:18:13 PST
Created
attachment 25819
[details]
Always check for icu-config first Fix ChangeLog
Mark Rowe (bdash)
Comment 3
2008-12-06 14:56:38 PST
Mac OS X ships with ICU, but doesn't ship with headers. The WebKit tree includes copies of the headers so that the system version of the library can be used.
Daniel Macks
Comment 4
2008-12-06 23:10:44 PST
(In reply to
comment #3
)
> Mac OS X ships with ICU, but doesn't ship with headers. The WebKit tree > includes copies of the headers so that the system version of the library can be > used.
I'm aware that OS X ships ICU libs but not headers, so it's great that WebKit itself has a copy of those missing files. But why does it need three copies of those files? Also, given that ICU is a generally useful thing, users may have ICU from some other vendor, installed in a more complete fashion (i.e., with headers).Given that it's an external lib (from perspective of WebKit), why not use it externally? Worse, if user does have a different ICU version installed, you're forcing use of headers (via local -I) that may not match the globally available lib (found via -l, there is no local -L for the lib). Even assuming a vanilla OS X system, 10.5 and 10.4 have different versions of ICU so half the time (as of public supported OS X releases:) you're *forcing* use of mismatched headers. Fink among other vendors can install global headers "that match OS X headerless lib", so I can be sure I get a matched set.
Mark Rowe (bdash)
Comment 5
2008-12-07 01:24:02 PST
(In reply to
comment #4
)
> (In reply to
comment #3
) > > Mac OS X ships with ICU, but doesn't ship with headers. The WebKit tree > > includes copies of the headers so that the system version of the library can be > > used. > > I'm aware that OS X ships ICU libs but not headers, so it's great that WebKit > itself has a copy of those missing files. But why does it need three copies of > those files?
Each WebKit project needs to be able to build independently of the others.
> > Also, given that ICU is a generally useful thing, users may have ICU from some > other vendor, installed in a more complete fashion (i.e., with headers).Given > that it's an external lib (from perspective of WebKit), why not use it > externally? Worse, if user does have a different ICU version installed, you're > forcing use of headers (via local -I) that may not match the globally available > lib (found via -l, there is no local -L for the lib). Even assuming a vanilla > OS X system, 10.5 and 10.4 have different versions of ICU so half the time (as > of public supported OS X releases:) you're *forcing* use of mismatched headers. > Fink among other vendors can install global headers "that match OS X headerless > lib", so I can be sure I get a matched set.
I made no statements about what behaviour the GTK port should take when building on Darwin, I simply pointed out why the headers are in the WebKit tree. Note that there is no problem in building against the 10.4 ICU headers but using the 10.5 ICU library. If there were problems with that it would indicate that ICU did not maintain source- and binary-level compatibility between releases.
Daniel Macks
Comment 6
2008-12-07 09:56:13 PST
Right, I agree that "it works on plain 10.4 and 10.5 systems". My point (with which I thought you were disagreeing) is only that it's fragile because it makes assumptions that the system *only* (or at least "first in -L paths") has the plain/expected libicu. My patch only changes behavior on machines that aren't vanilla...taking advantage of user-installed external ICU if it's present (otherwise falling back to the existing onboard headers and lib flags).
>Each WebKit project needs to be able to build independently of the others.
Ah, didn't realize the project was modularized like that. Makes sense.
Darin Adler
Comment 7
2008-12-10 14:48:16 PST
Comment on
attachment 25819
[details]
Always check for icu-config first
> + Default to use external libicu-config if avail on all platforms. > + > +
https://bugs.webkit.org/show_bug.cgi?id=22713
Tabs in ChangeLog. Please don't use tabs in patches you submit to WebKit. Change seems fine. r=me
Eric Seidel (no email)
Comment 8
2008-12-15 14:19:00 PST
Committing to
http://svn.webkit.org/repository/webkit/trunk
... M ChangeLog M configure.ac Committed
r39313
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug