Bug 22713

Summary: Confusing/repetitive onboard unicode sources
Product: WebKit Reporter: Daniel Macks <dmacks>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Always check for icu-config first
none
Always check for icu-config first darin: review+

Description Daniel Macks 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.
Comment 1 Daniel Macks 2008-12-06 12:51:47 PST
Created attachment 25817 [details]
Always check for icu-config first
Comment 2 Daniel Macks 2008-12-06 13:18:13 PST
Created attachment 25819 [details]
Always check for icu-config first

Fix ChangeLog
Comment 3 Mark Rowe (bdash) 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.
Comment 4 Daniel Macks 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.
Comment 5 Mark Rowe (bdash) 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.
Comment 6 Daniel Macks 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.

Comment 7 Darin Adler 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
Comment 8 Eric Seidel (no email) 2008-12-15 14:19:00 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	configure.ac
Committed r39313