[build] Explicitly link against icudata #3118
Conversation
|
@swift-ci please test |
|
Do you know if this needs to be merged simultaneously with swiftlang/swift#40340, or can this be merged first? |
|
CC @compnerd |
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Once the change is fixed up, this should be fine to merge first on its own, it will have no detrimental impact. |
|
Closing because we found a solution that doesn't require this. |
In preparation for swiftlang/swift#40340, explicitly link against icudata.