Skip to content

[SE-0211] Emoji properties require ICU 57 or later #19209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 15, 2018

Conversation

benrimmington
Copy link
Contributor

Availability of emoji properties should be platform versions which have a /usr/lib/libicucore.dylib derived from ICU 57 or later.

ICU macOS iOS tvOS watchOS
55 10.11 9 9 2
57 10.12 10 10 3
59 10.13 11 11 4

These platform versions are based on the unicode/uchar.h and unicode/uvernum.h headers of:

(I didn't check as far back as macOS 10.9 or iOS 7).

This pull request is a follow-up to:

@benrimmington
Copy link
Contributor Author

Cc: @allevato @milseman

@allevato
Copy link
Member

When I last brought this up, @milseman in this comment felt that the properties shouldn't be gated with availability annotations in this comment.

Reading over it again, I probably disagree with the first sentence "The properties are available"; before the version of ICU that implemented the corresponding version of the standard, the properties aren't available, and there's no way to use them on an older OS that produces the right result. That's distinct from the grapheme cluster behavior alluded to in String.count, where the concept exists but its behavior changes.

But I'd like to hear if Michael has any other concerns about this.

Sadly for Linux, there's no way to gate the properties based on version there, but that's ok, because the long-term solution is to bundle ICU with stdlib to ensure that it always contains what we need.

@milseman
Copy link
Member

Right, if the API isn't even available, then we need to disable it on that platform version. IIRC, I was referring to properties that were available but gave a different answer than what we were testing for.

@allevato
Copy link
Member

I regret not being clearer then in my original comment on the other PR. 🙂 In that case, I agree completely—let's do this, because the properties don't exist at all on the OS versions before those listed above.

Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor test change requested, LGTM

@milseman
Copy link
Member

@swift-ci please test and merge

@benrimmington
Copy link
Contributor Author

  • Build #1196: Automatically restarting the build, due to invalid sha
  • Build #1198: ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job.

@jrose-apple
Copy link
Contributor

@swift-ci Please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - ef1a6a4b7f4ddaba72b18b978266fce32b6277ed

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - ef1a6a4b7f4ddaba72b18b978266fce32b6277ed

@benrimmington
Copy link
Contributor Author

This is ready to merge (all checks have passed).

@milseman milseman merged commit 0e17e76 into swiftlang:master Sep 15, 2018
@benrimmington benrimmington deleted the emoji_gate_2 branch September 15, 2018 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants