[cmake] Add runpath for libBlocksRuntime.so #2246
Conversation
|
Which binary is this setting the I don't think its needed because its covered by the existing |
|
In my machine When I tried to run the The change allows (This is unrelated to Android, this is a Linux machine) |
|
Oh I see, it works normally on Linux because it is using the system installed I guess it depends if we always want to use the one built as part of libdispatch and ignore the system one. |
|
Your choice to merge or not. It was something I found in my setup. The Android runpath problem is more convoluted. |
|
This really is going down the wrong path. We should be reducing |
|
If this PR is just adjusting the |
|
@spevans we should never be using the system libBlocksRuntime (see the discussion on https://bugs.swift.org/browse/SR-9347) so anything still relying on it is a bug. Whether we use RPATHs or LD_LIBRARY_PATH to do that doesn't matter too much for a test program, but for binaries that are installed onto the system we should prefer to use RPATH. We should also not be including build tree RPATHs in the installed binaries. In cmake that should be possible with https://cmake.org/cmake/help/v3.4/variable/CMAKE_INSTALL_RPATH.html, which will cause the cmake |
|
@kevints I'm okay with RPATHs in this instance because these are test binaries which won't be distributed. |
|
The variable is also used while compiling I can (gladly) abandon this PR, but the rest of the RPATHs are still pointing to similar paths. |
|
@drodriguez Which binaries are affected by this PR, is it just |
|
This PR modified That variable is used while building One can move this change to another variable, but that doesn’t change the fact that the variable was applying the RPATH to all the artifacts and pointing to the build directory. |
|
Hmm. @spevans is correct that we shouldn't ship with build machine paths for at least some of the products — |
|
You could just add the extra directory into Would this change allow the removal of the |
|
I can change this PR to work for Linux at least. Android is a special creature. Maybe '$ORIGIN' works there, but I have to check. |
|
I think this should work in anybody Linux installation. Only the test binaries have extra rpaths. Also, Android is left without them for the time being. I will do it in another commit. @swift-ci please test |
|
@swift-ci please test linux |
They are still being included in the official releases for linux, as I noted to @drodriguez in the comments for a different pull a couple months ago. I see that it's still the case for libXCTest.so and other shared libraries in the 5.0 release, along with binaries like plutil, swift-build, or swift-package: |
|
Yes. That discussion points to the source of all those invalid runpaths, which come from the compiler itself embedding a full path of the build machine. Very nice when you are running those binaries in the build machine, useless for other cases. This PR only deals with the test binaries, when run against the non-installed libraries. The new commit just fixes some conflicts, but doesn’t try to do anything more. I think this PR might still be interesting by itself, specially since it deals with testing the right setup. |
|
@swift-ci please test |
|
@drodriguez So does this PR fix the Note that currently So that also needs fixing.
So we need to scrub those as well. We should really add a test into the I did open https://bugs.swift.org/browse/SR-9315 about the |
Correct. It removes the dispatch paths from
That explain why I’m seeing different results from @buttaface above. If you want to create that PR, I will be happy to accept it. Notice that right now, I think
I think that path is added by the compiler itself, and only in Linux. https://github.com/apple/swift/blob/ad79bbaf860a3281b72d9f775b7cbfde57e30639/lib/Driver/UnixToolChains.cpp#L199-L205. It has an obvious |
|
@swift-ci please test |
|
@swift-ci please test linux |
|
I was just having a re-read through this PR and noticed 2 other changes that need to be made: For platforms where the Also, |
|
@swift-ci please test |
|
I hope I captured all the feedback with the last version (environment variables removed, complete the rpaths with XCTest where needed). And since we are here, I don’t think (Additionally it has the rpath to the build tree even after installing, but as I have said above, that's something the compiler does). |
|
Ive done a fix for |
|
@swift-ci please test |
|
@spevans so I think removing I don’t know if it is worth it to copy the libraries to a temporal directory for testing LLDB or that will become untenable over time. |
|
I wouldnt bother about copying the libraries I would just leave the path in for now. In future we might be able to set an |
|
@swift-ci please test |
|
@drodriguez Do you want to rebase this and get it merged? |
drodriguez
force-pushed
the
more-runpaths
branch
from
e45580d to
5b8e349
Compare
|
@swift-ci please test Linux platform |
|
@swift-ci test linux |
Dispatch build leaves artifacts in both the root directory and the src directory, but only the src directory was being added to the runpath, so the libBlocksRuntime.so library might not have been found. Additionally, remove the rpath flags from libdispatch_ldflags, which are used to compile Foundation, but will point to the build directory results. Add those flags instead to a list used for the test binaries. In the end Foundation ends up with a path of `$ORIGIN`; TestFoundation has paths to Foundation, to Dispatch and BlocksRuntime, and to XCTest; while xdgTestHelper points to Foundation, and Dispatch and BlocksRuntime. Extract a small function for adding rpaths to a list, to make the rpath easier to read. Also, remove Android from the rpath flags, since Android will not work with the build tree rpaths, and will need its own solution. For the time being, do not set any rpath.
drodriguez
force-pushed
the
more-runpaths
branch
from
5b8e349 to
16f5cef
Compare
|
Rebased to avoid conflicts with @swift-ci please test Linux platform |
| file(TO_CMAKE_PATH "${FOUNDATION_PATH_TO_LIBDISPATCH_BUILD}" FOUNDATION_PATH_TO_LIBDISPATCH_BUILD) | ||
| # NOTE: the following two are for testing LLDB and others, but should be removed | ||
| append_linker_rpath(Foundation_RPATH ${FOUNDATION_PATH_TO_LIBDISPATCH_BUILD}/src) | ||
| append_linker_rpath(Foundation_RPATH ${FOUNDATION_PATH_TO_LIBDISPATCH_BUILD}) |
There was a problem hiding this comment.
We shouldn't be adding build paths to libFoundation.so we should fix lldb instead
There was a problem hiding this comment.
That NOTE is the result of our conversation on May 31st: #2246 (comment)
There was a problem hiding this comment.
@spevans: Some decision around this? I can try to fix it in LLDB, but I would l love to not have to modify it in there before merging this. Thanks!
|
For some reason https://ci.swift.org/view/Pull%20Request/job/swift-corelibs-foundation-PR-Linux/3426 haven't reported back here. Firing it up again. @swift-ci please test Linux platform |
|
Any interest in getting this in? I made a similar change to remove the libdispatch build directory runpath when packaging up the Swift toolchain for Android. |
|
This CMake config has changed so much over the past year that this pull doesn't apply anymore. It will have to be redone, either here or in a separate pull. |
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
Dispatch build leaves artifacts in both the root directory and the src
directory, but only the src directory was being added to the runpath, so
the libBlocksRuntime.so library might not have been found.
Additionally, remove the rpath flags from libdispatch_ldflags, which are
used to compile Foundation, but will point to the build directory
results. Add those flags instead to a list used for the test binaries.
In the end Foundation ends up with a path of
$ORIGIN; TestFoundationhas paths to Foundation, to Dispatch and BlocksRuntime, and to XCTest;
while xdgTestHelper points to Foundation, and Dispatch and
BlocksRuntime.
Extract a small function for adding rpaths to a list, to make the rpath
easier to read.
Also, remove Android from the rpath flags, since Android will not work
with the build tree rpaths, and will need its own solution. For the time
being, do not set any rpath.