Skip to content

Add ICU version check to ensure minimum of version 61 is used. #1866

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 25, 2019
Merged

Add ICU version check to ensure minimum of version 61 is used. #1866

merged 1 commit into from
Feb 25, 2019

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Jan 30, 2019

No description provided.

@spevans
Copy link
Contributor Author

spevans commented Jan 30, 2019

Note this should currently fail CI testing due to SR-9699

@spevans
Copy link
Contributor Author

spevans commented Jan 30, 2019

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented Jan 30, 2019

cc @compnerd What ICU version do you have with Windows? Or are you compiling it from the ICU source tree as part of the build?

@compnerd
Copy link
Member

@spevans - I'm not compiling it from source, for now, I'm using 63.1.

@spevans
Copy link
Contributor Author

spevans commented Jan 31, 2019

@compnerd FYI, there are some tests that are broken with ICU63, mostly to do with currencyCode, so if you see those tests failing its not a windows issue but something we need to fix in the tests anyway.

@compnerd
Copy link
Member

Thanks for the heads up, I'm still trying to get Foundation to build, let alone test though. Pretty close, NSData, NSFileManager (which is why I am anxious to get the NSData work in, so all the focus goes to the NSFileManager).

@spevans
Copy link
Contributor Author

spevans commented Feb 15, 2019

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented Feb 15, 2019

@compnerd @millenomi Any objections to merging this to prevent future ICU nonsense?

compnerd
compnerd reviewed Feb 24, 2019
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

LGTM (minus the one trivial bug)

CoreFoundation/CMakeLists.txt Outdated
if(ICU_VERSION VERSION_LESS ${ICU_MINIMUM_VERSION})
message(FATAL_ERROR "ICU version ${ICU_VERSION} is less than required version ${ICU_MINIMUM_VERSION}")
endif()
message(STATUS, "ICU_INCLUDE_DIR: ${ICU_INCLUDE_DIR}")
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the ,.

@spevans spevans force-pushed the pr_check_icu_version branch from 817c301 to 021533b Compare February 25, 2019 07:42
@spevans
Copy link
Contributor Author

spevans commented Feb 25, 2019

@swift-ci test

@spevans spevans mentioned this pull request Feb 25, 2019
Merged
@spevans
Copy link
Contributor Author

spevans commented Feb 25, 2019

@swift-ci test and merge

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented Feb 25, 2019

@swift-ci test and merge

@swift-ci swift-ci merged commit 9432a20 into swiftlang:master Feb 25, 2019
@spevans spevans mentioned this pull request Feb 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@spevans @compnerd @swift-ci