r281375 and r281708 increased our ICU dependency to required ICU 61. For RHEL 8, which uses ICU 60, I am going to patch out this new requirement. I think it would be useful to have upstream to facilitate WebKitGTK updates in other older distributions too, if the JSC intl developers are OK with it. Regardless of whether we do this upstream or not, I'd appreciate review anyway to check that they way I've patched out the new feature is reasonable.
Actually wait, it seems current trunk uses HAVE(ICU_U_LOCALE_DISPLAY_NAMES) for this, but left the ICU build dep at ICU 61 anyway. That is inconsistent.
(In reply to Michael Catanzaro from comment #1) > Actually wait, it seems current trunk uses HAVE(ICU_U_LOCALE_DISPLAY_NAMES) > for this, but left the ICU build dep at ICU 61 anyway. That is inconsistent. I got confused... I was looking at the older version of the file. Ignore....
I found r283459. Basically I will just revert that, then fix the guards to avoid the compilation error that was addressed by r281708.
I think ICU 60 is too old. Can we consider bundling ICU for old distribution?
(In reply to Yusuke Suzuki from comment #4) > I think ICU 60 is too old. > Can we consider bundling ICU for old distribution? I'd sooner just maintain the ICU 60 support downstream. Using system libraries is a lot nicer than bundling.
(In reply to Yusuke Suzuki from comment #4) > I think ICU 60 is too old. > Can we consider bundling ICU for old distribution? I'll WONTFIX this, and maintain ICU 60 support downstream instead.
Created attachment 449514 [details] 2.34 patch For anyone else who wants it: this patch allows compiling WebKitGTK 2.34 with ICU 60. It's pretty simple.
Reopening to attach new patch.
Created attachment 449520 [details] Patch
Comment on attachment 449520 [details] Patch This is my patch for trunk. webkit-patch reopened this bug when I used it to attach this patch, so I suppose I'll mark it for review. I don't think it's very much burden to maintain upstream, but if you don't want it then feel free to r- and I'll carry it downstream.
It seems that I broke the build for ICU >= 61. :D
Comment on attachment 449520 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449520&action=review > Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:143 > + return; I don’t think we need a return here. > Source/JavaScriptCore/runtime/IntlDisplayNames.h:39 > +#if !defined(HAVE_ICU_U_LOCALE_DISPLAY_NAMES) > +// We need 61 or later since part of implementation uses UCURR_NARROW_SYMBOL_NAME. > +#if U_ICU_VERSION_MAJOR_NUM >= 61 > +#define HAVE_ICU_U_LOCALE_DISPLAY_NAMES 1 > +#endif > +#endif This belongs somewhere like PlatformHave.h rather than in an individual header file. > Source/JavaScriptCore/runtime/IntlObject.cpp:242 > + putDirectWithoutTransition(vm, vm.propertyNames->DisplayNames, createDisplayNamesConstructor(vm, this), static_cast<unsigned>(PropertyAttribute::DontEnum)); We don’t want to pay runtime cost on most platforms just so we can support the one that doesn’t have this. If you could find a way without any runtime cost, then I’d have no objections to having this on trunk. But if not, then I’d prefer you keep it downstream.
Created attachment 449521 [details] Patch
(In reply to Darin Adler from comment #12) > > Source/JavaScriptCore/runtime/IntlDisplayNames.cpp:143 > > + return; > > I don’t think we need a return here. Right. There's an extra return in the current trunk code, too, which I can remove. > > Source/JavaScriptCore/runtime/IntlDisplayNames.h:39 > > +#if !defined(HAVE_ICU_U_LOCALE_DISPLAY_NAMES) > > +// We need 61 or later since part of implementation uses UCURR_NARROW_SYMBOL_NAME. > > +#if U_ICU_VERSION_MAJOR_NUM >= 61 > > +#define HAVE_ICU_U_LOCALE_DISPLAY_NAMES 1 > > +#endif > > +#endif > > This belongs somewhere like PlatformHave.h rather than in an individual > header file. That would be nicer, but the downside is we'd have to #include <unicode/uversion.h> in PlatformHave.h. Then it's going to be included *everywhere*. So I just put it back where it was before. This also matches HAVE_ICU_U_LIST_FORMATTER defined in IntlListFormat.h. That said, the risk of not putting it in PlatformHave.h is that it is real easy to forget to #include IntlDisplayNames.h to get it, in which case it will evaluate to false by mistake, as the EWS discovered up above when I committed the same error.... > > Source/JavaScriptCore/runtime/IntlObject.cpp:242 > > + putDirectWithoutTransition(vm, vm.propertyNames->DisplayNames, createDisplayNamesConstructor(vm, this), static_cast<unsigned>(PropertyAttribute::DontEnum)); > > We don’t want to pay runtime cost on most platforms just so we can support > the one that doesn’t have this. If you could find a way without any runtime > cost, then I’d have no objections to having this on trunk. But if not, then > I’d prefer you keep it downstream. Hmm, well it's of course *possible* to avoid this one-liner, but not easily. I don't think we want to teach the hash table generator to respect preprocessor guards, so we'd have to split the hash table definitions out into two separate header files. Doesn't seem worth it.
(In reply to Michael Catanzaro from comment #7) > For anyone else who wants it: this patch allows compiling WebKitGTK 2.34 > with ICU 60. It's pretty simple. This broke the build with ICU 61. I'm attaching a corrected patch for WebKitGTK 2.34.
Created attachment 449525 [details] Patch for WebKitGTK 2.34
Created attachment 449526 [details] Patch for WebKitGTK 2.34 One more try, this type hopefully without gratuitous typos.
(In reply to Michael Catanzaro from comment #17) > Created attachment 449526 [details] > Patch for WebKitGTK 2.34 > > One more try, this type hopefully without gratuitous typos. I am applying this version of the patch to the 2.34 release branch, and version 2.34.5 will include it. Thanks for providing the backported patch ready to use!
Hi Adrian, can we have this for 2.36 too, please? (In reply to Michael Catanzaro from comment #14) > Hmm, well it's of course *possible* to avoid this one-liner, but not easily. > I don't think we want to teach the hash table generator to respect > preprocessor guards, so we'd have to split the hash table definitions out > into two separate header files. Doesn't seem worth it. This isn't worth it. I highly doubt it's going to have any effect on performance. We should be extremely conservative with our ICU dependency because it breaks ABI with every release, which makes updating it impossible.
Pull request: https://github.com/WebKit/WebKit/pull/5910
Giving up on this. Closing.