Skip to content

Commit

Permalink
cargo-metadata: error out if same dep with differnt names
Browse files Browse the repository at this point in the history 
Previous, `cargo metadata` allows a dependency with different renamed
co-exist. However, its `resolve.nodes.deps` will miss that dependency,
which is wrong. After this commit, `cargo metadata starts erroring out
for that situation.
  • Loading branch information
@weihanglo
weihanglo committed Jan 10, 2023
1 parent 3c421a2 commit d827e47
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 121 deletions.
21 changes: 9 additions & 12 deletions src/cargo/core/compiler/artifact.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::core::compiler::{Context, CrateType, FileFlavor, Unit};
use crate::core::dependency::ArtifactKind;
use crate::core::{Dependency, Target, TargetKind};
use crate::CargoResult;
use std::collections::HashMap;
use std::collections::{HashMap, HashSet};
use std::ffi::OsString;

/// Return all environment variables for the given unit-dependencies
Expand Down Expand Up @@ -63,21 +63,18 @@ fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str {
///
/// Failure to match any target results in an error mentioning the parent manifests
/// `parent_package` name.
pub(crate) fn match_artifacts_kind_with_targets<'a, F>(
artifact_dep: &Dependency,
targets: &'a [Target],
pub(crate) fn match_artifacts_kind_with_targets<'t, 'd>(
artifact_dep: &'d Dependency,
targets: &'t [Target],
parent_package: &str,
mut callback: F,
) -> CargoResult<()>
where
F: FnMut(&ArtifactKind, &mut dyn Iterator<Item = &'a Target>),
{
) -> CargoResult<HashSet<(&'d ArtifactKind, &'t Target)>> {
let mut out = HashSet::new();
let artifact_requirements = artifact_dep.artifact().expect("artifact present");
for artifact_kind in artifact_requirements.kinds() {
let mut extend = |kind: &ArtifactKind, filter: &dyn Fn(&&Target) -> bool| {
let mut extend = |kind, filter: &dyn Fn(&&Target) -> bool| {
let mut iter = targets.iter().filter(filter).peekable();
let found = iter.peek().is_some();
callback(kind, &mut iter);
out.extend(std::iter::repeat(kind).zip(iter));
found
};
let found = match artifact_kind {
Expand All @@ -97,5 +94,5 @@ where
);
}
}
Ok(())
Ok(out)
}
89 changes: 42 additions & 47 deletions src/cargo/core/compiler/unit_dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,53 +555,48 @@ fn artifact_targets_to_unit_deps(
artifact_pkg: &Package,
dep: &Dependency,
) -> CargoResult<Vec<UnitDep>> {
let mut targets = HashSet::new();
match_artifacts_kind_with_targets(
dep,
artifact_pkg.targets(),
parent.pkg.name().as_str(),
|_, iter| targets.extend(iter),
)?;
let ret = targets
.into_iter()
.flat_map(|target| {
// We split target libraries into individual units, even though rustc is able
// to produce multiple kinds in an single invocation for the sole reason that
// each artifact kind has its own output directory, something we can't easily
// teach rustc for now.
match target.kind() {
TargetKind::Lib(kinds) => Box::new(
kinds
.iter()
.filter(|tk| matches!(tk, CrateType::Cdylib | CrateType::Staticlib))
.map(|target_kind| {
new_unit_dep(
state,
parent,
artifact_pkg,
target
.clone()
.set_kind(TargetKind::Lib(vec![target_kind.clone()])),
parent_unit_for,
compile_kind,
CompileMode::Build,
dep.artifact(),
)
}),
) as Box<dyn Iterator<Item = _>>,
_ => Box::new(std::iter::once(new_unit_dep(
state,
parent,
artifact_pkg,
target,
parent_unit_for,
compile_kind,
CompileMode::Build,
dep.artifact(),
))),
}
})
.collect::<Result<Vec<_>, _>>()?;
let ret =
match_artifacts_kind_with_targets(dep, artifact_pkg.targets(), parent.pkg.name().as_str())?
.into_iter()
.map(|(_artifact_kind, target)| target)
.flat_map(|target| {
// We split target libraries into individual units, even though rustc is able
// to produce multiple kinds in an single invocation for the sole reason that
// each artifact kind has its own output directory, something we can't easily
// teach rustc for now.
match target.kind() {
TargetKind::Lib(kinds) => Box::new(
kinds
.iter()
.filter(|tk| matches!(tk, CrateType::Cdylib | CrateType::Staticlib))
.map(|target_kind| {
new_unit_dep(
state,
parent,
artifact_pkg,
target
.clone()
.set_kind(TargetKind::Lib(vec![target_kind.clone()])),
parent_unit_for,
compile_kind,
CompileMode::Build,
dep.artifact(),
)
}),
) as Box<dyn Iterator<Item = _>>,
_ => Box::new(std::iter::once(new_unit_dep(
state,
parent,
artifact_pkg,
target,
parent_unit_for,
compile_kind,
CompileMode::Build,
dep.artifact(),
))),
}
})
.collect::<Result<Vec<_>, _>>()?;
Ok(ret)
}

Expand Down
110 changes: 54 additions & 56 deletions src/cargo/ops/cargo_output_metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,15 @@ struct Dep {
struct DepKindInfo {
kind: DepKind,
target: Option<Platform>,
/// What the manifest calls the crate.
///
/// A renamed dependency will show the rename instead of original name.
extern_name: InternedString,

// vvvvv The fields below are introduced for `-Z bindeps`.
/// Artifact's crate type, e.g. staticlib, cdylib, bin...
#[serde(skip_serializing_if = "Option::is_none")]
artifact: Option<&'static str>,
/// What the manifest calls the crate.
///
/// A renamed dependency will show the rename instead of original name.
#[serde(skip_serializing_if = "Option::is_none")]
extern_name: Option<InternedString>,
/// Equivalent to `{ target = "…" }` in an artifact dependency requirement.
///
/// * If the target points to a custom target JSON file, the path will be absolute.
Expand Down Expand Up @@ -205,9 +204,9 @@ fn build_resolve_graph_r(
let normalize_id = |id| -> PackageId { *package_map.get_key_value(&id).unwrap().0 };
let features = resolve.features(pkg_id).to_vec();

let deps: Vec<Dep> = resolve
.deps(pkg_id)
.filter(|(_dep_id, deps)| {
let deps = {
let mut dep_metadatas = Vec::new();
let iter = resolve.deps(pkg_id).filter(|(_dep_id, deps)| {
if requested_kinds == [CompileKind::Host] {
true
} else {
Expand All @@ -216,8 +215,8 @@ fn build_resolve_graph_r(
.any(|dep| target_data.dep_platform_activated(dep, *kind))
})
}
})
.filter_map(|(dep_id, deps)| {
});
for (dep_id, deps) in iter {
let mut dep_kinds = Vec::new();

let targets = package_map[&dep_id].targets();
Expand All @@ -227,30 +226,30 @@ fn build_resolve_graph_r(
resolve
.extern_crate_name_and_dep_name(pkg_id, dep_id, target)
.map(|(ext_crate_name, _)| ext_crate_name)
.ok()
};

let lib_target_name = targets.iter().find(|t| t.is_lib()).map(extern_name);
let lib_target = targets.iter().find(|t| t.is_lib());

for dep in deps.iter() {
let mut include_lib = || {
dep_kinds.push(DepKindInfo {
kind: dep.kind(),
target: dep.platform().cloned(),
artifact: None,
extern_name: lib_target_name,
compile_target: None,
bin_name: None,
});
};

// When we do have a library target, include them in deps if...
match (dep.artifact(), lib_target_name) {
// it is also an artifact dep with `{ …, lib = true }`
(Some(a), Some(_)) if a.is_lib() => include_lib(),
// it is not an artifact dep at all
(None, Some(_)) => include_lib(),
_ => {}
if let Some(target) = lib_target {
// When we do have a library target, include them in deps if...
let included = match dep.artifact() {
// it is not an artifact dep at all
None => true,
// it is also an artifact dep with `{ …, lib = true }`
Some(a) if a.is_lib() => true,
_ => false,
};
if included {
dep_kinds.push(DepKindInfo {
kind: dep.kind(),
target: dep.platform().cloned(),
extern_name: extern_name(target)?,
artifact: None,
compile_target: None,
bin_name: None,
});
}
}

// No need to proceed if there is no artifact dependency.
Expand All @@ -269,48 +268,47 @@ fn build_resolve_graph_r(
None => None,
};

if let Err(e) = match_artifacts_kind_with_targets(
dep,
targets,
pkg_id.name().as_str(),
|kind, targets| {
dep_kinds.extend(targets.map(|target| DepKindInfo {
kind: dep.kind(),
target: dep.platform().cloned(),
artifact: Some(kind.crate_type()),
extern_name: extern_name(target),
compile_target,
bin_name: target.is_bin().then(|| target.name().to_string()),
}))
},
) {
return Some(Err(e));
let target_set =
match_artifacts_kind_with_targets(dep, targets, pkg_id.name().as_str())?;
dep_kinds.reserve(target_set.len());
for (kind, target) in target_set.into_iter() {
dep_kinds.push(DepKindInfo {
kind: dep.kind(),
target: dep.platform().cloned(),
extern_name: extern_name(target)?,
artifact: Some(kind.crate_type()),
compile_target,
bin_name: target.is_bin().then(|| target.name().to_string()),
})
}
}

dep_kinds.sort();

let pkg = normalize_id(dep_id);

let dep = match (lib_target_name, dep_kinds.len()) {
(Some(name), _) => Some(Dep {
name,
let dep = match (lib_target, dep_kinds.len()) {
(Some(target), _) => Dep {
name: extern_name(target)?,
pkg,
dep_kinds,
}),
},
// No lib target exists but contains artifact deps.
(None, 1..) => Some(Dep {
(None, 1..) => Dep {
name: InternedString::new(""),
pkg,
dep_kinds,
}),
},
// No lib or artifact dep exists.
// Ususally this mean parent depending on non-lib bin crate.
(None, _) => None,
(None, _) => continue,
};
dep.map(Ok)
})
.collect::<CargoResult<_>>()?;

dep_metadatas.push(dep)
}
dep_metadatas
};

let dumb_deps: Vec<PackageId> = deps.iter().map(|dep| dep.pkg).collect();
let to_visit = dumb_deps.clone();
let node = MetadataResolveNode {
Expand Down
42 changes: 36 additions & 6 deletions tests/testsuite/metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1632,24 +1632,24 @@ fn workspace_metadata_with_dependencies_and_resolve() {
"target": null
},
{
"artifact": "bin",
"bin_name": "baz-name",
"artifact": "cdylib",
"compile_target": "wasm32-unknown-unknown",
"extern_name": "baz_name",
"extern_name": "artifact",
"kind": null,
"target": null
},
{
"artifact": "cdylib",
"artifact": "staticlib",
"compile_target": "wasm32-unknown-unknown",
"extern_name": "artifact",
"kind": null,
"target": null
},
{
"artifact": "staticlib",
"artifact": "bin",
"bin_name": "baz-name",
"compile_target": "wasm32-unknown-unknown",
"extern_name": "artifact",
"extern_name": "baz_name",
"kind": null,
"target": null
},
Expand Down Expand Up @@ -1887,6 +1887,36 @@ fn cargo_metadata_with_invalid_artifact_deps() {
.run();
}

#[cargo_test]
fn cargo_metadata_with_invalid_duplicate_renamed_deps() {
let p = project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.5.0"
[dependencies]
bar = { path = "bar" }
baz = { path = "bar", package = "bar" }
"#,
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_lib_manifest("bar"))
.file("bar/src/lib.rs", "")
.build();

p.cargo("metadata")
.with_status(101)
.with_stderr(
"\
[WARNING] please specify `--format-version` flag explicitly to avoid compatibility problems
[ERROR] the crate `foo v0.5.0 ([..])` depends on crate `bar v0.5.0 ([..])` multiple times with different names",
)
.run();
}

const MANIFEST_OUTPUT: &str = r#"
{
"packages": [{
Expand Down

0 comments on commit d827e47

Please sign in to comment.