From 6eb451ec92b6aaed6f03dd9a66122df1c34abc2b Mon Sep 17 00:00:00 2001 From: joshmc Date: Fri, 20 Nov 2020 18:57:13 +0000 Subject: [PATCH] ISSUE-16 - Construct cargo_geiger PackageId from a cargo_metadata PackageId: * Add method to construct Source from cargo metadata * Add tests Source construction * Remove further usages of package set Signed-off-by: joshmc --- Cargo.lock | 4 +- cargo-geiger-serde/Cargo.toml | 2 +- cargo-geiger/Cargo.toml | 2 +- cargo-geiger/src/mapping.rs | 28 ++-- cargo-geiger/src/mapping/geiger.rs | 133 +++++++++++++++++ cargo-geiger/src/mapping/metadata.rs | 138 ++---------------- cargo-geiger/src/scan.rs | 16 +- cargo-geiger/src/scan/default.rs | 1 - cargo-geiger/src/scan/default/table.rs | 1 - cargo-geiger/src/scan/forbid.rs | 1 - cargo-geiger/src/scan/forbid/table.rs | 1 - cargo-geiger/src/tree/traversal.rs | 3 - .../src/tree/traversal/dependency_kind.rs | 5 - .../src/tree/traversal/dependency_node.rs | 3 - cargo-geiger/tests/mod.rs | 2 +- .../Cargo.lock | 5 +- .../Cargo.lock | 5 + 17 files changed, 173 insertions(+), 177 deletions(-) create mode 100644 cargo-geiger/src/mapping/geiger.rs create mode 100644 test_crates/test5_workspace_with_virtual_manifest/Cargo.lock diff --git a/Cargo.lock b/Cargo.lock index b220e6d7..7a0bcb63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -207,7 +207,7 @@ dependencies = [ "rand", "regex", "rstest", - "semver 0.10.0", + "semver 0.11.0", "serde", "serde_json", "strum", @@ -221,7 +221,7 @@ dependencies = [ name = "cargo-geiger-serde" version = "0.1.0" dependencies = [ - "semver 0.10.0", + "semver 0.11.0", "serde", "url", ] diff --git a/cargo-geiger-serde/Cargo.toml b/cargo-geiger-serde/Cargo.toml index d5329c38..3ee50d92 100644 --- a/cargo-geiger-serde/Cargo.toml +++ b/cargo-geiger-serde/Cargo.toml @@ -7,6 +7,6 @@ name = "cargo-geiger-serde" version = "0.1.0" [dependencies] -semver = "0.10.0" +semver = "0.11.0" serde = { version = "1.0.116", features = ["derive"] } url = { version = "2.1.1", features = ["serde"] } diff --git a/cargo-geiger/Cargo.toml b/cargo-geiger/Cargo.toml index 01d848b6..4773af10 100644 --- a/cargo-geiger/Cargo.toml +++ b/cargo-geiger/Cargo.toml @@ -44,5 +44,5 @@ insta = "0.16.1" rand = "0.7.3" regex = "1.3.9" rstest = "0.6.4" -semver = "0.10.0" +semver = "0.11.0" tempfile = "3.1.0" diff --git a/cargo-geiger/src/mapping.rs b/cargo-geiger/src/mapping.rs index 3b4163a8..3670ce6d 100644 --- a/cargo-geiger/src/mapping.rs +++ b/cargo-geiger/src/mapping.rs @@ -1,10 +1,10 @@ mod core; +mod geiger; mod krates; mod metadata; use ::krates::Krates; use cargo::core::dependency::DepKind; -use cargo::core::{Package, PackageId, PackageSet}; use cargo_metadata::{DependencyKind, Metadata}; use std::collections::HashSet; use std::path::PathBuf; @@ -75,11 +75,17 @@ pub trait ToCargoCoreDepKind { pub trait ToCargoGeigerPackageId { fn to_cargo_geiger_package_id( &self, - krates: &Krates, - package_set: &PackageSet, + metadata: &Metadata, ) -> cargo_geiger_serde::PackageId; } +pub trait ToCargoGeigerSource { + fn to_cargo_geiger_source( + &self, + metadata: &Metadata, + ) -> cargo_geiger_serde::Source; +} + pub trait ToCargoMetadataDependencyKind { fn to_cargo_metadata_dependency_kind(&self) -> DependencyKind; } @@ -97,19 +103,3 @@ pub trait ToCargoMetadataPackageId { metadata: &Metadata, ) -> Option; } - -pub trait ToPackage { - fn to_package( - &self, - krates: &Krates, - package_set: &PackageSet, - ) -> Option; -} - -pub trait ToPackageId { - fn to_package_id( - &self, - krates: &Krates, - package_set: &PackageSet, - ) -> Option; -} diff --git a/cargo-geiger/src/mapping/geiger.rs b/cargo-geiger/src/mapping/geiger.rs new file mode 100644 index 00000000..f6d60291 --- /dev/null +++ b/cargo-geiger/src/mapping/geiger.rs @@ -0,0 +1,133 @@ +use super::ToCargoGeigerSource; + +use crate::mapping::ToCargoMetadataPackage; + +use cargo_metadata::Metadata; +use url::Url; + +impl ToCargoGeigerSource for cargo_metadata::PackageId { + fn to_cargo_geiger_source( + &self, + metadata: &Metadata, + ) -> cargo_geiger_serde::Source { + let package = self.to_cargo_metadata_package(metadata).unwrap(); + + match package.source { + Some(source) => handle_source_repr(&source.repr), + None => handle_path_source(self), + } + } +} + +fn handle_source_repr(source_repr: &str) -> cargo_geiger_serde::Source { + let mut source_repr_vec = source_repr.split('+').collect::>(); + + let source_type = source_repr_vec[0]; + + match source_type { + "registry" => { + cargo_geiger_serde::Source::Registry { + // It looks like cargo metadata drops this information + name: String::from("crates.io"), + url: Url::parse(source_repr_vec.pop().unwrap()).unwrap(), + } + } + "git" => { + let raw_git_representation = source_repr_vec.pop().unwrap(); + let raw_git_url = Url::parse(raw_git_representation).unwrap(); + let git_url_without_query = format!( + "{}://{}{}", + raw_git_url.scheme(), + raw_git_url.host_str().unwrap(), + raw_git_url.path() + ); + let revision = raw_git_url + .query_pairs() + .find(|(query_key, _)| query_key == "rev") + .unwrap() + .1; + + cargo_geiger_serde::Source::Git { + url: Url::parse(&git_url_without_query).unwrap(), + rev: String::from(revision), + } + } + _ => { + panic!("Unrecognised source type: {}", source_type) + } + } +} + +fn handle_path_source( + package_id: &cargo_metadata::PackageId, +) -> cargo_geiger_serde::Source { + let raw_repr = package_id.repr.clone(); + let raw_path_repr = raw_repr[1..raw_repr.len() - 1] + .split("+file://") + .skip(1) + .collect::>() + .pop() + .unwrap(); + + let source_url: Url; + if cfg!(windows) { + source_url = Url::from_file_path(&raw_path_repr[1..]).unwrap(); + } else { + source_url = Url::from_file_path(raw_path_repr).unwrap(); + } + + cargo_geiger_serde::Source::Path(source_url) +} + +#[cfg(test)] +mod geiger_tests { + use super::*; + + use rstest::*; + use url::Url; + + #[rstest( + input_source_repr, + expected_source, + case( + "registry+https://github.com/rust-lang/crates.io-index", + cargo_geiger_serde::Source::Registry { + name: String::from("crates.io"), + url: Url::parse("https://github.com/rust-lang/crates.io-index").unwrap() + } + ), + case( + "git+https://github.com/rust-itertools/itertools.git?rev=8761fbefb3b209", + cargo_geiger_serde::Source::Git { + url: Url::parse("https://github.com/rust-itertools/itertools.git").unwrap(), + rev: String::from("8761fbefb3b209") + } + ) + )] + fn handle_source_repr_test( + input_source_repr: &str, + expected_source: cargo_geiger_serde::Source, + ) { + let source = handle_source_repr(input_source_repr); + assert_eq!(source, expected_source); + } + + #[rstest] + fn handle_path_source_test() { + if !cfg!(windows) { + let package_id = cargo_metadata::PackageId { + repr: String::from("(path+file:///cargo_geiger/test_crates/test1_package_with_no_deps)"), + }; + + let expected_source = cargo_geiger_serde::Source::Path( + Url::from_file_path( + "/cargo_geiger/test_crates/test1_package_with_no_deps", + ) + .unwrap(), + ); + + let source = handle_path_source(&package_id); + assert_eq!(source, expected_source); + } + } +} diff --git a/cargo-geiger/src/mapping/metadata.rs b/cargo-geiger/src/mapping/metadata.rs index 3f3f566a..6d98b282 100644 --- a/cargo-geiger/src/mapping/metadata.rs +++ b/cargo-geiger/src/mapping/metadata.rs @@ -2,18 +2,16 @@ use super::{ DepsNotReplaced, GetPackageNameFromCargoMetadataPackageId, GetPackageVersionFromCargoMetadataPackageId, GetRoot, MatchesIgnoringSource, ToCargoCoreDepKind, ToCargoGeigerPackageId, - ToCargoMetadataPackageId, ToPackage, ToPackageId, + ToCargoMetadataPackageId, }; -use crate::mapping::ToCargoMetadataPackage; +use crate::mapping::{ToCargoGeigerSource, ToCargoMetadataPackage}; use cargo::core::dependency::DepKind; -use cargo::core::{Package, PackageId, PackageSet}; use cargo_metadata::{DependencyKind, Metadata}; use krates::Krates; use std::collections::HashSet; use std::path::PathBuf; -use url::Url; impl DepsNotReplaced for cargo_metadata::Metadata { fn deps_not_replaced( @@ -82,49 +80,15 @@ impl ToCargoCoreDepKind for DependencyKind { impl ToCargoGeigerPackageId for cargo_metadata::PackageId { fn to_cargo_geiger_package_id( &self, - krates: &Krates, - package_set: &PackageSet, + metadata: &Metadata, ) -> cargo_geiger_serde::PackageId { - let package_id = self.to_package_id(krates, package_set).unwrap(); - let source = package_id.source_id(); - let source_url = source.url(); - // Canonicalize paths as cargo does not seem to do so on all platforms. - let source_url = if source_url.scheme() == "file" { - match source_url.to_file_path() { - Ok(p) => { - let p = p.canonicalize().expect( - "A package source path could not be canonicalized", - ); - Url::from_file_path(p) - .expect("A URL could not be created from a file path") - } - Err(_) => source_url.clone(), - } - } else { - source_url.clone() - }; - let source = if source.is_git() { - cargo_geiger_serde::Source::Git { - url: source_url, - rev: source - .precise() - .expect("Git revision should be known") - .to_string(), - } - } else if source.is_path() { - cargo_geiger_serde::Source::Path(source_url) - } else if source.is_registry() { - cargo_geiger_serde::Source::Registry { - name: source.display_registry_name(), - url: source_url, - } - } else { - panic!("Unsupported source type: {:?}", source) - }; + let package = self.to_cargo_metadata_package(metadata).unwrap(); + let metadata_source = self.to_cargo_geiger_source(metadata); + cargo_geiger_serde::PackageId { - name: package_id.name().to_string(), - version: package_id.version().clone(), - source, + name: package.name, + version: package.version, + source: metadata_source, } } } @@ -159,49 +123,6 @@ impl ToCargoMetadataPackage for cargo_metadata::PackageId { } } -impl ToPackage for cargo_metadata::PackageId { - fn to_package( - &self, - krates: &Krates, - package_set: &PackageSet, - ) -> Option { - let package_id = self.to_package_id(krates, package_set).unwrap(); - Some( - package_set - .get_one(package_id) - .unwrap_or_else(|_| { - // TODO: Avoid panic, return Result. - panic!("Expected to find package by id: {}", package_id); - }) - .clone(), - ) - } -} - -impl ToPackageId for cargo_metadata::PackageId { - fn to_package_id( - &self, - krates: &Krates, - package_set: &PackageSet, - ) -> Option { - krates.node_for_kid(&self).and_then(|package| { - package_set - .package_ids() - .filter(|package_id| { - package_id.name().to_string() == package.krate.name - && package_id.version().major - == package.krate.version.major - && package_id.version().minor - == package.krate.version.minor - && package_id.version().patch - == package.krate.version.patch - }) - .collect::>() - .pop() - }) - } -} - #[cfg(test)] mod metadata_tests { use super::*; @@ -212,7 +133,7 @@ mod metadata_tests { use crate::cli::{get_registry, get_workspace, resolve}; use cargo::core::registry::PackageRegistry; - use cargo::core::Workspace; + use cargo::core::{Package, Workspace}; use cargo::Config; use cargo_metadata::{CargoOpt, Metadata, MetadataCommand}; use krates::Builder as KratesBuilder; @@ -318,22 +239,12 @@ mod metadata_tests { #[rstest] fn to_cargo_geiger_package_id_test() { - let args = FeaturesArgs::default(); - let config = Config::default().unwrap(); - let (package, mut registry, workspace) = - construct_package_registry_workspace_tuple(&config); - - let (package_set, _) = - resolve(&args, package.package_id(), &mut registry, &workspace) - .unwrap(); - - let (krates, metadata) = construct_krates_and_metadata(); + let (_, metadata) = construct_krates_and_metadata(); let root_package = metadata.root_package().unwrap(); - let cargo_geiger_package_id = root_package - .id - .to_cargo_geiger_package_id(&krates, &package_set); + let cargo_geiger_package_id = + root_package.id.to_cargo_geiger_package_id(&metadata); assert_eq!(cargo_geiger_package_id.name, root_package.name); @@ -351,29 +262,6 @@ mod metadata_tests { ); } - #[rstest] - fn to_package_id_test() { - let args = FeaturesArgs::default(); - let config = Config::default().unwrap(); - let (package, mut registry, workspace) = - construct_package_registry_workspace_tuple(&config); - - let (package_set, _) = - resolve(&args, package.package_id(), &mut registry, &workspace) - .unwrap(); - - let (krates, metadata) = construct_krates_and_metadata(); - - let cargo_metadata_package = metadata.root_package().unwrap(); - let package_id = cargo_metadata_package - .id - .clone() - .to_package_id(&krates, &package_set) - .unwrap(); - - assert_eq!(cargo_metadata_package.name, package_id.name().to_string()); - } - fn construct_krates_and_metadata() -> (Krates, Metadata) { let metadata = MetadataCommand::new() .manifest_path("./Cargo.toml") diff --git a/cargo-geiger/src/scan.rs b/cargo-geiger/src/scan.rs index 3d20f739..8300b762 100644 --- a/cargo-geiger/src/scan.rs +++ b/cargo-geiger/src/scan.rs @@ -168,7 +168,6 @@ fn package_metrics( cargo_metadata_parameters: &CargoMetadataParameters, geiger_context: &GeigerContext, graph: &Graph, - package_set: &PackageSet, root_package_id: cargo_metadata::PackageId, ) -> Vec<(PackageInfo, Option)> { let mut package_metrics = @@ -180,20 +179,17 @@ fn package_metrics( while !indices.is_empty() { let i = indices.pop().unwrap(); let package_id = graph.graph[i].clone(); - let mut package = - PackageInfo::new(package_id.to_cargo_geiger_package_id( - cargo_metadata_parameters.krates, - package_set, - )); + let mut package = PackageInfo::new( + package_id + .to_cargo_geiger_package_id(cargo_metadata_parameters.metadata), + ); for edge in graph.graph.edges(i) { let dep_index = edge.target(); if visited.insert(dep_index) { indices.push(dep_index); } - let dep = graph.graph[dep_index].to_cargo_geiger_package_id( - cargo_metadata_parameters.krates, - package_set, - ); + let dep = graph.graph[dep_index] + .to_cargo_geiger_package_id(cargo_metadata_parameters.metadata); package.add_dependency( dep, diff --git a/cargo-geiger/src/scan/default.rs b/cargo-geiger/src/scan/default.rs index 4351cef0..d09140a8 100644 --- a/cargo-geiger/src/scan/default.rs +++ b/cargo-geiger/src/scan/default.rs @@ -135,7 +135,6 @@ fn scan_to_report( cargo_metadata_parameters, &geiger_context, graph, - package_set, root_package_id, ) { let package_metrics = match package_metrics_option { diff --git a/cargo-geiger/src/scan/default/table.rs b/cargo-geiger/src/scan/default/table.rs index 32893a93..2a11351a 100644 --- a/cargo-geiger/src/scan/default/table.rs +++ b/cargo-geiger/src/scan/default/table.rs @@ -53,7 +53,6 @@ pub fn scan_to_table( let text_tree_lines = walk_dependency_tree( cargo_metadata_parameters, &graph, - package_set, &scan_parameters.print_config, root_package_id, ); diff --git a/cargo-geiger/src/scan/forbid.rs b/cargo-geiger/src/scan/forbid.rs index e4ad22ee..63ac01d7 100644 --- a/cargo-geiger/src/scan/forbid.rs +++ b/cargo-geiger/src/scan/forbid.rs @@ -62,7 +62,6 @@ fn scan_forbid_to_report( cargo_metadata_parameters, &geiger_context, graph, - package_set, root_package_id, ) { let pack_metrics = match package_metrics { diff --git a/cargo-geiger/src/scan/forbid/table.rs b/cargo-geiger/src/scan/forbid/table.rs index 3366f46d..a5ce6dd5 100644 --- a/cargo-geiger/src/scan/forbid/table.rs +++ b/cargo-geiger/src/scan/forbid/table.rs @@ -32,7 +32,6 @@ pub fn scan_forbid_to_table( let tree_lines = walk_dependency_tree( cargo_metadata_parameters, &graph, - package_set, &print_config, root_package_id, ); diff --git a/cargo-geiger/src/tree/traversal.rs b/cargo-geiger/src/tree/traversal.rs index b9b7021c..0565fe51 100644 --- a/cargo-geiger/src/tree/traversal.rs +++ b/cargo-geiger/src/tree/traversal.rs @@ -10,7 +10,6 @@ use super::construct_tree_vines_string; use dependency_kind::walk_dependency_kind; use dependency_node::walk_dependency_node; -use cargo::core::PackageSet; use std::collections::HashSet; pub struct WalkDependencyParameters<'a> { @@ -29,7 +28,6 @@ pub struct WalkDependencyParameters<'a> { pub fn walk_dependency_tree( cargo_metadata_parameters: &CargoMetadataParameters, graph: &Graph, - package_set: &PackageSet, print_config: &PrintConfig, root_package_id: cargo_metadata::PackageId, ) -> Vec { @@ -47,7 +45,6 @@ pub fn walk_dependency_tree( walk_dependency_node( cargo_metadata_parameters, &node, - package_set, &mut walk_dependency_paramters, ) } diff --git a/cargo-geiger/src/tree/traversal/dependency_kind.rs b/cargo-geiger/src/tree/traversal/dependency_kind.rs index 1916b51c..6cd8a8de 100644 --- a/cargo-geiger/src/tree/traversal/dependency_kind.rs +++ b/cargo-geiger/src/tree/traversal/dependency_kind.rs @@ -6,7 +6,6 @@ use super::dependency_node::walk_dependency_node; use crate::mapping::CargoMetadataParameters; use cargo::core::dependency::DepKind; -use cargo::core::PackageSet; use std::iter::Peekable; use std::slice::Iter; @@ -14,7 +13,6 @@ pub fn walk_dependency_kind( cargo_metadata_parameters: &CargoMetadataParameters, dep_kind: DepKind, deps: &mut Vec, - package_set: &PackageSet, walk_dependency_parameters: &mut WalkDependencyParameters, ) -> Vec { if deps.is_empty() { @@ -42,7 +40,6 @@ pub fn walk_dependency_kind( cargo_metadata_parameters, dependency, &mut node_iterator, - package_set, &mut text_tree_lines, walk_dependency_parameters, ); @@ -54,7 +51,6 @@ fn handle_walk_dependency_node( cargo_metadata_parameters: &CargoMetadataParameters, dependency: &cargo_metadata::PackageId, node_iterator: &mut Peekable>, - package_set: &PackageSet, text_tree_lines: &mut Vec, walk_dependency_parameters: &mut WalkDependencyParameters, ) { @@ -64,7 +60,6 @@ fn handle_walk_dependency_node( text_tree_lines.append(&mut walk_dependency_node( cargo_metadata_parameters, dependency, - package_set, walk_dependency_parameters, )); walk_dependency_parameters.levels_continue.pop(); diff --git a/cargo-geiger/src/tree/traversal/dependency_node.rs b/cargo-geiger/src/tree/traversal/dependency_node.rs index 926ac7b9..ef7f22a8 100644 --- a/cargo-geiger/src/tree/traversal/dependency_node.rs +++ b/cargo-geiger/src/tree/traversal/dependency_node.rs @@ -8,7 +8,6 @@ use super::construct_tree_vines_string; use super::walk_dependency_kind; use cargo::core::dependency::DepKind; -use cargo::core::PackageSet; use petgraph::visit::EdgeRef; use petgraph::EdgeDirection; use std::collections::HashMap; @@ -16,7 +15,6 @@ use std::collections::HashMap; pub fn walk_dependency_node( cargo_metadata_parameters: &CargoMetadataParameters, package: &cargo_metadata::PackageId, - package_set: &PackageSet, walk_dependency_parameters: &mut WalkDependencyParameters, ) -> Vec { let new = walk_dependency_parameters.print_config.all @@ -48,7 +46,6 @@ pub fn walk_dependency_node( cargo_metadata_parameters, *dep_kind, nodes, - package_set, walk_dependency_parameters, ); diff --git a/cargo-geiger/tests/mod.rs b/cargo-geiger/tests/mod.rs index b2d3aa44..342b9aaf 100644 --- a/cargo-geiger/tests/mod.rs +++ b/cargo-geiger/tests/mod.rs @@ -656,7 +656,7 @@ mod external { "https://github.com/rust-itertools/itertools.git", ) .unwrap(), - rev: "8761fbefb3b209cf41829f8dba38044b69c1d8dd".into(), + rev: "8761fbefb3b209".into(), }, } } diff --git a/test_crates/test4_workspace_with_top_level_package/Cargo.lock b/test_crates/test4_workspace_with_top_level_package/Cargo.lock index 34b53708..d2d4c54d 100644 --- a/test_crates/test4_workspace_with_top_level_package/Cargo.lock +++ b/test_crates/test4_workspace_with_top_level_package/Cargo.lock @@ -9,9 +9,8 @@ name = "test1_package_with_no_deps" version = "0.1.0" [[package]] -name = "test4_workspace_with_virtual_manifest" +name = "test4_workspace_with_top_level_package" version = "0.1.0" dependencies = [ - "test1_package_with_no_deps 0.1.0", + "test1_package_with_no_deps", ] - diff --git a/test_crates/test5_workspace_with_virtual_manifest/Cargo.lock b/test_crates/test5_workspace_with_virtual_manifest/Cargo.lock new file mode 100644 index 00000000..254cf73f --- /dev/null +++ b/test_crates/test5_workspace_with_virtual_manifest/Cargo.lock @@ -0,0 +1,5 @@ +# This file is automatically @generated by Cargo. +# It is not intended for manual editing. +[[package]] +name = "member1" +version = "0.1.0"