Bug 209694 – Update minimum ICU version to 60.2

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ross Kirsling]

Update minimum ICU version to 57

[Ross Kirsling]

Created attachment 394785 [details]
Patch

[Keith Miller]

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.

[Ross Kirsling]

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

[Ross Kirsling]

More than happy to send an email before landing though.

[Yusuke Suzuki]

Oh, hmm, Windows port is getting compile error.

[Ross Kirsling]

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

[Yusuke Suzuki]

@Brent

Can we update ICU in AppleWin ports? It seems that bundled ICU is very old (49.1).

[Darin Adler]

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?"

[Darin Adler]

I am hoping Brent will know the answer!

[Alexey Proskuryakov]

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

[Brent Fulgham]

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

[Ross Kirsling]

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

[Alexey Proskuryakov]

Yes, macOS Mojave is currently the oldest one supported in trunk.

[Darin Adler]

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

[Darin Adler]

Once we figure out the AppleWin situation, lets go for 62.1 minimum version?

[Ross Kirsling]

Created attachment 394877 [details]
Patch

[Alexey Proskuryakov]

> Do you think we should take the Sierra and HighSierra lines out of
> TestExpectations?

Certainly!

[Ross Kirsling]

Created attachment 394881 [details]
Patch

[Ross Kirsling]

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?

[Ross Kirsling]

Created attachment 394884 [details]
Patch

[Darin Adler]

I’m trying to write review comments, but the patch is so big it’s not working.

[Ross Kirsling]

Created attachment 394947 [details]
Patch for Commenting

Here are the changes outside of Source/WTF/icu/, for ease of review.

[Brent Fulgham]

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

[Darin Adler]

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.

[Darin Adler]

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

[Ross Kirsling]

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

[Darin Adler]

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.

[Ross Kirsling]

Created attachment 394985 [details]
Patch

[Ross Kirsling]

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.

[Carlos Alberto Lopez Perez]

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>

[Ross Kirsling]

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

[Konstantin Tokarev]

What was the reason for requiring 62 in the first place? From patch it seems like there is no conditional compilation requiring versions > 60

[Darin Adler]

Let's rename this bug to reflect our new proposed minimum.

[Darin Adler]

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.

[Konstantin Tokarev]

If principal point is UChar being char16_t, this transition happened in 59

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 395101 [details]
Patch

[Konstantin Tokarev]

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

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 395116 [details]
Patch

[Ross Kirsling]

Alright, this should be fully ready for all platforms now, just as soon as AppleWin's ZIP file is updated.

[Ross Kirsling]

Any update on the AppleWin situation? Please let me know if there's anything a non-Appler can do to help!

[Darin Adler]

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.

[Darin Adler]

It’s a peculiar approach, but it’s worked for us on macOS and iOS.

[Don Olmstead]

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

[Alexey Proskuryakov]

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

[Ross Kirsling]

Created attachment 395309 [details]
Patch with AppleWin workaround

[Darin Adler]

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

[Ross Kirsling]

Created attachment 395328 [details]
Patch with AppleWin workaround (round 2)

[Alexey Proskuryakov]

> 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

[Ross Kirsling]

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

[Ross Kirsling]

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

[Don Olmstead]

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

[Ross Kirsling]

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!

[Ross Kirsling]

Created attachment 395606 [details]
Patch for landing

[Ross Kirsling]

Created attachment 395615 [details]
Patch for landing

[EWS]

Committed r259606: <https://trac.webkit.org/changeset/259606>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395615 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61364567>