Fix for bindgen use in rust_shared_library

[neilisaac]

When linking a rust_shared_library that depends on a bindgen_rust_library, I get errors such as:

          /usr/bin/ld.gold: error: /.../some_rust_binding.rlib(some_c_lib.o): requires dynamic R_X86_64_PC32 reloc against '_ZNSs4_Rep20_S_empty_rep_storageE' which may overflow at runtime; recompile with -fPIC

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.

[daivinhtran]

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.

[daivinhtran]

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.

[neilisaac]

@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.

[daivinhtran]

Sorry for the long-awaited reply. I was on vacation for past few weeks.

@UebelAndre

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?

[UebelAndre]

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.