Bug 170997 - Clean up ICU headers
Summary: Clean up ICU headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy VanWagoner
URL:
Keywords:
Depends on: 169458
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-19 09:42 PDT by Andy VanWagoner
Modified: 2017-04-24 22:19 PDT (History)
12 users (show)

See Also:


Attachments
Patch (597.66 KB, patch)
2017-04-24 19:22 PDT, Andy VanWagoner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 6 Andy VanWagoner 2017-04-24 19:22:06 PDT
Created attachment 308046 [details]
Patch
Comment 7 JF Bastien 2017-04-24 20:18:59 PDT
Comment on attachment 308046 [details]
Patch

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

Looks good, will wait for bots to be happy too.

> Source/JavaScriptCore/icu/unicode/platform.h:127
> + *  http://www.chromium.org/nativeclient

lol

> Source/JavaScriptCore/icu/unicode/uchar.h:42
> +#define U_UNICODE_VERSION "7.0"

Does this indirectly make us support new things through the system library? i.e. new things in the .h opt us into thing the library supported but were off due to some header-declared feature set? I don't know how most of ICU works :)
Comment 8 Andy VanWagoner 2017-04-24 20:44:02 PDT
(In reply to JF Bastien from comment #7)
> Comment on attachment 308046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308046&action=review
> 
> Looks good, will wait for bots to be happy too.
> 
> > Source/JavaScriptCore/icu/unicode/platform.h:127
> > + *  http://www.chromium.org/nativeclient
> 
> lol
> 
> > Source/JavaScriptCore/icu/unicode/uchar.h:42
> > +#define U_UNICODE_VERSION "7.0"
> 
> Does this indirectly make us support new things through the system library?
> i.e. new things in the .h opt us into thing the library supported but were
> off due to some header-declared feature set? I don't know how most of ICU
> works :)

Considering the system headers are used on most platforms, including iOS, Apple TV, and Apple Watch, I don't think this opts us into anything in particular. There are a few defines that can cause the build to fail if left their original value, and so I preserved the edits we had in them and documented it.

This particular define is actually the following in my iPhoneOS.platform version:

#define U_UNICODE_VERSION "8.0"
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 10 JF Bastien 2017-04-24 21:49:03 PDT
Comment on attachment 308046 [details]
Patch

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