Bug 235367 – Allow building with ICU 60

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

(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....

[Michael Catanzaro]

I found r283459. Basically I will just revert that, then fix the guards to avoid the compilation error that was addressed by r281708.

[Yusuke Suzuki]

I think ICU 60 is too old.
Can we consider bundling ICU for old distribution?

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

Reopening to attach new patch.

[Michael Catanzaro]

Created attachment 449520 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

It seems that I broke the build for ICU >= 61. :D

[Darin Adler]

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.

[Michael Catanzaro]

Created attachment 449521 [details]
Patch

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Created attachment 449525 [details]
Patch for WebKitGTK 2.34

[Michael Catanzaro]

Created attachment 449526 [details]
Patch for WebKitGTK 2.34

One more try, this type hopefully without gratuitous typos.

[Adrian Perez]

(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!

[Michael Catanzaro]

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.

[Michael Catanzaro]

Pull request: https://github.com/WebKit/WebKit/pull/5910

[Michael Catanzaro]

Giving up on this. Closing.