-
Notifications
You must be signed in to change notification settings - Fork 434
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
Incorrect linker flags when using cc_common.link
#2413
Comments
UebelAndre
added
bug
bindgen
Relates to Rust Bindgen: https://rust-lang.github.io/rust-bindgen/
labels
Jan 10, 2024
ghost
mentioned this issue
Jan 11, 2024
Thank you for filling this issue @daivinhtran, I should have done it myself… I've opened a PR to attempt to fix this issue: #2415 I've never used |
daivinhtran
pushed a commit
that referenced
this issue
Feb 16, 2024
### Problem As of now (before this PR), we seem to mix `link_flags` file into using for two purposes. For ``` cc_library( name = "simple", srcs = ["simple.cc"], hdrs = ["simple.h"], linkopts = ["-framework"], ) rust_bindgen_library( name = "simple_bindgen", cc_lib = ":simple", header = "simple.h", ) rust_binary( name = "simple_example", srcs = ["main.rs"], deps = [":simple_bindgen"], ) ``` `rust_bindgen_library` produces a `link_flags` file with ``` -lstatic=simple -C link-args=-framework ``` `-lstatic=simple` is consumed by `rustc` whereas `-framework` is expected to be consumed by an actual linker (either invoked by `rustc` or `cc_common.link`) The flags are then passed to `rustc` command to compile `libsimple_bindgen.rlib`. It does not yield any error because `-lstatic=simple` is correctly used whereas `-framework` is no-op (there is no linker being invoked for producing `rlib`). However, the rustc ***doesn't*** pass `-framework` to the linker that link the `rust_binary` (which is where the cc linkopts matters). Another problem is that this approach is not compatible with `experimental_use_cc_common_link` option which let `cc_common.link` instead of `rustc` invoke the linker. See #2413 ### Solution We're referring "link" as at least two things (which I think what causes problem here): 1. https://doc.rust-lang.org/rustc/command-line-arguments.html#-l-link-the-generated-crate-to-a-native-library 2. https://doc.rust-lang.org/rustc/codegen-options/index.html#linker https://doc.rust-lang.org/rustc/codegen-options/index.html#link-args As proposed in #2415 (comment), this PR splits `<rust_library_bindgen>__bindgen.link_flags` produced by `rust_bindgen` rule into two files: 1. `rustc_flags` ``` -lstatic=simple ``` 2. `linker_flags` ``` -framework ``` We "sort-of" (but not perfectly) had it correct before #2361 when we link the binary directly with the cc_library (aka both kinds of flags). But since we want to support the Cargo style of linking ``` cc_lib -> bindgen -> lib_a -> bin ``` instead of ``` cc_lib -> bindgen -> lib_a -> bin \___________________________^ ``` we can pass `-lstatic=simple` to the `rustc` command that builds `simple_bindgen` (rust_library) and propagate `linkopts` to `simple_example` (rust_binary) so that the linker can use it. ``` cc_library -> rust_bindgen -> rust_library -> rust_binary -lstatic=simple -Clink-args="-framework" ``` This is long and sounds like a proposal. I'm open for suggestion @UebelAndre @illicitonion @krasimirgg Fixes #2413
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See #2407 (comment) for more context. I believe #2407 breaks any user that uses
cc_common.link
for linking.Filing a bug so we can triage and prioritize fixing it.
The text was updated successfully, but these errors were encountered: