-
Notifications
You must be signed in to change notification settings - Fork 394
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 linkopts set in cc deps of bindgen #2422
Fix linkopts set in cc deps of bindgen #2422
Conversation
…d `user_link_flags` in bindgen. (bazelbuild#2407)" This reverts commit 90e4505.
810d128
to
ca9a40f
Compare
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.
Maybe it's time we added some starlark unit test for the bindgen rules that transitioned targets to use experimental_use_cc_common_link
?
rust/private/providers.bzl
Outdated
@@ -75,8 +75,8 @@ BuildInfo = provider( | |||
"compile_data": "Depset[File]: Compile data provided by the build script that was not copied into `out_dir`.", | |||
"dep_env": "Optinal[File]: extra build script environment varibles to be set to direct dependencies.", | |||
"flags": "Optional[File]: file containing additional flags to pass to rustc", | |||
"link_flags": "Optional[File]: file containing flags to pass to the linker", | |||
"link_search_paths": "Optional[File]: file containing search paths to pass to the linker", | |||
"linker_flags": "Optional[File]: file containing flags to pass to the linker", |
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 might have missed it but can you explain the name change?
rust/private/rustc.bzl
Outdated
@@ -1011,6 +1011,8 @@ def construct_arguments( | |||
env.update(link_env) | |||
rustc_flags.add(ld, format = "--codegen=linker=%s") | |||
rustc_flags.add_joined("--codegen", link_args, join_with = " ", format_joined = "link-args=%s") | |||
# TODO pass linkopts (now in linker_flags_files) from cc deps to linker |
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.
Is this not a major part of this change? What's the impact of deferring it?
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.
This needs to be done before this PR is merged. I was putting up the PR just to make sure the idea makes sense.
Also, what would be the impact of exposing a |
I believe this fixes an issue we encountered where C++ toolchains in nix environments would randomly depend on the host's glibc in some cases. When we enabled We found that the breakage came "somewhere" from the bindgen logic in rules_rust. I think it was something where our dynamic libcxx from nixpkgs was wrongly linked against the host's glibc instead of the nixpkgs glibc. This then led to these confusing errors down the line. cc @adam-singer |
@UebelAndre That's what I'm proposing to do and it makes sense to me (see https://bazel.build/reference/be/c-cpp#cc_library.linkopts).
What do you mean by "libs" here? Rust or cc or both? If you meant "libs" as cc deps, that would reverse what you did in #2361, correct? |
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.
What do you mean by "libs" here?
"libs" being the cc_lib
targets (C/C++ libraries passed to bindgen)
@UebelAndre That's what I'm proposing to do and it makes sense to me (see https://bazel.build/reference/be/c-cpp#cc_library.linkopts).
-lstatic
shouldn't be included inCcInfo
because it's a rustc flag.
Sweet! But that's not in this PR is it? It looks like there's new plumbing for an additional BuildInfo
flags file but that might be unnecessary if we can instead add back CcInfo
with a surgical set of flags to get the same results. That said though, maybe it's beneficial to have an extra flags file but right now I question the necessity.
This is now done in rules_rust/bindgen/private/bindgen.bzl Lines 293 to 300 in bf34e83
That's correct. I just removed the @UebelAndre PTAL cc: @thb-sb |
079e06c
to
db9dcd1
Compare
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.
Yeah, this is exactly what I was wondering if we could do! If this works for you then I'm happy with the change 😄
This is a follow-up PR of #2422. I'm scoping it to a separate PR to introduce [@rules_testing](https://github.com/bazelbuild/rules_testing) (a new Starlark testing framework). The framework removes a lot of boilerplate code (unncessarily) required when writing analysis tests.
This is a follow-up PR of bazelbuild#2422. I'm scoping it to a separate PR to introduce [@rules_testing](https://github.com/bazelbuild/rules_testing) (a new Starlark testing framework). The framework removes a lot of boilerplate code (unncessarily) required when writing analysis tests.
Problem
As of now (before this PR), we seem to mix
link_flags
file into using for two purposes.For
rust_bindgen_library
produces alink_flags
file with-lstatic=simple
is consumed byrustc
whereas-framework
is expected to be consumed by an actual linker (either invoked byrustc
orcc_common.link
)The flags are then passed to
rustc
command to compilelibsimple_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 producingrlib
). However, the rustc doesn't pass-framework
to the linker that link therust_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 letcc_common.link
instead ofrustc
invoke the linker. See #2413Solution
We're referring "link" as at least two things (which I think what causes problem here):
As proposed in #2415 (comment), this PR splits
<rust_library_bindgen>__bindgen.link_flags
produced byrust_bindgen
rule into two files:rustc_flags
linker_flags
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
instead of
we can pass
-lstatic=simple
to therustc
command that buildssimple_bindgen
(rust_library) and propagatelinkopts
tosimple_example
(rust_binary) so that the linker can use it.This is long and sounds like a proposal. I'm open for suggestion @UebelAndre @illicitonion @krasimirgg
Fixes #2413