22713 2008-12-06 10:51:53 -0800 Confusing/repetitive onboard unicode sources 2008-12-15 14:19:00 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Mac OS X 10.4 RESOLVED FIXED P2 Normal --- 0 dmacks webkit-unassigned oldest_to_newest 101665 0 dmacks 2008-12-06 10:51:53 -0800 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. 101668 1 25817 dmacks 2008-12-06 12:51:47 -0800 Created attachment 25817 Always check for icu-config first 101672 2 25819 dmacks 2008-12-06 13:18:13 -0800 Created attachment 25819 Always check for icu-config first Fix ChangeLog 101687 3 mrowe 2008-12-06 14:56:38 -0800 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. 101713 4 dmacks 2008-12-06 23:10:44 -0800 (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. 101718 5 mrowe 2008-12-07 01:24:02 -0800 (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. 101752 6 dmacks 2008-12-07 09:56:13 -0800 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. 102157 7 25819 darin 2008-12-10 14:48:16 -0800 Comment on attachment 25819 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 102603 8 eric 2008-12-15 14:19:00 -0800 Committing to http://svn.webkit.org/repository/webkit/trunk ... M ChangeLog M configure.ac Committed r39313 25817 2008-12-06 12:51:47 -0800 2008-12-06 13:18:13 -0800 Always check for icu-config first prep.patch text/plain 1636 dmacks SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM5MDY5 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTEgQEAKKzIwMDgtMTIt MDYgIERhbmllbCBNYWNrcyAgPHNldCBFTUFJTF9BRERSRVNTIGVudmlyb25tZW50IHZhcmlhYmxl PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisJRGVmYXVsdCB0byB1 c2UgZXh0ZXJuYWwgbGliaWN1LWNvbmZpZyBpZiBhdmFpbCBvbiBhbGwgcGxhdGZvcm1zLgorCisg ICAgICAgICogY29uZmlndXJlLmFjOgorCiAyMDA4LTEyLTAzICBTaW1vbiBIYXVzbWFubiAgPGhh dXNtYW5uQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgVG9yIEFybmUgVmVzdGLD uC4KSW5kZXg6IGNvbmZpZ3VyZS5hYwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBjb25maWd1cmUuYWMJKHJldmlz aW9uIDM5MDY4KQorKysgY29uZmlndXJlLmFjCSh3b3JraW5nIGNvcHkpCkBAIC0yMzQsMTggKzIz NCwxOCBAQCBlc2FjCiBBQ19NU0dfUkVTVUxUKFskdW5pY29kZV9iYWNrZW5kXSkKIAogaWYgdGVz dCAiJHVuaWNvZGVfYmFja2VuZCIgPSAiaWN1IjsgdGhlbgotCWlmIHRlc3QgIiRvc19kYXJ3aW4i ID0gInllcyI7IHRoZW4KLQkJVU5JQ09ERV9DRkxBR1M9Ii1JXCQoc3JjZGlyKS9KYXZhU2NyaXB0 Q29yZS9pY3UgLUlcJChzcmNkaXIpL1dlYkNvcmUvaWN1IgotCQlVTklDT0RFX0xJQlM9Ii1saWN1 Y29yZSIKLQllbGlmIHRlc3QgIiRvc193aW4zMiIgPSAieWVzIjsgdGhlbgotCQlVTklDT0RFX0NG TEFHUz0iIgotCQlVTklDT0RFX0xJQlM9Ii1saWN1aW4gLWxpY3V1YyIKLQllbHNlCi0JCUFDX1BB VEhfUFJPRyhpY3VfY29uZmlnLCBpY3UtY29uZmlnLCBubykKLQkJaWYgdGVzdCAiJGljdV9jb25m aWciID0gIm5vIjsgdGhlbgorCUFDX1BBVEhfUFJPRyhpY3VfY29uZmlnLCBpY3UtY29uZmlnLCBu bykKKwlpZiB0ZXN0ICIkaWN1X2NvbmZpZyIgPSAibm8iOyB0aGVuCisJCWlmIHRlc3QgIiRvc19k YXJ3aW4iID0gInllcyI7IHRoZW4KKwkJCVVOSUNPREVfQ0ZMQUdTPSItSVwkKHNyY2RpcikvSmF2 YVNjcmlwdENvcmUvaWN1IC1JXCQoc3JjZGlyKS9XZWJDb3JlL2ljdSIKKwkJCVVOSUNPREVfTElC Uz0iLWxpY3Vjb3JlIgorCQllbGlmIHRlc3QgIiRvc193aW4zMiIgPSAieWVzIjsgdGhlbgorCQkJ VU5JQ09ERV9DRkxBR1M9IiIKKwkJCVVOSUNPREVfTElCUz0iLWxpY3VpbiAtbGljdXVjIgorCQll bHNlCiAJCQlBQ19NU0dfRVJST1IoW0Nhbm5vdCBmaW5kIGljdS1jb25maWcuIFRoZSBJQ1UgbGli cmFyeSBpcyBuZWVkZWQuXSkKIAkJZmkKLQorCWVsc2UKIAkJIyBXZSBkb24ndCB1c2UgLS1jZmxh Z3MgYXMgdGhpcyBnaXZlcyB1cyBhIGxvdCBvZiB0aGluZ3MgdGhhdCB3ZSBkb24ndAogCQkjIG5l Y2Vzc2FyaWx5IHdhbnQsIGxpa2UgZGVidWdnaW5nIGFuZCBvcHRpbWl6YXRpb24gZmxhZ3MKIAkJ IyBTZWUgbWFuICgxKSBpY3UtY29uZmlnIGZvciBtb3JlIGluZm8uCg== 25819 2008-12-06 13:18:13 -0800 2008-12-10 14:48:16 -0800 Always check for icu-config first unicode.patch text/plain 1667 dmacks SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM5MDY5 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMDgtMTIt MDYgIERhbmllbCBNYWNrcyAgPGRtYWNrc0BuZXRzcGFjZS5vcmc+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKwlEZWZhdWx0IHRvIHVzZSBleHRlcm5hbCBsaWJpY3Ut Y29uZmlnIGlmIGF2YWlsIG9uIGFsbCBwbGF0Zm9ybXMuCisKKwlodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjI3MTMKKworICAgICAgICAqIGNvbmZpZ3VyZS5hYzoKKwog MjAwOC0xMi0wMyAgU2ltb24gSGF1c21hbm4gIDxoYXVzbWFubkB3ZWJraXQub3JnPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IFRvciBBcm5lIFZlc3Riw7guCkluZGV4OiBjb25maWd1cmUuYWMKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gY29uZmlndXJlLmFjCShyZXZpc2lvbiAzOTA2OCkKKysrIGNvbmZpZ3VyZS5h Ywkod29ya2luZyBjb3B5KQpAQCAtMjM0LDE4ICsyMzQsMTggQEAgZXNhYwogQUNfTVNHX1JFU1VM VChbJHVuaWNvZGVfYmFja2VuZF0pCiAKIGlmIHRlc3QgIiR1bmljb2RlX2JhY2tlbmQiID0gImlj dSI7IHRoZW4KLQlpZiB0ZXN0ICIkb3NfZGFyd2luIiA9ICJ5ZXMiOyB0aGVuCi0JCVVOSUNPREVf Q0ZMQUdTPSItSVwkKHNyY2RpcikvSmF2YVNjcmlwdENvcmUvaWN1IC1JXCQoc3JjZGlyKS9XZWJD b3JlL2ljdSIKLQkJVU5JQ09ERV9MSUJTPSItbGljdWNvcmUiCi0JZWxpZiB0ZXN0ICIkb3Nfd2lu MzIiID0gInllcyI7IHRoZW4KLQkJVU5JQ09ERV9DRkxBR1M9IiIKLQkJVU5JQ09ERV9MSUJTPSIt bGljdWluIC1saWN1dWMiCi0JZWxzZQotCQlBQ19QQVRIX1BST0coaWN1X2NvbmZpZywgaWN1LWNv bmZpZywgbm8pCi0JCWlmIHRlc3QgIiRpY3VfY29uZmlnIiA9ICJubyI7IHRoZW4KKwlBQ19QQVRI X1BST0coaWN1X2NvbmZpZywgaWN1LWNvbmZpZywgbm8pCisJaWYgdGVzdCAiJGljdV9jb25maWci ID0gIm5vIjsgdGhlbgorCQlpZiB0ZXN0ICIkb3NfZGFyd2luIiA9ICJ5ZXMiOyB0aGVuCisJCQlV TklDT0RFX0NGTEFHUz0iLUlcJChzcmNkaXIpL0phdmFTY3JpcHRDb3JlL2ljdSAtSVwkKHNyY2Rp cikvV2ViQ29yZS9pY3UiCisJCQlVTklDT0RFX0xJQlM9Ii1saWN1Y29yZSIKKwkJZWxpZiB0ZXN0 ICIkb3Nfd2luMzIiID0gInllcyI7IHRoZW4KKwkJCVVOSUNPREVfQ0ZMQUdTPSIiCisJCQlVTklD T0RFX0xJQlM9Ii1saWN1aW4gLWxpY3V1YyIKKwkJZWxzZQogCQkJQUNfTVNHX0VSUk9SKFtDYW5u b3QgZmluZCBpY3UtY29uZmlnLiBUaGUgSUNVIGxpYnJhcnkgaXMgbmVlZGVkLl0pCiAJCWZpCi0K KwllbHNlCiAJCSMgV2UgZG9uJ3QgdXNlIC0tY2ZsYWdzIGFzIHRoaXMgZ2l2ZXMgdXMgYSBsb3Qg b2YgdGhpbmdzIHRoYXQgd2UgZG9uJ3QKIAkJIyBuZWNlc3NhcmlseSB3YW50LCBsaWtlIGRlYnVn Z2luZyBhbmQgb3B0aW1pemF0aW9uIGZsYWdzCiAJCSMgU2VlIG1hbiAoMSkgaWN1LWNvbmZpZyBm b3IgbW9yZSBpbmZvLgo=