Feature request: Support -Zbindeps in crate universe when using a nightly compiler

[csmulhern]

This would allow easy dependence on binary dependencies (e.g. bindgen or wasm-bindgen) by directly adding a dependency in Cargo.toml, rather than following the convoluted process here: https://bazelbuild.github.io/rules_rust/crate_universe.html#binary-dependencies.

For more info on bindeps, see: https://rust-lang.github.io/rfcs/3028-cargo-binary-dependencies.html.

Initially, my idea was just to add "-Zbindeps" to other_options here, when using a nightly version of cargo (we can also add a separate CLI option to splicing to make this more explicitly opt-in):

rules_rust/crate_universe/src/metadata.rs

Lines 77 to 83 in ca750fa

	let metadata = self
	.cargo_bin
	.metadata_command()?
	.current_dir(manifest_dir)
	.manifest_path(manifest_path.as_ref())
	.other_options(["--locked".to_owned()])
	.exec()?;

However, this does not work, due to use of cargo_metadata. The actual Cargo.toml file managed by the end user is parsed by cargo_metadata (which serializes packages into this struct: https://github.com/oli-obk/cargo_metadata/blob/010cd78751e51dc48d0ca08ca0f4be3ac054a689/src/lib.rs#L289), and then written out as a generated file (which includes various additional attributes used by crate universe). The generated Cargo.toml file is the actual file passed to cargo when splicing. The serialization / deserialization through cargo_metadata is stripping out any unknown fields defined within cargo_metadata. Critically, this is dropping the artifact field defined on a bin-only package when using bindeps.

1. Why do we need to add additional fields to a generated Cargo.toml before splicing?

2. It seems to me like we could generate the generated Cargo.toml with a generic TOML library, rather than round tripping through cargo_metadata. Would maintainers be open to such an approach? Are there any concerns about such an approach?

[illicitonion]

In general we definitely want to support bindeps, and I'd be excited to review a PR adding support for it. I've had in my to do list for a long time to make sure the cargo metadata support for reporting them is sufficient for our needs, and I would be very grateful if someone took that to do off my hands!

In terms of implementation, is the problem here just that we need some extra fields added to a struct in the cargo_metadata crate, and that right now we're losing those fields on round-tripping? If so, I'd recommend roughly the following:

1. Fork cargo_metadata and add the fields.
2. File a PR upstream - it's possible this will get a "not yet" response, because the feature is unstable and they don't want to merge the new schema until it stabilises.
3. Update rules_rust to use the fork rather than the released version of cargo_metadata. If/when the upstream PR merges, we can switch back to the upstream releases.

Note that

rules_rust/crate_universe/src/metadata.rs

Lines 92 to 96 in ca750fa

	#[derive(Clone)]
	pub struct Cargo {
	path: PathBuf,
	full_version: Arc<Mutex<Option<String>>>,
	}

explicitly exists to make it easy to do things like "inject new flags to all cargo invocations, conditionally based on the cargo version", so that should be a handy hook for deciding to add a -Zbindeps flag :)

[csmulhern]

Thanks for all the info. That plan of attack sounds reasonable to me.

I guess I'm questioning if we really need to roundtrip through cargo_metadata for the Cargo.toml generation step where we add the crate universe annotations. I worry that this will be a perpetual problem by going through this serialization / deserialization that only works for fields known by cargo_metadata. I'm guessing we're only adding some TOML keys / values, so I was wondering if it'd be easier to just simplify the generation step to use the toml crate to append those values instead.

[illicitonion]

It's a reasonable question!

In general cargo has a pretty stable API surface, and having a shared set of structured data types we can inspect and verify seems like a useful thing.

I think I'd probably lean towards "Let's keep this in the back of our minds and re-consider if we end up frequently being constrained by cargo_metadata lagging behind cargo", as I suspect we'll probably diverge from what's actually in cargo_metadata very rarely. But if it becomes a frequent problem, we can either discuss with the cargo_metadata folks, or start working on the raw toml?

[dtolnay]

This issue is a duplicate of #1739.

[csmulhern]

I have a patch that enables this within crate_universe when using a nightly version of cargo.

See: https://github.com/csmulhern/rules_rust/commit/e48ed839cb9fb7ad6cd4e10878d40ee647182a0c.

This depends on landing a patch in cargo_toml that adds support for serializing / deserializing artifact keys in cargo_toml::DependencyDetail. E.g. cargo_toml.patch.

[illicitonion]

Very cool @csmulhern, thanks for putting this together! That's a pleasingly small diff :)

I'd be happy to merge this ~as-is, with the following steps:

1. Let's add a test in examples/crate_universe/cargo_bindeps which actually executes the bin and asserts it worked
2. Let's try to upstream the patch to cargo_toml (behind a feature if needs-be, or something, but 🤞 we can just get it merged as-is), and if not, we can point at a git dep in your fork, pinned to a specific rev.

[csmulhern]

Very cool @csmulhern, thanks for putting this together! That's a pleasingly small diff :)

No problem! I'm glad it was a small change too.

I'd be happy to merge this ~as-is, with the following steps:

1. Let's add a test in examples/crate_universe/cargo_bindeps which actually executes the bin and asserts it worked

Sure thing.

2. Let's try to upstream the patch to cargo_toml (behind a feature if needs-be, or something, but 🤞 we can just get it merged as-is), and if not, we can point at a git dep in your fork, pinned to a specific rev.

Sounds good. I'm hoping we can just land the patch upstream, so have been waiting on the following discussion: https://gitlab.com/lib.rs/cargo_toml/-/issues/27. Will consider a fork depending on the outcome of that discussion.

Will update this thread as things develop.

[csmulhern]

@illicitonion the upstream changes were landed in cargo_toml, and published in 0.17.0. See: https://gitlab.com/lib.rs/cargo_toml/-/merge_requests/19.

I've now updated my patch to use the new version of cargo_toml, and added a test that executes the binary dependency.

Link to PR: #2226.