Bug 209694 - Update minimum ICU version to 60.2
Summary: Update minimum ICU version to 60.2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks: 210845
  Show dependency treegraph
 
Reported: 2020-03-27 20:29 PDT by Ross Kirsling
Modified: 2020-04-21 22:13 PDT (History)
31 users (show)

See Also:


Attachments
Patch (8.03 MB, patch)
2020-03-27 20:34 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.33 MB, patch)
2020-03-29 14:07 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.35 MB, patch)
2020-03-29 19:16 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.35 MB, patch)
2020-03-29 21:35 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for Commenting (41.61 KB, patch)
2020-03-30 12:31 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.35 MB, patch)
2020-03-30 16:48 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.36 MB, patch)
2020-03-31 15:56 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (8.36 MB, patch)
2020-03-31 17:56 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch with AppleWin workaround (8.36 MB, patch)
2020-04-02 15:15 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch with AppleWin workaround (round 2) (8.36 MB, patch)
2020-04-02 17:10 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (8.36 MB, patch)
2020-04-06 13:04 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch for landing (8.36 MB, patch)
2020-04-06 14:07 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-03-27 20:29:27 PDT
Update minimum ICU version to 57
Comment 1 Ross Kirsling 2020-03-27 20:34:27 PDT
Created attachment 394785 [details]
Patch
Comment 2 Keith Miller 2020-03-27 20:39:33 PDT
IMO, it seems like we should discuss this on WebKit-dev before committing to it. 57 might be the right number but we should get confirmation from all the other maintainers first.
Comment 3 Ross Kirsling 2020-03-27 20:42:40 PDT
Looks like GTK, WPE, WinCairo, and PlayStation are all on 63.1, but my understanding is that we're required to support back to macOS Sierra, which is on 57.1 (according to this: https://opensource.apple.com/release/macos-10126.html).
Comment 4 Ross Kirsling 2020-03-27 20:45:32 PDT
More than happy to send an email before landing though.
Comment 5 Yusuke Suzuki 2020-03-27 20:46:53 PDT
Oh, hmm, Windows port is getting compile error.
Comment 6 Ross Kirsling 2020-03-27 21:01:59 PDT
(In reply to Yusuke Suzuki from comment #5)
> Oh, hmm, Windows port is getting compile error.

Yikes, apparently AppleWin is using 49.1?! I'm amazed that that works at all...
(https://developer.apple.com/opensource/internet/WebKitAuxiliaryLibrary.zip)


I wonder if we could have this updated? We can certainly update the Mac headers without removing any version checks in this patch, but bug 209579 suggests that we definitely want to *have* a clear minimum version, and it seems like "what Sierra uses" would be most appropriate...
Comment 7 Yusuke Suzuki 2020-03-28 00:16:45 PDT
@Brent

Can we update ICU in AppleWin ports? It seems that bundled ICU is very old (49.1).
Comment 8 Darin Adler 2020-03-28 09:30:28 PDT
I’m pretty sure we can easily update ICU for the AppleWin port, but a more apropos question is, "Who knows how to do it?"
Comment 9 Darin Adler 2020-03-28 09:30:50 PDT
I am hoping Brent will know the answer!
Comment 10 Alexey Proskuryakov 2020-03-28 10:11:30 PDT
> Looks like GTK, WPE, WinCairo, and PlayStation are all on 63.1, but my
> understanding is that we're required to support back to macOS Sierra, which
> is on 57.1 (according to this:
> https://opensource.apple.com/release/macos-10126.html).

For ports maintained by Apple, that would be macOS Mojave and ICU 62.1.
Comment 11 Brent Fulgham 2020-03-28 11:07:56 PDT
(In reply to Darin Adler from comment #9)
> I am hoping Brent will know the answer!

I don’t know how to do this myself, but I know who to ask. I will get right on that!
Comment 12 Ross Kirsling 2020-03-28 12:03:15 PDT
(In reply to Alexey Proskuryakov from comment #10)
> > Looks like GTK, WPE, WinCairo, and PlayStation are all on 63.1, but my
> > understanding is that we're required to support back to macOS Sierra, which
> > is on 57.1 (according to this:
> > https://opensource.apple.com/release/macos-10126.html).
> 
> For ports maintained by Apple, that would be macOS Mojave and ICU 62.1.

Do you mean that we don't need to worry about Sierra? Yusuke and Darin thought that we did, so I assumed there must be some internal build requirements. If not, I'd gladly update all the way to 62 instead!

(In reply to Brent Fulgham from comment #11)
> (In reply to Darin Adler from comment #9)
> > I am hoping Brent will know the answer!
> 
> I don’t know how to do this myself, but I know who to ask. I will get right
> on that!

Thanks Brent!!
Comment 13 Alexey Proskuryakov 2020-03-28 16:23:08 PDT
Yes, macOS Mojave is currently the oldest one supported in trunk.
Comment 14 Darin Adler 2020-03-28 16:42:38 PDT
(In reply to Alexey Proskuryakov from comment #13)
> Yes, macOS Mojave is currently the oldest one supported in trunk.

That’s great news. Glad the Sierra support was a misunderstanding.

Do you think we should take the Sierra and HighSierra lines out of TestExpectations?
Comment 15 Darin Adler 2020-03-28 17:36:56 PDT
Once we figure out the AppleWin situation, lets go for 62.1 minimum version?
Comment 16 Ross Kirsling 2020-03-29 14:07:12 PDT
Created attachment 394877 [details]
Patch
Comment 17 Alexey Proskuryakov 2020-03-29 18:29:16 PDT
> Do you think we should take the Sierra and HighSierra lines out of
> TestExpectations?

Certainly!
Comment 18 Ross Kirsling 2020-03-29 19:16:29 PDT
Created attachment 394881 [details]
Patch
Comment 19 Ross Kirsling 2020-03-29 19:22:00 PDT
Cocoa platforms needed a lot of reinterpret-casting for UniChar, since these headers made it possible to keep treating UChar as uint16_t until now, even though it's char16_t as of ICU 59.

There's surely a better way to handle this (see, for instance, bug 195346), but hopefully this suffices for now?
Comment 20 Ross Kirsling 2020-03-29 21:35:18 PDT
Created attachment 394884 [details]
Patch
Comment 21 Darin Adler 2020-03-30 11:35:41 PDT
I’m trying to write review comments, but the patch is so big it’s not working.
Comment 22 Ross Kirsling 2020-03-30 12:31:56 PDT
Created attachment 394947 [details]
Patch for Commenting

Here are the changes outside of Source/WTF/icu/, for ease of review.
Comment 23 Brent Fulgham 2020-03-30 13:06:46 PDT
(In reply to Darin Adler from comment #8)
> I’m pretty sure we can easily update ICU for the AppleWin port, but a more
> apropos question is, "Who knows how to do it?"

It looks like the ICU we use on our real shipping software is ICU 62, so it seems like this is just a matter of getting the header files in our zip file updated.
Comment 24 Darin Adler 2020-03-30 13:09:29 PDT
Comment on attachment 394947 [details]
Patch for Commenting

View in context: https://bugs.webkit.org/attachment.cgi?id=394947&action=review

Seems like we should stop using the name UChar eventually and use char16_t directly instead.

Looks good. If you know why Windows build is failing and can fix it, then r=me

> Source/WTF/wtf/text/cocoa/StringViewCocoa.mm:46
> +    return adoptNS([[NSString alloc] initWithCharactersNoCopy:reinterpret_cast<UniChar*>(const_cast<UChar*>(characters16())) length:length() freeWhenDone:NO]);

Type name here is wrong. This should be "unichar*" for passing to NSString.

> Source/WebCore/editing/cocoa/DataDetection.mm:380
> +        auto currentCharPtr = reinterpret_cast<const UniChar*>(currentTextUpconvertedCharacters.get());

I don’t think the typecast is needed. We dereference the pointer and use its value, but I think it can be char16_t.

> Source/WebCore/page/ios/FrameIOS.mm:792
> +        [result addObject:adoptNS([[NSString alloc] initWithCharacters:reinterpret_cast<const UniChar*>(interpretation.data()) length:interpretation.size()]).get()];

Should be unichar (all lowercase).

> Source/WebCore/platform/text/CharacterProperties.h:36
> +    return unicodeBlock == UBLOCK_MISCELLANEOUS_SYMBOLS

Maybe a switch instead?

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMTextIterator.mm:82
> +        return reinterpret_cast<const UniChar*>(text.characters16());

Should be "const unichar*", all lowercase.

> Source/WebKitLegacy/mac/WebCoreSupport/WebVisitedLinkStore.mm:87
> +    if (const UniChar* characters = CFStringGetCharactersPtr((__bridge CFStringRef)urlString)) {

How about auto here?

> Source/WebKitLegacy/mac/WebView/WebTextIterator.mm:111
> +        return reinterpret_cast<const UniChar*>(text.characters16());

Should be unichar (all lowercase) here.
Comment 25 Darin Adler 2020-03-30 13:14:48 PDT
(In reply to Brent Fulgham from comment #23)
> (In reply to Darin Adler from comment #8)
> > I’m pretty sure we can easily update ICU for the AppleWin port, but a more
> > apropos question is, "Who knows how to do it?"
> 
> It looks like the ICU we use on our real shipping software is ICU 62, so it
> seems like this is just a matter of getting the header files in our zip file
> updated.

Maybe we can use the headers checked into WTF instead? Like for the Cocoa platforms.
Comment 26 Ross Kirsling 2020-03-30 16:33:57 PDT
(In reply to Darin Adler from comment #24)
> Comment on attachment 394947 [details]
> Patch for Commenting
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394947&action=review
> 
> Seems like we should stop using the name UChar eventually and use char16_t
> directly instead.

I can agree with that!

> Looks good. If you know why Windows build is failing and can fix it, then
> r=me

The current error EWS is showing is just the same issue -- UChar was wchar_t before ICU 59 on Windows.

Will address the rest of your comments.
Comment 27 Darin Adler 2020-03-30 16:43:57 PDT
Changing UChar to be char16_t instead of uint16_t is going to be a nice fix for passing uint16_t to functions like StringBuilder::append and makeString. Lets us take out some places where we cast to int or unsigned to work around the ambiguity.

Like in WebSocket::connect for example.
Comment 28 Ross Kirsling 2020-03-30 16:48:04 PDT
Created attachment 394985 [details]
Patch
Comment 29 Ross Kirsling 2020-03-30 19:03:02 PDT
I added the version requirement to Options*.cmake in the latest patch as well -- this means that ARMv7 EWS (and maybe MIPS? not sure yet) will need to be updated, but Caio suggested to me that this should be doable.
Comment 30 Carlos Alberto Lopez Perez 2020-03-31 14:55:20 PDT
This its NOT OK for WebKitGTK.

We still have to support ICU 60 because it is what ships Ubuntu 18.04 LTS (Bionic) https://packages.ubuntu.com/search?keywords=libicu-dev&searchon=names&suite=bionic&section=all

And per our policies dependency we should aim at supporting that until 2021-04 <https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy>
Comment 31 Ross Kirsling 2020-03-31 15:02:33 PDT
(In reply to Carlos Alberto Lopez Perez from comment #30)
> This its NOT OK for WebKitGTK.
> 
> We still have to support ICU 60 because it is what ships Ubuntu 18.04 LTS
> (Bionic)
> https://packages.ubuntu.com/search?keywords=libicu-
> dev&searchon=names&suite=bionic&section=all
> 
> And per our policies dependency we should aim at supporting that until
> 2021-04 <https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy>

Thanks for the important clarification!

I think this patch is still valid other than the Options*.cmake changes, but I guess this means our official minimum version needs to be 60 then.
Comment 32 Konstantin Tokarev 2020-03-31 15:12:09 PDT
What was the reason for requiring 62 in the first place? From patch it seems like there is no conditional compilation requiring versions > 60
Comment 33 Darin Adler 2020-03-31 15:12:28 PDT
Let's rename this bug to reflect our new proposed minimum.
Comment 34 Darin Adler 2020-03-31 15:26:16 PDT
We’re just trying to find the right new minimum. At first we thought it was going to be 57. I think 60 will be good.
Comment 35 Konstantin Tokarev 2020-03-31 15:35:52 PDT
If principal point is UChar being char16_t, this transition happened in 59
Comment 36 Ross Kirsling 2020-03-31 15:49:12 PDT
(In reply to Konstantin Tokarev from comment #35)
> If principal point is UChar being char16_t, this transition happened in 59

That's not the motivation, that's just the thing that Cocoa hadn't yet adapted to. The motivations are:
1. to actually have a minimum version (see bug 209579)
2. to support development of ECMA-402 Intl API features, which JSC is quite behind on (and which are often a matter of just exposing ICU functionality to JavaScript)
Comment 37 Ross Kirsling 2020-03-31 15:56:51 PDT
Created attachment 395101 [details]
Patch
Comment 38 Konstantin Tokarev 2020-03-31 16:34:45 PDT
(In reply to Ross Kirsling from comment #36)
> (In reply to Konstantin Tokarev from comment #35)
> > If principal point is UChar being char16_t, this transition happened in 59
> 
> That's not the motivation, that's just the thing that Cocoa hadn't yet
> adapted to. The motivations are:
> 1. to actually have a minimum version (see bug 209579)
> 2. to support development of ECMA-402 Intl API features, which JSC is quite
> behind on (and which are often a matter of just exposing ICU functionality
> to JavaScript)

Thanks for explanation! It would be great to have it in commit message as well.
Comment 39 Ross Kirsling 2020-03-31 17:16:01 PDT
(In reply to Konstantin Tokarev from comment #38)
> Thanks for explanation! It would be great to have it in commit message as
> well.

Will add!
Comment 40 Ross Kirsling 2020-03-31 17:56:34 PDT
Created attachment 395116 [details]
Patch
Comment 41 Ross Kirsling 2020-03-31 23:05:59 PDT
Alright, this should be fully ready for all platforms now, just as soon as AppleWin's ZIP file is updated.
Comment 42 Ross Kirsling 2020-04-02 09:43:57 PDT
Any update on the AppleWin situation? Please let me know if there's anything a non-Appler can do to help!
Comment 43 Darin Adler 2020-04-02 10:35:07 PDT
We ought to try the experiment where we use the ICU headers checked into the WebKit repository rather than the ICU headers from the AppleWin .zip file. I think a non-Apple person might be able to try it; not 100% sure.
Comment 44 Darin Adler 2020-04-02 10:35:40 PDT
It’s a peculiar approach, but it’s worked for us on macOS and iOS.
Comment 45 Don Olmstead 2020-04-02 12:44:28 PDT
(In reply to Darin Adler from comment #43)
> We ought to try the experiment where we use the ICU headers checked into the
> WebKit repository rather than the ICU headers from the AppleWin .zip file. I
> think a non-Apple person might be able to try it; not 100% sure.

My concern there is the Apple Win internal build. It builds each directory individually and there's no script in WebKit repo that emulates this behavior so people outside of Apple could check to make sure things are behaving.

As a quick workaround Ross you can change https://github.com/WebKit/webkit/blob/master/Source/cmake/WebKitFindPackage.cmake#L34

- find_path(ICU_INCLUDE_DIR NAMES unicode/utypes.h)
+ set(ICU_INCLUDE_DIR ${WTF_DIR}/icu)

which should get things a bit further.
Comment 46 Alexey Proskuryakov 2020-04-02 13:35:44 PDT
(In reply to Darin Adler from comment #43)
> We ought to try the experiment where we use the ICU headers checked into the
> WebKit repository rather than the ICU headers from the AppleWin .zip file. I
> think a non-Apple person might be able to try it; not 100% sure.

We'd need to update the libraries, as having new headers is of little use if linking is going to fail.
Comment 47 Ross Kirsling 2020-04-02 15:15:50 PDT
Created attachment 395309 [details]
Patch with AppleWin workaround
Comment 48 Darin Adler 2020-04-02 15:53:29 PDT
(In reply to Alexey Proskuryakov from comment #46)
> (In reply to Darin Adler from comment #43)
> > We ought to try the experiment where we use the ICU headers checked into the
> > WebKit repository rather than the ICU headers from the AppleWin .zip file. I
> > think a non-Apple person might be able to try it; not 100% sure.
> 
> We'd need to update the libraries, as having new headers is of little use if
> linking is going to fail.

Brent said: "It looks like the ICU we use on our real shipping software is ICU 62, so it seems like this is just a matter of getting the header files in our zip file updated."
Comment 49 Ross Kirsling 2020-04-02 17:10:57 PDT
Created attachment 395328 [details]
Patch with AppleWin workaround (round 2)
Comment 50 Alexey Proskuryakov 2020-04-02 18:02:38 PDT
> Brent said: "It looks like the ICU we use on our real shipping software is
> ICU 62, so it seems like this is just a matter of getting the header files
> in our zip file updated."

I think that .lib linker stub files would also need to be updated. I downloaded WebKitSupportLibrary.zip, but I can't find which versions these have.

lib64/libicuuc.lib
lib64/libicuin.lib
Comment 51 Ross Kirsling 2020-04-02 19:46:25 PDT
Comment on attachment 395328 [details]
Patch with AppleWin workaround (round 2)

Pointing ICU_INCLUDE_DIR at WTF/icu didn't work, but actually performing the copy appears to have!
(The layout test failures here are unrelated to this patch.)
Comment 52 Ross Kirsling 2020-04-02 20:40:55 PDT
(In reply to Alexey Proskuryakov from comment #50)
> > Brent said: "It looks like the ICU we use on our real shipping software is
> > ICU 62, so it seems like this is just a matter of getting the header files
> > in our zip file updated."
> 
> I think that .lib linker stub files would also need to be updated. I
> downloaded WebKitSupportLibrary.zip, but I can't find which versions these
> have.
> 
> lib64/libicuuc.lib
> lib64/libicuin.lib

I tried `dumpbin /exports libicuin.lib` and it contains all of the unumf_* symbols that were introduced in ICU 62 (https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/unumberformatter_8h.html#a6f47836ca05077fc912ad24e462312c6). So it really should be safe to just copy the new headers until somebody has time to update WebKitAuxiliaryLibrary.zip.
Comment 53 Don Olmstead 2020-04-03 11:01:34 PDT
(In reply to Ross Kirsling from comment #51)
> Comment on attachment 395328 [details]
> Patch with AppleWin workaround (round 2)
> 
> Pointing ICU_INCLUDE_DIR at WTF/icu didn't work, but actually performing the
> copy appears to have!
> (The layout test failures here are unrelated to this patch.)

> if (APPLE OR WTF_PLATFORM_APPLE_WIN)
>    file(COPY ${WTF_DIR}/icu/unicode DESTINATION ${ICU_INCLUDE_DIRS})

This value is only set directly for Apple ports since they have a well defined location for where the headers go. AppleWin does not set this anywhere so I'm not sure where exactly you're actually copying this to.

Also I have no idea how well copying will work in the AppleWin internal build. If we copied it into WebKitLibraries would that mean the updated headers would be there when JavaScriptCore, WebCore, WebKitLegaccy are built?
Comment 54 Ross Kirsling 2020-04-06 11:36:02 PDT
Comment on attachment 395116 [details]
Patch

Woohoo! WebKitAuxiliaryLibrary.zip has been updated and this is now ready to go, sans workaround. Thank you, Per Arne!
Comment 55 Ross Kirsling 2020-04-06 13:04:47 PDT
Created attachment 395606 [details]
Patch for landing
Comment 56 Ross Kirsling 2020-04-06 14:07:15 PDT
Created attachment 395615 [details]
Patch for landing
Comment 57 EWS 2020-04-06 16:15:52 PDT
Committed r259606: <https://trac.webkit.org/changeset/259606>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395615 [details].
Comment 58 Radar WebKit Bug Importer 2020-04-06 16:16:34 PDT
<rdar://problem/61364567>