Bug 235367 - Allow building with ICU 60
Summary: Allow building with ICU 60
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-19 10:09 PST by Michael Catanzaro
Modified: 2022-04-22 10:52 PDT (History)
16 users (show)

See Also:


Attachments
2.34 patch (2.09 KB, patch)
2022-01-19 13:43 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2022-01-19 14:47 PST, Michael Catanzaro
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (12.46 KB, patch)
2022-01-19 15:08 PST, Michael Catanzaro
mcatanzaro: review?
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for WebKitGTK 2.34 (2.39 KB, patch)
2022-01-19 15:47 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch for WebKitGTK 2.34 (2.39 KB, patch)
2022-01-19 15:52 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2022-01-19 10:09:36 PST
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.
Comment 1 Michael Catanzaro 2022-01-19 10:12:45 PST
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.
Comment 2 Michael Catanzaro 2022-01-19 10:16:08 PST
(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....
Comment 3 Michael Catanzaro 2022-01-19 10:22:43 PST
I found r283459. Basically I will just revert that, then fix the guards to avoid the compilation error that was addressed by r281708.
Comment 4 Yusuke Suzuki 2022-01-19 11:50:29 PST
I think ICU 60 is too old.
Can we consider bundling ICU for old distribution?
Comment 5 Michael Catanzaro 2022-01-19 12:25:21 PST
(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.
Comment 6 Michael Catanzaro 2022-01-19 12:49:01 PST
(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.
Comment 7 Michael Catanzaro 2022-01-19 13:43:37 PST
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.
Comment 8 Michael Catanzaro 2022-01-19 14:47:37 PST
Reopening to attach new patch.
Comment 9 Michael Catanzaro 2022-01-19 14:47:40 PST
Created attachment 449520 [details]
Patch
Comment 10 Michael Catanzaro 2022-01-19 14:49:16 PST
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.
Comment 11 Michael Catanzaro 2022-01-19 15:03:22 PST
It seems that I broke the build for ICU >= 61. :D
Comment 12 Darin Adler 2022-01-19 15:03:33 PST
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.
Comment 13 Michael Catanzaro 2022-01-19 15:08:47 PST
Created attachment 449521 [details]
Patch
Comment 14 Michael Catanzaro 2022-01-19 15:25:59 PST
(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.
Comment 15 Michael Catanzaro 2022-01-19 15:46:50 PST
(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.
Comment 16 Michael Catanzaro 2022-01-19 15:47:14 PST
Created attachment 449525 [details]
Patch for WebKitGTK 2.34
Comment 17 Michael Catanzaro 2022-01-19 15:52:16 PST
Created attachment 449526 [details]
Patch for WebKitGTK 2.34

One more try, this type hopefully without gratuitous typos.
Comment 18 Adrian Perez 2022-02-08 06:31:14 PST
(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!
Comment 19 Michael Catanzaro 2022-04-22 10:52:38 PDT
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.