Skip to content

[build] Explicitly link against icudata #3118

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

Closed
wants to merge 1 commit into from
Closed

[build] Explicitly link against icudata #3118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ if(HAS_LIBDISPATCH_API)
find_package(dispatch CONFIG REQUIRED)
endif()

find_package(ICU COMPONENTS uc i18n REQUIRED)
find_package(ICU COMPONENTS uc i18n data REQUIRED)

include(SwiftSupport)
include(GNUInstallDirs)
Expand Down
3 changes: 2 additions & 1 deletion CoreFoundation/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,8 @@ endif()
if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
target_link_libraries(CoreFoundation PRIVATE
ICU::uc
ICU::i18n)
ICU::i18n
ICU::data)
Copy link
Member

@compnerd compnerd Dec 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This seems wrong. The tests in fact show that this is incorrect - the linking was sufficient. The problem is that the data is not found at runtime. This should be handled via a DT_RUNPATH or DT_RPATH in TestFoundation. This potentially can break the Windows builds. In fact, if nothing else, this doesn't fix the issue - this adds a DT_NEEDED on the icudata, but that won't help.

Copy link
Contributor Author

@Azoy Azoy Dec 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

In my testing, the icudataswift library was not being linked correctly at all in CoreFoundation, and TestFoundation is just an example executable that won't be able to run without the correct path for the data library. The data library seems to be a dependency of i18n and uc and running ldd libFoundation.so returns the following:

libicuucswift.so.65 => /swift/build/Ninja-RelWithDebInfoAssert/swift-linux-x86_64/lib/swift/linux/x86_64/libicuucswift.so.65 (0x00007fc5f7a03000)
libicui18nswift.so.65 => /swift/build/Ninja-RelWithDebInfoAssert/swift-linux-x86_64/lib/swift/linux/x86_64/libicui18nswift.so.65 (0x00007fc5f76d0000)
libicudataswift.so.65 => not found

I don't think this is something specific to TestFoundation unless we expect each executable depending on Foundation to pass in DT_RPATH. Also, this was tested with other changes committed to the dropping ICU PR that make this work. Yes, this alone does not solve the issue, but this and the other new changes do at least solve the problem for TestFoundation.

Copy link
Member

Choose a reason for hiding this comment

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

If CoreFoundation itself does not rely on libicudata but only indirectly through libicuuc, I think Saleem is right that we shouldn't add that dependency here. What may fix it instead is to add $ORIGIN to the DT_RPATH of the libicu libraries that we build for linux, which is also needed to fix the currently broken plutil executable that we ship from this repo.

We should've been doing that already and I think it'll fix both that plutil issue and this newly uncovered ICU issue.

endif()

if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
Expand Down