diff --git a/.gitignore b/.gitignore index ea8c4bf..eb5a316 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1 @@ -/target +target diff --git a/fixtures/optional_non_dev_dep/Cargo.lock b/fixtures/optional_non_dev_dep/Cargo.lock new file mode 100644 index 0000000..fbafbea --- /dev/null +++ b/fixtures/optional_non_dev_dep/Cargo.lock @@ -0,0 +1,25 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +version = 4 + +[[package]] +name = "libz-rs-sys" +version = "0.5.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c10501e7805cee23da17c7790e59df2870c0d4043ec6d03f67d31e2b53e77415" +dependencies = [ + "zlib-rs", +] + +[[package]] +name = "optional_non_dev_dep" +version = "0.1.0" +dependencies = [ + "libz-rs-sys", +] + +[[package]] +name = "zlib-rs" +version = "0.5.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "40990edd51aae2c2b6907af74ffb635029d5788228222c4bb811e9351c0caad3" diff --git a/fixtures/optional_non_dev_dep/Cargo.toml b/fixtures/optional_non_dev_dep/Cargo.toml new file mode 100644 index 0000000..ed41789 --- /dev/null +++ b/fixtures/optional_non_dev_dep/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "optional_non_dev_dep" +version = "0.1.0" +edition = "2024" +publish = false + +[dependencies] +libz-rs-sys = { version = "=0.5.5", optional = true } + +[dev-dependencies] +libz-rs-sys = "=0.5.5" + +[workspace] diff --git a/fixtures/optional_non_dev_dep/src/lib.rs b/fixtures/optional_non_dev_dep/src/lib.rs new file mode 100644 index 0000000..b93cf3f --- /dev/null +++ b/fixtures/optional_non_dev_dep/src/lib.rs @@ -0,0 +1,14 @@ +pub fn add(left: u64, right: u64) -> u64 { + left + right +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn it_works() { + let result = add(2, 2); + assert_eq!(result, 4); + } +} diff --git a/src/common.rs b/src/common.rs index 2372967..3a9e29f 100644 --- a/src/common.rs +++ b/src/common.rs @@ -1,7 +1,7 @@ use anyhow::bail; use cargo_metadata::{ - semver::VersionReq, CargoOpt::AllFeatures, CargoOpt::NoDefaultFeatures, Dependency, - DependencyKind, Metadata, MetadataCommand, Package, PackageId, + CargoOpt::AllFeatures, CargoOpt::NoDefaultFeatures, DependencyKind, Metadata, MetadataCommand, + NodeDep, Package, PackageId, }; use std::collections::{HashMap, HashSet}; @@ -85,12 +85,12 @@ fn sourced_dependencies_from_metadata( } } - for pkg in meta.workspace_members { - *how.get_mut(&pkg).unwrap() = PkgSource::Local; + for pkg in &meta.workspace_members { + *how.get_mut(pkg).unwrap() = PkgSource::Local; } if no_dev { - (how, what) = extract_non_dev_dependencies(&mut how, &mut what); + (how, what) = extract_non_dev_dependencies(&meta, &mut how, &mut what); } let dependencies: Vec<_> = how @@ -107,42 +107,30 @@ fn sourced_dependencies_from_metadata( Ok(dependencies) } -#[derive(Eq, Hash, PartialEq)] -struct Dep { - name: String, - req: VersionReq, -} - -impl Dep { - fn from_cargo_metadata_dependency(dep: &Dependency) -> Self { - Self { - name: dep.name.clone(), - req: dep.req.clone(), - } - } - - fn matches(&self, pkg: &Package) -> bool { - self.name == pkg.name && self.req.matches(&pkg.version) - } -} - -/// Start with the `PkgSource::Local` packages, then iteratively add non-dev-dependencies until no more -/// packages can be added, and return the results. -/// -/// Note that matching dependencies to packages is "best effort." The fields that Cargo uses to -/// determine a package's id are its name, version, and source: -/// https://github.com/rust-lang/cargo/blob/dd5134c7a59e3a3b8587f1ef04a930185d2ca503/src/cargo/core/package_id.rs#L29-L31 +/// Start with the `PkgSource::Local` packages, then iteratively add non-dev-dependencies until no +/// more packages can be added, and return the results. /// -/// When matching dependencies to packages, we use the package's name and version, but not its source -/// (see [`Dep`]). Experiments suggest that source strings can vary. So comparing them seems risky. -/// Also, it is better to err on the side of inclusion. +/// This function uses the resolved dependency graph from `cargo metadata` to determine which +/// dependencies are actually used. This function does _not_ use the declared dependencies, which +/// may include optional dependencies that aren't actually used. fn extract_non_dev_dependencies( + meta: &Metadata, how: &mut HashMap, what: &mut HashMap, ) -> (HashMap, HashMap) { let mut how_new = HashMap::new(); let mut what_new = HashMap::new(); + let Some(resolve) = &meta.resolve else { + return (HashMap::new(), HashMap::new()); + }; + + let node_deps: HashMap<&PackageId, &[NodeDep]> = resolve + .nodes + .iter() + .map(|node| (&node.id, node.deps.as_slice())) + .collect(); + let mut ids = how .iter() .filter_map(|(id, source)| { @@ -158,9 +146,15 @@ fn extract_non_dev_dependencies( let mut deps = HashSet::new(); for id in ids.drain(..) { - for dep in &what.get(&id).unwrap().dependencies { - if dep.kind != DependencyKind::Development { - deps.insert(Dep::from_cargo_metadata_dependency(dep)); + if let Some(node_deps) = node_deps.get(&id) { + for dep in *node_deps { + if dep + .dep_kinds + .iter() + .any(|info| info.kind != DependencyKind::Development) + { + deps.insert(&dep.pkg); + } } } @@ -168,9 +162,9 @@ fn extract_non_dev_dependencies( what_new.insert(id.clone(), what.remove(&id).unwrap()); } - for pkg in what.values() { - if deps.iter().any(|dep| dep.matches(pkg)) { - ids.push(pkg.id.clone()); + for pkg_id in what.keys() { + if deps.contains(pkg_id) { + ids.push(pkg_id.clone()); } } } @@ -231,7 +225,7 @@ pub fn comma_separated_list(list: &[String]) -> String { #[cfg(test)] mod tests { use super::{sourced_dependencies_from_metadata, SourcedPackage}; - use cargo_metadata::Metadata; + use cargo_metadata::{Metadata, MetadataCommand}; use std::{ cmp::Ordering, env::var, @@ -281,7 +275,8 @@ mod tests { } } - // `cargo` has `snapbox` as a dev dependency. `snapbox` has `snapbox-macros` as a normal dependency. + // `cargo` has `snapbox` as a dev dependency. `snapbox` has `snapbox-macros` as a normal + // dependency. #[test] fn cargo() { @@ -306,6 +301,22 @@ mod tests { assert!(deps.iter().any(|dep| dep.package.name == "snapbox-macros")); } + #[test] + fn optional_dependency_excluded_when_not_activated() { + let metadata = MetadataCommand::new() + .current_dir("fixtures/optional_non_dev_dep") + .exec() + .unwrap(); + + let deps = sourced_dependencies_from_metadata(metadata.clone(), false).unwrap(); + assert!(deps.iter().any(|dep| dep.package.name == "libz-rs-sys")); + + let deps_no_dev = sourced_dependencies_from_metadata(metadata, true).unwrap(); + assert!(!deps_no_dev + .iter() + .any(|dep| dep.package.name == "libz-rs-sys")); + } + fn sourced_dependencies_from_file(path: impl AsRef) -> Vec { let contents = read_to_string(path).unwrap(); serde_json::from_str::>(&contents).unwrap()