Link actions don't get link_flags files propagated from transitive cargo_build_script targets

[illicitonion]

This code diligently forwards all link search path files transitively from cargo_build_script targets, but not the link flag files from them:

rules_rust/rust/private/rustc.bzl

Lines 1458 to 1493 in 1469cd7

	def _create_extra_input_args(build_info, dep_info):
	"""Gather additional input arguments from transitive dependencies
	Args:
	build_info (BuildInfo): The BuildInfo provider from the target Crate's set of inputs.
	dep_info (DepInfo): The Depinfo provider form the target Crate's set of inputs.
	Returns:
	tuple: A tuple of the following items:
	- (depset[File]): A list of all build info `OUT_DIR` File objects
	- (str): The `OUT_DIR` of the current build info
	- (File): An optional generated environment file from a `cargo_build_script` target
	- (depset[File]): All direct and transitive build flag files from the current build info.
	"""
	input_files = []
	# Arguments to the commandline line wrapper that are going to be used
	# to create the final command line
	out_dir = None
	build_env_file = None
	build_flags_files = []
	if build_info:
	out_dir = build_info.out_dir.path
	build_env_file = build_info.rustc_env
	build_flags_files.append(build_info.flags)
	build_flags_files.append(build_info.link_flags)
	input_files.append(build_info.out_dir)
	input_files.append(build_info.link_flags)
	return (
	depset(input_files, transitive = [dep_info.link_search_path_files]),
	out_dir,
	build_env_file,
	depset(build_flags_files, transitive = [dep_info.link_search_path_files]),
	)

This is buggy in two ways:

1. We should be propagating all of the transitive BuildInfo.link_flags up to the link action (but only the link action, not compile actions)
2. We shouldn't be propagating all of the transitive BuildInfo. link_search_path_filess to compile actions, only up to the link action.

The first matters quite a bit. The second is a relatively minor detail.

[sulmone]

Thanks for looking into this!

[sulmone]

I made a basic example repo that shows a working patch to the rust_rules repo:
https://github.com/sulmone/rust_cpp_interop
which uses:
https://github.com/sulmone/rules_rust/tree/transitive-patch

I have a rust_binary package (rust_bin) that depends on a c++ package (cxx_lib) that depends on a rust_library package (rust_lib) that has a number of cargo dependencies. (Sorry if that is a bit convoluted)

[illicitonion]

I put together a draft of this in https://github.com/illicitonion/rules_rust/tree/link-action-flags but it uncovered an interesting issue...

For context, the rough shape of the situation is:

Rust depends on C++ depends on Rust depends on libopenssl-sys
libopenssl-sys has a cargo build script which emits cargo:rustc-link-lib=-lssl

The cargo docs describe that:

The -l flag is only passed to the library target of the package, unless there is no library target, in which case it is passed to all targets. This is done because all other targets have an implicit dependency on the library target, and the given library to link should only be included once. This means that if a package has both a library and a binary target, the library has access to the symbols from the given lib, and the binary should access them through the library target's public API.

We have some prior art in #448 where we started only propagating link flags to the parent crate. Per the cargo docs, I believe this is (roughly) the correct behaviour. (Roughly because, if there was no depending library, we should propagate the flag to the binary, but in practice people don't use rules_rust with a rust_binary depending directly on a cargo_build_script, so I think we can ignore that for now). In particular, my patch breaks a test added in that PR.

Currently, if we write a simple rust_binary which uses reqwest (and in turn uses libopenssl-sys), things work fine.

The problem arises when the dependency path from the rust_binary to the cargo build script emitting cargo:rustc-link-lib=-lssl goes via a cc_library. This radically changes our linking model - instead of using --extern=reqwest=path/to/reqwest linking for dependencies, we use -L path/to/reqwest and -lreqwest-hash. I don't know exactly the mechanism that cargo uses to propagate the linker requirement here - I've asked on zulip for more information.

/cc @krasimirgg who knows a lot more about the Rust/C++ linking story than I do, in case this sparks any inspiration...

[krasimirgg]

The problem arises when the dependency path from the rust_binary to the cargo build script emitting cargo:rustc-link-lib=-lssl goes via a cc_library. This radically changes our linking model - instead of using --extern=reqwest=path/to/reqwest linking for dependencies, we use -L path/to/reqwest and -lreqwest-hash

Right, I think that's roughly what I'd expect. In general, we want the rust targets to be useable in a mixed c++ and rust context. In the C++ world, standard for Bazel is to use the cc_common linking infrastructure to describe the linker inputs of a final c++ binary. Whenever there's a rust dependency of a cc library, rules_rust establishes some common linker definitions (establish_cc_info), trying to build a mapping from the rust side and the bazel common linking side. The difference on the final linker command line in your example is due to that, I think.
Unfortunately I believe it's currently not 100% possible to emulate some of cargo_build_script / cargo-s behaviours, especially around linking without manual work.
Also, from my limited understanding of cargo_build_script-s, it outputs some linker arguments that assume rustc is used as a linker, which isn't the case when using common cc_linking or when the rust library is just a transitive dependency of a top-level cc_binary for example.
A strategy that I've seen working in similar situations where we wrap a native library in a sys crate was to manually add the native cc_library target as a deps of the rust_library sys crate, ensuring that at least when linking in a cc_common context the order and dependencies between the sys archive and the native archive is correct.

[illicitonion]

From https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/rustc-link-lib.20propagation/near/320247453 it sounds like we realistically have two options to make this reliably work:

1. Somehow propagate the rust providers through the cc_library and track enough information that we know we should ignore the CcInfo flags for this transitive library and prefer the CrateInfo to the CcInfo if we have both. The "somehow propagating" seems hard (but could maybe be done with an aspect?), but the "track enough information to choose one over the other" should be easy - both providers keep the label around, so we should be able to just discard the wrong one if we have both.
2. Populate the proper link flags on the CcInfo provider in the first place - there are two pieces missing here, one is being able to add linkflag files (i.e. files which contain linkflags, because we only find out the linkflags to add by running an action) to a CcInfo provider, and the other being having a way to reliably get the linkflags from an rlib (in the example of this issue, we actually already have this because they're output by a cargo_build_script, so we already have the appropriate linkflags file, but in the general case we don't).

@krasimirgg How do the above options sound to you? It feels like 2 is probably the correct approach, because it would make cc_binary-drive graphs work more reliably, but that 1 is probably a far easier approach (because it doesn't require modifications to the core CcInfo provider)... Do you have any impressions on whether we could get CcInfo modified to support action-output flagfiles as well as known-up-front flags?