From d827e476befbb3a2e0960c252b81a13fd0f7571b Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 10 Jan 2023 01:02:09 +0000 Subject: [PATCH] cargo-metadata: error out if same dep with differnt names 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. --- src/cargo/core/compiler/artifact.rs | 21 ++-- src/cargo/core/compiler/unit_dependencies.rs | 89 +++++++-------- src/cargo/ops/cargo_output_metadata.rs | 110 +++++++++---------- tests/testsuite/metadata.rs | 42 ++++++- 4 files changed, 141 insertions(+), 121 deletions(-) diff --git a/src/cargo/core/compiler/artifact.rs b/src/cargo/core/compiler/artifact.rs index d4894d485e02..6459e4b0e861 100644 --- a/src/cargo/core/compiler/artifact.rs +++ b/src/cargo/core/compiler/artifact.rs @@ -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 @@ -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), -{ +) -> CargoResult> { + 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 { @@ -97,5 +94,5 @@ where ); } } - Ok(()) + Ok(out) } diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 80e40804a5e6..6731342ee7ff 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -555,53 +555,48 @@ fn artifact_targets_to_unit_deps( artifact_pkg: &Package, dep: &Dependency, ) -> CargoResult> { - 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>, - _ => Box::new(std::iter::once(new_unit_dep( - state, - parent, - artifact_pkg, - target, - parent_unit_for, - compile_kind, - CompileMode::Build, - dep.artifact(), - ))), - } - }) - .collect::, _>>()?; + 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>, + _ => Box::new(std::iter::once(new_unit_dep( + state, + parent, + artifact_pkg, + target, + parent_unit_for, + compile_kind, + CompileMode::Build, + dep.artifact(), + ))), + } + }) + .collect::, _>>()?; Ok(ret) } diff --git a/src/cargo/ops/cargo_output_metadata.rs b/src/cargo/ops/cargo_output_metadata.rs index 4b8c04256d87..e9713e4826b8 100644 --- a/src/cargo/ops/cargo_output_metadata.rs +++ b/src/cargo/ops/cargo_output_metadata.rs @@ -91,16 +91,15 @@ struct Dep { struct DepKindInfo { kind: DepKind, target: Option, + /// 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, /// Equivalent to `{ target = "…" }` in an artifact dependency requirement. /// /// * If the target points to a custom target JSON file, the path will be absolute. @@ -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 = 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 { @@ -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(); @@ -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. @@ -269,22 +268,18 @@ 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()), + }) } } @@ -292,25 +287,28 @@ fn build_resolve_graph_r( 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::>()?; + + dep_metadatas.push(dep) + } + dep_metadatas + }; + let dumb_deps: Vec = deps.iter().map(|dep| dep.pkg).collect(); let to_visit = dumb_deps.clone(); let node = MetadataResolveNode { diff --git a/tests/testsuite/metadata.rs b/tests/testsuite/metadata.rs index 64daaf947fae..eb84336aaced 100644 --- a/tests/testsuite/metadata.rs +++ b/tests/testsuite/metadata.rs @@ -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 }, @@ -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": [{