-
Notifications
You must be signed in to change notification settings - Fork 413
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
Fix for bindgen use in rust_shared_library #2517
base: main
Are you sure you want to change the base?
Conversation
@@ -94,7 +94,7 @@ def rust_bindgen_library( | |||
rust_library( | |||
name = name, | |||
srcs = [name + "__bindgen.rs"], | |||
deps = deps + [name + "__bindgen"], |
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.
From rust_bindgen implementation, name + "__bindgen"
seems to already re-export cc_lib
through the CcInfo
provider.
I'm afraid this isn't the right fix because name + "__bindgen"
target now becomes completely unused. I think CI not failing yields that we don't have enough testing coverage in-place.
Could you look into why cc_lib
is not propagated to downstream correctly?
#2422 might give some context.
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.
rules_rust/test/bindgen/bindgen_test.bzl
Lines 15 to 35 in 777f3e5
cc_library( | |
name = name + "_cc", | |
srcs = ["simple.cc"], | |
hdrs = ["simple.h"], | |
linkopts = ["-shared"], | |
tags = ["manual"], | |
) | |
rust_bindgen_library( | |
name = name + "_rust_bindgen", | |
cc_lib = name + "_cc", | |
header = "simple.h", | |
tags = ["manual"], | |
edition = "2021", | |
) | |
rust_binary( | |
name = name + "_rust_binary", | |
srcs = ["main.rs"], | |
deps = [name + "_rust_bindgen"], | |
tags = ["manual"], | |
edition = "2021", | |
) |
is a test where rust_bindgen_library
is a dep of rust_binary
. Since it works for for rust_binary
, it's probably a bug if it doesn't work for rusts_shared_library
.
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.
@daivinhtran if I'm reading the error message correctly, it seems like it's linking the cc_lib's code from the .rlib file rather than a .a file from the original cc_library target. That would work fine if you're linking statically.
There's a comment in _rust_bindgen_impl:
# As in https://github.com/bazelbuild/rules_rust/pull/2361, we want
# to link cc_lib to the direct parent (rlib) using `-lstatic=<cc_lib>` rustc flag
# Hence, we do not need to provide the whole CcInfo of cc_lib because
# it will cause the downstream binary to link the cc_lib again
# (same effect as setting `leak_symbols` attribute above)
# The CcInfo here only contains the custom link flags (i.e. linkopts attribute)
# specified by users in cc_lib
I'm not sure if that's a good idea. Why not let rules_cc handle the corner cases itself? I don't have all the context here, but why can't rust_bindgen just be a code generator? That's what I assumed it was from the documentation
Generates a rust source file from a cc_library and a header.
hence I expected rust_bindgen_library to wrap it in a rust_library with dependencies.
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.
Sorry for the long-awaited reply. I was on vacation for past few weeks.
It looks like the decision of linking cc statically was originated in https://github.com/bazelbuild/rules_rust/pull/2361/files#diff-4bccb0ba6f0f44e4670d5726a5fcc04375b792fdaf7c0ad68b8891d1557c26d2R116. @neilisaac brought up a good point that this approach doesn't seem correct when in rust_shared_library
. @UebelAndre Have you thought about this scenario before? Or was it an oversight?
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 did not think about the scenario with rust_shared_library
. It would be very helpful if this PR could add an integration test that builds and consumes a rust_shared_library
which uses rust_bindgen
so we can come to an implementation that works for all use cases.
When linking a rust_shared_library that depends on a bindgen_rust_library, I get errors such as:
It looks to me like the rust_library target is including the cc_lib in its rlib output. This PR changes it to depend on the cc_library target instead, where the cc toolchain will provide both PIC and non-PIC objects to select as appropriate at link time.