|Summary:||Clean up ICU headers|
|Product:||WebKit||Reporter:||Andy VanWagoner <andy>|
|Component:||Platform||Assignee:||Andy VanWagoner <andy>|
|Severity:||Normal||CC:||ap, buildbot, commit-queue, ddkilzer, ggaren, jfbastien, keith_miller, mark.lam, mitz, mmaxfield, msaboff, sbarati|
|Version:||WebKit Nightly Build|
|Bug Depends on:||169458|
Description Andy VanWagoner 2017-04-19 09:42:47 PDT
Ideally we should not need the ICU headers in the WebKit source, so if possible, we should remove them. If we cannot remove them, then all ICU headers should be on the same version (iirc 55.1 is the minimum version for all platforms).
Comment 1 JF Bastien 2017-04-19 10:36:41 PDT
We seem to have 4 copies, and as best I can tell from the project files we probably don't build for earlier than 10.10 at this point.
Comment 2 Andy VanWagoner 2017-04-19 11:54:30 PDT
I found the ICU headers in Xcode.app/Contents but it looks like it is only part of the AppleTV, iPhone, & Watch platforms. /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform/Developer/SDKs/AppleTVOS.sdk/usr/include/unicode /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/usr/include /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS.sdk/usr/include/unicode The MacOSX platform does *not* have a corresponding include folder. /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unicode Should the README in the icu folders of the projects be updated to make it more clear that it's not just Mac OS X 10.4, but also all subsequent versions of Mac OS X, that require the headers?
Comment 3 JF Bastien 2017-04-19 12:56:39 PDT
(In reply to Andy VanWagoner from comment #2) > I found the ICU headers in Xcode.app/Contents but it looks like it is only > part of the AppleTV, iPhone, & Watch platforms. > > /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform/ > Developer/SDKs/AppleTVOS.sdk/usr/include/unicode > /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/ > Developer/SDKs/iPhoneOS.sdk/usr/include > /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/ > Developer/SDKs/WatchOS.sdk/usr/include/unicode > > The MacOSX platform does *not* have a corresponding include folder. > > /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/ > Developer/SDKs/MacOSX.sdk/usr/include/unicode > > Should the README in the icu folders of the projects be updated to make it > more clear that it's not just Mac OS X 10.4, but also all subsequent > versions of Mac OS X, that require the headers? Dan was just telling me the same thing :) I think you're correct: the READMEs are misleading, it's 10.4 and up.
Comment 4 Andy VanWagoner 2017-04-22 10:57:49 PDT
I've started by simply updating all the existing files to v55.1. Should I add headers we don't use yet, as long as they are part of the C API?
Comment 5 JF Bastien 2017-04-24 09:16:26 PDT
(In reply to Andy VanWagoner from comment #4) > I've started by simply updating all the existing files to v55.1. Should I > add headers we don't use yet, as long as they are part of the C API? I think this would be fine since we already support 55.1. Of course, use of newer APIs will need the same gymnastics to avoid breaking older ports. Could you also update the READMEs to avoid the confusing w.r.t. 10.4? It seems the issue it mentions is here to stay, not a temporary thing as it makes it sound.
Comment 7 JF Bastien 2017-04-24 20:18:59 PDT
Comment 8 Andy VanWagoner 2017-04-24 20:44:02 PDT
Comment 9 JF Bastien 2017-04-24 21:48:53 PDT
OK this looks good. iOS-sim fails seem unrelated. I'll revert if anything breaks, but this seems pretty low risk. Thanks for taking this on :)
Comment 11 WebKit Commit Bot 2017-04-24 22:19:10 PDT
Comment on attachment 308046 [details] Patch Clearing flags on attachment: 308046 Committed r215722: <http://trac.webkit.org/changeset/215722>
Comment 12 WebKit Commit Bot 2017-04-24 22:19:11 PDT
All reviewed patches have been landed. Closing bug.