Skip to content

[Build System: CMake] Update the default install_name_dir to /usr/lib/swift for the standard library and overlays. #24382

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 4 commits into from
May 13, 2019

Conversation

Rostepher
Copy link
Contributor

rdar://46736483

Ross Bayer added 4 commits April 29, 2019 16:46
…ary install_name_dir. Those libraries are not going to be installed in /usr/lib/swift and thus need to be controlled via a separate mechanism.
…ary to DARWIN_INSTALL_NAME_DIR which better explains that this argument only controls the install_name_dir for Darwin platforms.
@Rostepher
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Is there some private changes that require this to be a parameter? I really want to move towards using CMake rather than the custom variant that we keep building and tearing down. Can we not just use the global variable in the core add_library_single rather than passing it down all the way?

@Rostepher
Copy link
Contributor Author

@swift-ci please smoke test Linux platform

@Rostepher
Copy link
Contributor Author

@compnerd I'm not sure what global variable you are referencing. We intend to change this install_name_dir for the standard library and overlays, but not for the private libraries that aren't installed. If there's a cleaner way to do this then I'm more than happy to do so.

@compnerd
Copy link
Member

compnerd commented May 1, 2019

@Rostepher SWIFT_DARWIN_STDLIB_INSTALL_NAME_DIR is a global variable, you can just reference it in the innermost function rather than pass it along from the top level

@Rostepher
Copy link
Contributor Author

@compnerd I realize that and we are using that variable for all the public stdlib content. The private content that isn't actually installed now uses SWIFT_DARWIN_STDLIB_PRIVATE_INSTALL_NAME_DIR which I manually pass along via this variable.

@compnerd
Copy link
Member

compnerd commented May 2, 2019

Oh, I get it now. You are using that to override the value. I wonder if we can infer the private-ness/public-ness and avoid passing this down. But, if there isn't a way to do that, I imagine that you need this right now, so we can deal with rewriting this later.

@Rostepher
Copy link
Contributor Author

I thought about trying to infer the value, but it seemed messier to me to have even more magic inside add_swift_target_library. Instead I opted to have a more general purpose argument that would allow us to control this.

@compnerd
Copy link
Member

compnerd commented May 2, 2019

I think that we should reconsider that. We need to move away from the custom argument thing so that we can adopt regular CMake best practices. The current mechanism really breaks cross-compilation and we need to strip out the changes to make that work properly.

@Rostepher Rostepher merged commit 23a868e into swiftlang:master May 13, 2019
@Rostepher Rostepher deleted the usr-lib-install-name-dir branch May 13, 2019 22:05
dan-zheng added a commit to dan-zheng/swift that referenced this pull request Jun 10, 2019
`install_name_dir` for the standard library has been changed from
@rpath to /usr/lib/swift in swiftlang#24382.

This patch may resolve "Library not loaded: /usr/lib/swift/libswiftTensorFlow.dylib"
linker issues: tensorflow/swift-apis#136
dan-zheng added a commit that referenced this pull request Jun 12, 2019
…25328)

`install_name_dir` for the standard library has been changed from
@rpath to /usr/lib/swift in #24382.

This patch resolves linker issues on macOS:
"Library not loaded: /usr/lib/swift/libswiftTensorFlow.dylib"
See tensorflow/swift-apis#136 for more information.
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.

3 participants