bazelbuild / rules_rust

Rust rules for Bazel
https://bazelbuild.github.io/rules_rust/
Apache License 2.0
667 stars 431 forks source link

`crates_vendor` ignores package renames in Cargo.toml for aliases #1515

Open csmulhern opened 2 years ago

csmulhern commented 2 years ago

For example, this entry in Cargo.toml:

[dependencies.async_trait]
package = "async-trait"
version = "0.1.51"
default_features = false
features = []

Will generate an alias such as:

alias(
    name = "async-trait",
    actual = "@crates__async-trait-0.1.57//:async_trait",
    tags = ["manual"],
)

Whereas I'd expect the name to be async_trait (vs async-trait).

csmulhern commented 2 years ago

In actuality, it is only ignoring renames that only change dashes into underscores.

The offending code is here:

https://github.com/bazelbuild/rules_rust/blob/86dc561f9b08d75ee890bdfdbab737fac93a1dd1/crate_universe/src/metadata/dependency.rs#L126

https://github.com/bazelbuild/rules_rust/blob/86dc561f9b08d75ee890bdfdbab737fac93a1dd1/crate_universe/src/metadata/dependency.rs#L231-L240

https://github.com/bazelbuild/rules_rust/blob/86dc561f9b08d75ee890bdfdbab737fac93a1dd1/crate_universe/src/utils.rs#L6-L8

From what I can tell, the NodeDep name from cargo_metadata should already be the correct name, with renames taken into account (see: https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.NodeDep.html). @UebelAndre, what is the purpose of get_target_alias here?

csmulhern commented 1 year ago

The problem is that the dependency name is automatically renamed (or 'sanitized' in above lingo) for modules containing hyphens. This means get_target_alias cannot actually tell the difference between an automatic rename, and an intentional rename.

I.e. both:

[dependencies.async_trait]
package = "async-trait"
version = "0.1.51"

And

[dependencies]
async-trait = "0.1.51"

will have a dependency name of async_trait, even though the former is explicitly aliased, whereas the latter is not. This will cause get_target_alias to return None in both cases, meaning the alias will ignore the rename and use async-trait in both cases.

@UebelAndre, any thoughts on how we can make it so at least the first case will use a proper alias?

csmulhern commented 1 year ago

It seems like one angle would be to use Node.dependencies instead of Node.deps, which doesn't expose automatic renaming. Not sure how to efficiently go from a PackageId to the dependency metadata however.

See: https://github.com/oli-obk/cargo_metadata/blob/8fa38ab99b7b9ab52669d57521ae8026437b8d01/src/lib.rs#L287