Fix linkopts set in cc deps of bindgen

[tidefield]

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

[UebelAndre]

Maybe it's time we added some starlark unit test for the bindgen rules that transitioned targets to use experimental_use_cc_common_link?

[UebelAndre]

I might have missed it but can you explain the name change?

[UebelAndre]

Is this not a major part of this change? What's the impact of deferring it?

[tidefield]

This needs to be done before this PR is merged. I was putting up the PR just to make sure the idea makes sense.

[UebelAndre]

Also, what would be the impact of exposing a CcInfo provider with linkopts but not the -lstatic flags? That would bubble the flags up to binaries right? While not directly linking libs into the final binary?

[aaronmondal]

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 --incompatible_enable_cc_toolchain_resolution (or used Bazel7 which makes this the default) our builds would mostly pass but during compilation (not linking) of higher-level targets we'd get issues like "Hey your glibc version is incompatible" or some completely random errors like "duplicate main function" and failed macro resolutions.

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

[tidefield]

Exposing a CcInfo provider with linkopts but not the -lstatic flags? That would bubble the flags up to binaries right?

@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 in CcInfo because it's a rustc flag.

While not directly linking libs into the final binary?

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?

[UebelAndre]

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 in CcInfo 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.

[tidefield]

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

This is now done in

rules_rust/bindgen/private/bindgen.bzl

Lines 293 to 300 in bf34e83

	CcInfo(
	linking_context = cc_common.create_linking_context(
	linker_inputs = depset([cc_common.create_linker_input(
	owner = ctx.label,
	user_link_flags = _get_user_link_flags(cc_lib),
	)]),
	),
	),

That said though, maybe it's beneficial to have an extra flags file but right now I question the necessity.

That's correct. I just removed the linker_flags file.

@UebelAndre PTAL

cc: @thb-sb

[UebelAndre]

Yeah, this is exactly what I was wondering if we could do! If this works for you then I'm happy with the change 😄