Skip to content

[cmake] Fix ICU static linking #3133

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 2, 2022
Merged

[cmake] Fix ICU static linking #3133

merged 1 commit into from
Feb 2, 2022

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Jan 26, 2022 •
edited
Loading

When linking Foundation, we were only passing -licui18nswift as a static library, but it depends on icuucswift and icudataswift, so make sure we static link against those as well.

Resolves: https://bugs.swift.org/browse/SR-15770 and rdar://problem/87991595

@Azoy Azoy requested review from compnerd and drexin January 26, 2022 20:06
@Azoy
Copy link
Contributor Author

Azoy commented Jan 26, 2022

@swift-ci please build toolchain linux platform

@parkera parkera requested a review from millenomi January 26, 2022 20:49
@Azoy
Copy link
Contributor Author

Azoy commented Jan 26, 2022

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jan 26, 2022

@swift-ci please build toolchain linux platform

compnerd
compnerd requested changes Jan 26, 2022
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.

I think that we should be handling these similar to the icuin case - this will break builds when used outside of the host.

@Azoy Azoy mentioned this pull request Jan 27, 2022
Merged
@Azoy
Fix ICU static linking
602c081 
lookup name vs. hardcoding it
@Azoy Azoy force-pushed the fix-icu-static-linking branch from 93b318e to 602c081 Compare January 27, 2022 18:37
@Azoy
Copy link
Contributor Author

Azoy commented Jan 27, 2022

I've updated the PR to lookup the library name like icui18n is doing, but it requires explicitly passing ICU_DATA_LIBRARY in the build-script-impl, so I've done that here: swiftlang/swift#41041

@Azoy
Copy link
Contributor Author

Azoy commented Jan 27, 2022

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@compnerd
Copy link
Member

compnerd commented Feb 1, 2022

Please test with following PRs:
swiftlang/swift#41115

@swift-ci please test macOS platform

@millenomi millenomi merged commit 0abdcc4 into swiftlang:main Feb 2, 2022
@Azoy Azoy deleted the fix-icu-static-linking branch February 2, 2022 19:44
Azoy pushed a commit to Azoy/swift-corelibs-foundation that referenced this pull request Feb 7, 2022
@millenomi @Azoy
Merge pull request swiftlang#3133 from Azoy/fix-icu-static-linking
dfc3998 
[cmake] Fix ICU static linking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millenomi millenomi millenomi approved these changes

@compnerd compnerd compnerd approved these changes

@stephentyrone stephentyrone stephentyrone approved these changes

@drexin drexin Awaiting requested review from drexin

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

Successfully merging this pull request may close these issues.
5 participants
@Azoy @MaxDesiatov @compnerd @millenomi @stephentyrone