Summary: | Update minimum ICU version to 60.2 | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Ross Kirsling <ross.kirsling> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, ap, benjamin, bfulgham, bugs-noreply, cdumez, clopez, cmarcelo, darin, don.olmstead, eric.carlson, ews-watchlist, glenn, guijemont, gyuyoung.kim, Hironori.Fujii, jer.noble, keith_miller, mark.lam, mifenton, mmaxfield, msaboff, philipj, pvollan, ryuan.choi, saam, sergio, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=209579 https://bugs.webkit.org/show_bug.cgi?id=210812 https://bugs.webkit.org/show_bug.cgi?id=210845 https://bugs.webkit.org/show_bug.cgi?id=229608 |
||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 210845 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Ross Kirsling
2020-03-27 20:29:27 PDT
Created attachment 394785 [details]
Patch
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. 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). More than happy to send an email before landing though. Oh, hmm, Windows port is getting compile error. (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... @Brent Can we update ICU in AppleWin ports? It seems that bundled ICU is very old (49.1). 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?" I am hoping Brent will know the answer! > 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.
(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! (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!! Yes, macOS Mojave is currently the oldest one supported in trunk. (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? Once we figure out the AppleWin situation, lets go for 62.1 minimum version? Created attachment 394877 [details]
Patch
> Do you think we should take the Sierra and HighSierra lines out of
> TestExpectations?
Certainly!
Created attachment 394881 [details]
Patch
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? Created attachment 394884 [details]
Patch
I’m trying to write review comments, but the patch is so big it’s not working. Created attachment 394947 [details]
Patch for Commenting
Here are the changes outside of Source/WTF/icu/, for ease of review.
(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 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. (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. (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. 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. Created attachment 394985 [details]
Patch
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. 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§ion=all And per our policies dependency we should aim at supporting that until 2021-04 <https://trac.webkit.org/wiki/WebKitGTK/DependenciesPolicy> (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§ion=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. What was the reason for requiring 62 in the first place? From patch it seems like there is no conditional compilation requiring versions > 60 Let's rename this bug to reflect our new proposed minimum. 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. If principal point is UChar being char16_t, this transition happened in 59 (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) Created attachment 395101 [details]
Patch
(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. (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! Created attachment 395116 [details]
Patch
Alright, this should be fully ready for all platforms now, just as soon as AppleWin's ZIP file is updated. Any update on the AppleWin situation? Please let me know if there's anything a non-Appler can do to help! 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. It’s a peculiar approach, but it’s worked for us on macOS and iOS. (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. (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. Created attachment 395309 [details]
Patch with AppleWin workaround
(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." Created attachment 395328 [details]
Patch with AppleWin workaround (round 2)
> 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 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.)
(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. (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 on attachment 395116 [details]
Patch
Woohoo! WebKitAuxiliaryLibrary.zip has been updated and this is now ready to go, sans workaround. Thank you, Per Arne!
Created attachment 395606 [details]
Patch for landing
Created attachment 395615 [details]
Patch for landing
Committed r259606: <https://trac.webkit.org/changeset/259606> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395615 [details]. |