-
Notifications
You must be signed in to change notification settings - Fork 442
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
Enables runtime linking for darwin platforms #766
Conversation
Seems related to #540 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think this looks good but do you think you could Bazelify the example you have and add it to examples/runtime_linking
or something so we can be sure to avoid potential regressions in the future 😅 🙏?
My intention is to fix #621 and so this will be bazel-ified and checked by CI by those targets. The difficulty with adapting the example I have linked here is that it requires using an unreleased version of Bazel so that --incompatible_macos_set_install_name can be set. Otherwise I'd need to write a new rule that post processes cc_library targets to set the install name of the dylib, which I'm not sure is worth the effort. I wanted to submit this PR independently because it is preventing me from linking against a vulkan dylib that I am cc_importing in my project, ahead of a full fix for #621. I can also wait to submit a PR with a full fix for #621, which involves:
This will specifically solve the issues with the tests in @examples/ffi/rust_calling_c/BUILD.bazel on macOS. To resolve the issues with the tests in @examples/ffi/rust_calling_c/simple/BUILD.bazel, we also need to resolve #741, which I believe should be resolved by using the dynamic library specified by the cc toolchain. I also plan to take a look at this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me - maybe worth a comment above the if statement explaining that darwin doesn't work before (link to PR/commit) and that it's known not to work <4.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thanks! I agree with #766 (review) but otherwise looks good and am excited to see this issue getting fixed 😄
Runtime linking with rpaths works correctly on darwin with only this change, for dylibs that have the correct extension (dylib) and the correct rpath configuration. Runtime linking against dynamic libraries created by bazel targets remains broken, due to them not having install names set correctly. That is fixed by using --incompatible_macos_set_install_name after bazelbuild/bazel#13427.
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much!
Runtime linking with rpaths works correctly on darwin with only this change, for dylibs that have the correct extension (dylib) and the correct rpath configuration. Runtime linking against dynamic libraries created by bazel targets remains broken, due to them not having install names set correctly. That is fixed by using --incompatible_macos_set_install_name after bazelbuild/bazel#13427.
I have attached a zip file with a simple example workspace demonstrating this.
example.zip
Run:
(cd lib; ./build.sh) bazel run //:main