Skip to content

[build][android] set INSTALL_RPATH properly for shared libraries #30235

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
Apr 18, 2020

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Mar 5, 2020

This finally allows removing the toolchain/SDK rpaths that are currently automatically added when compiling Swift shared libraries and executables on non-Darwin platforms. Also, Similar flag added in #30430 instead. Remove unnecessary checks for Android's INSTALL_RPATH, now that the CMake setup has been refactored to separate host and target library configuration.

I've backported this swiftc flag to the Swift 5.1.4 package script for Android and then use it to avoid adding the toolchain rpath for Foundation and other Swift shared libraries in the toolchain.

That would be useful for the official releases for linux too, here's what the 5.1.4 release and the latest 5.2 snapshot show:

> readelf -d swift-5.1.4-RELEASE-ubuntu18.04/usr/lib/swift/linux/libFoundation.so | ag runpath
0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.1-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:/home/buildnode/jenkins/workspace/oss-swift-5.1-package-linux-ubuntu-18_04/build/buildbot_linux/libdispatch-linux-x86_64/src:$ORIGIN]
> readelf -d swift-5.2-DEVELOPMENT-SNAPSHOT-2020-03-03-a-ubuntu18.04/usr/lib/swift/linux/libFoundation.so | ag runpath
0x000000000000001d (RUNPATH)            Library runpath: [/home/buildnode/jenkins/workspace/oss-swift-5.2-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:/home/buildnode/jenkins/workspace/oss-swift-5.2-package-linux-ubuntu-18_04/build/buildbot_linux/libdispatch-linux-x86_64:$ORIGIN]

@brentdax, you modified some of this rpath config last year, let me know what you think. @drodriguez, let me know about the Android part. @tachoknight, this should be useful for all Swift packagers.

endif()
set_target_properties("${target}"
PROPERTIES
INSTALL_RPATH "$ORIGIN")
Copy link
Member Author

Choose a reason for hiding this comment

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

No checks needed now that this is only set for host libraries on Android.

@finagolfin
Copy link
Member Author

Ping, this is a small pull that would help get rid of unnecessary rpaths in Swift binaries and shared libraries on ELF platforms.

@drodriguez
Copy link
Contributor

I don't know about the logic of the changes (if they are right or not), but it doesn't break the Android builds for me, so I hope it doesn't break them in CI.

But this changes the Linux builds slightly, in a way similar to what the DarwinToolchain.cpp seems to do, so it will affect more than Android. If anyone has any objections, or worries, it will be nice to know them before this moves further.

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 8f0f6067a14331e3454edf1e307a447e86c5c4a0

@finagolfin
Copy link
Member Author

finagolfin commented Mar 16, 2020

All this pull does is allow those on ELF platforms, ie linux, Android, BSD, and so on, to disable the host toolchain rpath from being automatically added to Swift shared libraries by manually adding the -no-stdlib-rpath flag, which is already available and working on macOS. Given the example in the OP, where the build/CI toolchain rpath has been wrongly added to the official release of the Foundation shared library for linux and now is invoked on any other host on which that official linux toolchain Foundation is run, this flag would optionally allow disabling adding that toolchain rpath, as I show in the OP for Foundation with my linked Android Swift 5.1.4 packaging script patch.

Since this merely enables an optional flag that is already defined for macOS on ELF platforms too, it should have no effect on the pre-existing tests, builds, and so on. It merely gives those who know what they're doing a flag they can use to disable that toolchain rpath from being added to Swift ELF shared libraries, just like on macOS.

@finagolfin
Copy link
Member Author

Oh, the macOS CI failure seems spurious.

Host libraries will likely all need ORIGIN set, whereas only set it for
target libraries that will be packaged with a native toolchain on Android.
@finagolfin finagolfin changed the title [Driver] Wire up -no-stdlib-rpath for linux/Unix and properly set INSTALL_RPATH for Android [build][android] set INSTALL_RPATH properly for shared libraries Mar 18, 2020
@finagolfin
Copy link
Member Author

Saleem just merged #30430 which adds -no-toolchain-stdlib-rpath instead, so I removed the -no-stdlib-rpath commit in this pull. All that's left is the simple changes to the INSTALL_RPATH.

I haven't added the native sysroot location, ie something like /usr/lib/swift/linux above instead of just $ORIGIN alone, to the install rpath, as I believe that's a bad idea and actually shouldn't be done for linux either. It is very likely that the versions of the different target libraries from the separate rpaths wouldn't be the same and it would cause more confusion than help. I can remove those for linux/cygwin too in this pull, if the reviewers agree.

@drodriguez
Copy link
Contributor

@swift-ci please test and merge

@finagolfin
Copy link
Member Author

Hmm, it didn't go through, but that's probably for the best, as I think I'll have to change this pull a bit. Hold off on merging for now, I'll update this pull in a day or two.

@finagolfin
Copy link
Member Author

I don't think I'll add anything else after all, please go ahead and merge.

@drexin
Copy link
Contributor

drexin commented Apr 16, 2020

@swift-ci smoke test

@finagolfin
Copy link
Member Author

Linux failure is spurious.

@drexin
Copy link
Contributor

drexin commented Apr 16, 2020

@swift-ci smoke test linux

@finagolfin
Copy link
Member Author

Ready to go? This only affects Android, so very safe to merge.

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.

4 participants