[CMake] Don't install libllbuild in the Swift toolchain now that it's statically linked #870
Conversation
… statically linked Now that libllbuildSwift is statically linked against libllbuild with swiftlang#852, remove the runpath and don't install it. Also, the Android build started failing, so set the appropriate flag for it to work again.
|
@johari, would be good to fix this before the upcoming 5.9 branch, let me know what you think. |
|
cc/ @owenv @jakepetroules @dmbryson and @compnerd |
|
|
||
| if(CMAKE_SYSTEM_NAME STREQUAL Android) | ||
| set(CMAKE_HAVE_LIBC_PTHREAD TRUE) | ||
| endif() |
There was a problem hiding this comment.
I think that this condition isn't exactly the best, but it works for now. We should prefer to figure this out more generally as musl also includes pthread in libc.
| ARCHIVE DESTINATION lib${LLBUILD_LIBDIR_SUFFIX} | ||
| LIBRARY DESTINATION lib${LLBUILD_LIBDIR_SUFFIX} | ||
| RUNTIME DESTINATION bin | ||
| COMPONENT libllbuild) |
There was a problem hiding this comment.
I think that we should nix this part as well. Although it doesn't really matter much for Windows distributions as the packaging of the installer will drop it, it does help keep things clean. Does it make sense to continue distributing the headers? Or should we also be dropping those? Sorry about this mess I created!
There was a problem hiding this comment.
I don't know if the rest is needed or not, haven't looked into it.
| target_link_options(llbuildSwift PRIVATE "SHELL:-no-toolchain-stdlib-rpath") | ||
| set_target_properties(llbuildSwift PROPERTIES | ||
| INSTALL_RPATH "$ORIGIN:$ORIGIN/../../$<LOWER_CASE:${CMAKE_SYSTEM_NAME}>") | ||
| INSTALL_RPATH "$ORIGIN/../../$<LOWER_CASE:${CMAKE_SYSTEM_NAME}>") |
There was a problem hiding this comment.
The diff rendering here is silly, the change is removing $ORIGIN not altering the second path. That seems reasonable as there is nothing from that location that should now be needed.
|
@johari, would you please run the CI and merge? |
|
@SwiftCI please smoke test |
|
I believe it's called @swift-ci. |
|
@swift-ci please smoke test |
|
CI is green, but let's ensure it doesn't break downstream before merging it. Reverting them is not fun. |
|
Ping, can be merged now? |
|
Making a quick inquiry with @bnbarham and we should be good to go. |
4 participants
Now that
libllbuildSwiftis statically linked againstlibllbuildwith #852, remove the runpath and don't install it. Also, the Android build started failing, so set the appropriate flag for it to work again.Previously, we installed
libllbuild.soin the Swift toolchain alongsidelibllbuildSwift.soand set the ELF runpath so it knew it was in the same directory:Now that @compnerd made it link statically, we're wrongly shipping the static libllbuild and still have the now unneeded runpath:
Also, I started seeing cross-compilation failures on my daily Android CI with this move to static linking, which this Android change now fixes, finagolfin/swift-android-sdk#94.