Skip to content

plutil: Add a specific rpath for plutil and use it when linking. #2307

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
Jun 25, 2019

Conversation

spevans
Copy link
Contributor

@spevans spevans commented May 26, 2019

  • plutil requires an rpath of '$ORIGIN/../lib/swift/${swift_os}'
    so set this specificaly for plutil.

  • Add a test for running plutil, which relies on LD_LIBRARY_PATH
    being set when the tests are run to point to the build directories.

@spevans
Copy link
Contributor Author

spevans commented May 26, 2019

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented May 26, 2019

@swift-ci test macos

@spevans
Copy link
Contributor Author

spevans commented May 26, 2019

@swift-ci test macos

-L${CMAKE_CURRENT_BINARY_DIR}
-L;${FOUNDATION_PATH_TO_LIBDISPATCH_BUILD}/src
Copy link
Member

Choose a reason for hiding this comment

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

This may add an empty /src to the library search path. @millenomi - is building Foundation with Dispatch something which is desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

${FOUNDATION_PATH_TO_LIBDISPATCH_BUILD}/src was in the libdispatch_ldflags as a RUNPATH, so this is still needed for the link to succeed, but the RUNPATH itself isnt.

@spevans
Copy link
Contributor Author

spevans commented May 27, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented May 28, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test macos

3 similar comments
@spevans
Copy link
Contributor Author

spevans commented May 28, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test macos

@spevans
Copy link
Contributor Author

spevans commented May 28, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test macos

@spevans
Copy link
Contributor Author

spevans commented May 28, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test macos

@millenomi
Copy link
Contributor

I'm really not sure we should ship RPATHs in final executables. @jrose-apple how does the rest of the setup deal with dependencies?

@jrose-apple
Copy link
Contributor

:-/ I think this is the right answer on Linux where we don't have an equivalent of Mach-O's @executable_path.

@jrose-apple
Copy link
Contributor

($ORIGIN is that equivalent but AFAIK you can't link directly to something via $ORIGIN; you have to go through @rpath.)

Alternately, static-link all the things, but that's not a good answer either.

@spevans
Copy link
Contributor Author

spevans commented May 28, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test macos

@spevans
Copy link
Contributor Author

spevans commented May 30, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57
swiftlang/swift-integration-tests#58

@swift-ci please test

@jrose-apple
Copy link
Contributor

I don't think the cross-PR testing supports multiple PRs from the same repo. :-(

@spevans
Copy link
Contributor Author

spevans commented May 30, 2019

Yeah, I wanted to test swiftlang/swift-integration-tests#58 as it looks like tests can't be run directly in the swift-integration-tests repo (or maybe I dont have access).

@spevans
Copy link
Contributor Author

spevans commented May 31, 2019

@millenomi We still need to add RPATHs whilst we are installing from a tar since the install could be located anywhere, so paths relative to $ORIGIN are needed. The only alternative would be to create proper packages and then add the swift lib directory as a config file in /etc/ld.so.conf.d/

Other binaries ship with even worse paths, here is swift-build:

chrpath  -l ~/swift-DEVELOPMENT-SNAPSHOT-2019-01-15-a-ubuntu18.04/usr/bin/swift-build
/home/spse/swift-DEVELOPMENT-SNAPSHOT-2019-01-15-a-ubuntu18.04/usr/bin/swift-build: RUNPATH=/home/buildnode/jenkins/workspace/oss-swift-package-linux-ubuntu-18_04/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN:$ORIGIN/../lib/swift/linux:/home/buildnode/jenkins/workspace/oss-swift-package-linux-ubuntu-18_04/build/buildbot_linux/swiftpm-linux-x86_64/.bootstrap/lib/swift/linux:$ORIGIN/../lib/swift/pm/llbuild:/home/buildnode/jenkins/workspace/oss-swift-package-linux-ubuntu-18_04/build/buildbot_linux/llbuild-linux-x86_64/lib

The only two ways I can think of to clean this up would be

  1. Add an option to swiftc to disable adding RPATHs by default - A bit heavy if its only binaries built by the swift install that have bad paths but might be useful in other cases.
  2. Install chrpath and use it to prune the paths after building.

@jrose-apple
Copy link
Contributor

jrose-apple commented May 31, 2019

That option is coming in swiftlang/swift#24787, by the way, though it isn't hooked up to Linux yet. (I haven't landed it because it's immediately going to cause merge conflicts with the Apple-internal branches and I need to be ready to resolve them.)

@spevans
Copy link
Contributor Author

spevans commented May 31, 2019

That -no-stdlib-rpath options look ideal, I will keep an eye on the PR and then update it for Linux if anything needs to be done. I think that will allow us clean up a lot of RPATHs in the corelibs.

- plutil requires an rpath of '$ORIGIN/../lib/swift/${swift_os}'
  so set this specificaly for plutil.

- Add a test for running plutil, which relies on LD_LIBRARY_PATH
  being set when the tests are run to point to the build directories.
@spevans spevans force-pushed the pr_fix_plutil_rpath branch from bd445ee to c76eb37 Compare June 22, 2019 17:32
@spevans
Copy link
Contributor Author

spevans commented Jun 22, 2019

@swift-ci test linux

@spevans
Copy link
Contributor Author

spevans commented Jun 22, 2019

Please test with following pull request:
swiftlang/swift-integration-tests#57

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Jun 25, 2019

@swift-ci test linux

@spevans spevans merged commit 8d99ac9 into swiftlang:master Jun 25, 2019
@compnerd
Copy link
Member

@drodriguez
Copy link
Contributor

I don't know what's the configuration of the Linux CI machines, and if it is easy to change them, but if libBlocksRuntime.so was not installed in the system, many of these problems would surface earlier when testing Linux.

@spevans
Copy link
Contributor Author

spevans commented Jun 26, 2019

Ive unistalled the blocksRuntime package on my test VM now so it should catch anymore of these problems if I make them in future PRs.

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.

5 participants