Skip to content

[cmake] Find ICU in CoreFoundation. #1746

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
Nov 7, 2018

Conversation

drodriguez
Copy link
Contributor

ICU is also included in CoreFoundation, but the include paths were
not used, which meant that the headers were probably the system
headers, if they were there. This might have work in Ubuntu, but
might not work in other systems with older ICUs.

The patch passes the ICU_ROOT from the top level CMakeList.txt into
the CoreFoundation one and performs a find_package to find the right
include path.

/cc @compnerd

@compnerd
Copy link
Member

compnerd commented Nov 3, 2018

@spevans already fixed this. We should however pass down the ICU_ROOT parameter as you have done. Can you please rebase this?

@spevans
Copy link
Contributor

spevans commented Nov 3, 2018

Should probably keep the addition of "${CMAKE_CURRENT_SOURCE_DIR}/../cmake/modules" to CMAKE_MODULE_PATH as well and get rid of the duplicate of FindICU.cmake that I made

The patch passes the ICU_ROOT from the top level CMakeList.txt into
the CoreFoundation one and performs a find_package to find the right
include path.

It also uses the Find ICU script from the top level, instead of the
duplicated script (which has been removed).
@drodriguez drodriguez force-pushed the cmake-icu-to-core-foundation branch from b421c69 to 212e3ab Compare November 4, 2018 02:06
@drodriguez
Copy link
Contributor Author

I hope this was what you both were intending: I removed the duplicated script and I passed ICU_ROOT instead of ICU_INCLUDE_DIR.

@compnerd
Copy link
Member

compnerd commented Nov 6, 2018

@swift-ci please test and merge

1 similar comment
@millenomi
Copy link
Contributor

@swift-ci please test and merge

@swift-ci swift-ci merged commit 042b6cb into swiftlang:master Nov 7, 2018
@milseman
Copy link
Member

milseman commented Nov 8, 2018

This seems to be causing an issue on PR testing:

https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04/6459/changes

18:51:43 /home/buildnode/jenkins/workspace/oss-swift-incremental-RA-linux-ubuntu-14_04/buildbot_incremental/swiftpm-linux-x86_64/x86_64-unknown-linux/release/swift-build-stage1: symbol lookup error: /home/buildnode/jenkins/workspace/oss-swift-incremental-RA-linux-ubuntu-14_04/buildbot_incremental/swiftpm-linux-x86_64/.bootstrap/bin/../lib/swift/linux/libFoundation.so: undefined symbol: UCNV_FROM_U_CALLBACK_STOP_52

@drodriguez drodriguez deleted the cmake-icu-to-core-foundation branch July 16, 2019 22:39
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.

6 participants