diff --git a/README.md b/README.md index 7187401..9a86a97 100644 --- a/README.md +++ b/README.md @@ -165,9 +165,11 @@ The `ripples` command is the star of our CI circus! This precision tool determin **What it does:** - Maps changed files to their containing crates +- Canonicalizes crates by their manifest paths so duplicate package names stay unique across workspaces - Traces dependencies to find all affected components - Distinguishes between directly and indirectly affected crates - Provides both workspace and crate-level impact analysis +- Resolves workspace dependencies via Cargo metadata rather than directory-name heuristics - Outputs machine-readable formats for CI integration **When to use it:** @@ -182,7 +184,9 @@ The `ripples` command is the star of our CI circus! This precision tool determin Unlike naive approaches that rebuild everything or guess based on directory names, `ripples` understands your actual dependency graph. It precisely identifies affected components, even when: - Multiple crates exist in a single workspace +- Different workspaces reuse the same crate name - Changes affect shared dependencies +- Workspace directories diverge from their package names - File moves or renames occur - Only test files are modified diff --git a/src/analyzer/analyzer_impl.rs b/src/analyzer/analyzer_impl.rs index eb51344..e688482 100644 --- a/src/analyzer/analyzer_impl.rs +++ b/src/analyzer/analyzer_impl.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::path::{Path, PathBuf}; use console::style; @@ -29,13 +29,17 @@ pub enum CrateMemberBuilderError { } // Type aliases to reduce complexity -type WorkspaceProcessResult = (PathBuf, WorkspaceInfo, Vec<(String, PathBuf)>); +pub type CrateWorkspaceMap = HashMap>; +pub type CratePathToWorkspaceMap = HashMap; + +type WorkspaceProcessResult = (PathBuf, WorkspaceInfo); type ParallelProcessResults = Vec; #[derive(Debug, Clone)] pub struct WorkspaceAnalyzer { workspaces: HashMap, - crate_to_workspace: HashMap, + crate_to_workspaces: CrateWorkspaceMap, + crate_path_to_workspace: CratePathToWorkspaceMap, crate_to_paths: HashMap>, } @@ -226,6 +230,8 @@ impl CrateMemberBuilder { pub struct Dependency { name: String, target: Option, + path: Option, + is_workspace: bool, } impl Dependency { @@ -240,12 +246,22 @@ impl Dependency { pub fn target(&self) -> Option<&str> { self.target.as_deref() } + + pub fn path(&self) -> Option<&PathBuf> { + self.path.as_ref() + } + + pub fn is_workspace(&self) -> bool { + self.is_workspace + } } #[derive(Default)] pub struct DependencyBuilder { name: Option, target: Option, + path: Option, + is_workspace: bool, } #[derive(Error, Debug, Diagnostic)] @@ -263,6 +279,8 @@ impl From<&Dependency> for DependencyBuilder { Self { name: Some(dep.name().to_string()), target: dep.target().map(|t| t.to_string()), + path: dep.path().cloned(), + is_workspace: dep.is_workspace(), } } } @@ -278,10 +296,22 @@ impl DependencyBuilder { self } + pub fn with_path(mut self, path: impl Into) -> Self { + self.path = Some(path.into()); + self + } + + pub fn with_is_workspace(mut self, is_workspace: bool) -> Self { + self.is_workspace = is_workspace; + self + } + pub fn build(self) -> Result { Ok(Dependency { name: self.name.ok_or(DependencyBuilderError::MissingName)?, target: self.target, + path: self.path, + is_workspace: self.is_workspace, }) } } @@ -296,7 +326,8 @@ impl WorkspaceAnalyzer { pub fn new() -> Self { Self { workspaces: HashMap::new(), - crate_to_workspace: HashMap::new(), + crate_to_workspaces: HashMap::new(), + crate_path_to_workspace: HashMap::new(), crate_to_paths: HashMap::new(), } } @@ -305,8 +336,12 @@ impl WorkspaceAnalyzer { &self.workspaces } - pub fn crate_to_workspace(&self) -> &HashMap { - &self.crate_to_workspace + pub fn crate_to_workspace(&self) -> &CrateWorkspaceMap { + &self.crate_to_workspaces + } + + pub fn crate_path_to_workspace(&self) -> &CratePathToWorkspaceMap { + &self.crate_path_to_workspace } pub fn crate_to_paths(&self) -> &HashMap> { @@ -395,19 +430,41 @@ impl WorkspaceAnalyzer { } fn merge_results(&mut self, results: ParallelProcessResults) { - for (path, info, crate_mappings) in results { - // Populate crate_to_paths mapping from the workspace info - for member in &info.members { - self.crate_to_paths + for (workspace_path, mut info) in results { + let workspace_key = workspace_path + .canonicalize() + .unwrap_or_else(|_| workspace_path.clone()); + + // Populate crate lookups from the workspace info + for member in &mut info.members { + let crate_path = member + .path + .canonicalize() + .unwrap_or_else(|_| member.path.clone()); + + member.path = crate_path.clone(); + + if let Some(entry) = self.crate_to_paths.get_mut(&member.name) { + if !entry.iter().any(|existing| existing == &crate_path) { + entry.push(crate_path.clone()); + } + } else { + self.crate_to_paths + .entry(member.name.clone()) + .or_default() + .push(crate_path.clone()); + } + + self.crate_to_workspaces .entry(member.name.clone()) .or_default() - .push(member.path.clone()); - } + .insert(workspace_key.clone()); - self.workspaces.insert(path, info); - for (crate_name, workspace_path) in crate_mappings { - self.crate_to_workspace.insert(crate_name, workspace_path); + self.crate_path_to_workspace + .insert(crate_path, workspace_key.clone()); } + + self.workspaces.insert(workspace_key, info); } } @@ -445,7 +502,7 @@ impl WorkspaceAnalyzer { root: WorkspaceRoot, ) -> Result { // Process members in parallel and collect both results and errors - let results: Vec> = root + let results: Vec> = root .members() .par_iter() .map(|member| { @@ -456,18 +513,17 @@ impl WorkspaceAnalyzer { root.workspace_dependencies(), root.path(), ) - .map(|crate_member| (crate_member, member.name().to_string())) .wrap_err_with(|| format!("Failed to analyze crate '{}'", member.name())) }) .collect(); // Separate successful results from errors - let mut members_with_mappings = Vec::new(); + let mut members = Vec::new(); let mut crate_errors = Vec::new(); for result in results { match result { - Ok(data) => members_with_mappings.push(data), + Ok(member) => members.push(member), Err(e) => crate_errors.push(e), } } @@ -477,23 +533,13 @@ impl WorkspaceAnalyzer { eprintln!("{} {}", style("āš ").yellow(), error); } - let members: Vec = members_with_mappings - .iter() - .map(|(m, _)| m.clone()) - .collect(); - - let crate_mappings: Vec<(String, PathBuf)> = members_with_mappings - .into_iter() - .map(|(_, name)| (name, root.path().clone())) - .collect(); - let workspace_info = WorkspaceInfo { name: root.name().to_string(), members, is_standalone: root.is_standalone(), }; - Ok((root.path().clone(), workspace_info, crate_mappings)) + Ok((root.path().clone(), workspace_info)) } fn analyze_crate_member( @@ -520,7 +566,9 @@ impl WorkspaceAnalyzer { #[cfg(test)] mod tests { + use std::collections::BTreeSet; use std::fs; + use std::path::PathBuf; use tempfile::TempDir; @@ -601,4 +649,90 @@ crate-a = { path = "../crate-a" } let crate_b = ws.members.iter().find(|m| m.name == "crate-b").unwrap(); assert_eq!(crate_b.dev_dependencies.len(), 1); // crate-a } + + #[test] + fn test_duplicate_crate_names_map_to_multiple_workspaces() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + + let workspace_a = root.join("workspace-a"); + let workspace_b = root.join("workspace-b"); + + fs::create_dir_all(workspace_a.join("shared/src")).unwrap(); + fs::create_dir_all(workspace_b.join("shared/src")).unwrap(); + + fs::write( + workspace_a.join("Cargo.toml"), + r#" +[workspace] +members = ["shared"] +"#, + ) + .unwrap(); + fs::write( + workspace_a.join("shared/Cargo.toml"), + "[package]\nname = \"shared\"\n", + ) + .unwrap(); + fs::write(workspace_a.join("shared/src/lib.rs"), "pub fn a() {}").unwrap(); + + fs::write( + workspace_b.join("Cargo.toml"), + r#" +[workspace] +members = ["shared"] +"#, + ) + .unwrap(); + fs::write( + workspace_b.join("shared/Cargo.toml"), + "[package]\nname = \"shared\"\n", + ) + .unwrap(); + fs::write(workspace_b.join("shared/src/lib.rs"), "pub fn b() {}").unwrap(); + + let mut analyzer = WorkspaceAnalyzer::new(); + analyzer + .discover_workspaces(&[root.to_path_buf()], None) + .unwrap(); + + let shared_entries = analyzer + .crate_to_workspace() + .get("shared") + .expect("shared crate should be indexed"); + + let expected_ws_paths: BTreeSet = [ + workspace_a.canonicalize().unwrap(), + workspace_b.canonicalize().unwrap(), + ] + .into_iter() + .collect(); + + let actual_ws_paths: BTreeSet = shared_entries + .iter() + .map(|p| p.canonicalize().unwrap()) + .collect(); + + assert_eq!(actual_ws_paths, expected_ws_paths); + + let crate_paths = analyzer + .crate_to_paths() + .get("shared") + .expect("crate paths should be tracked"); + assert_eq!(crate_paths.len(), 2); + + for crate_path in crate_paths { + let resolved = crate_path.canonicalize().unwrap(); + let ws = analyzer + .crate_path_to_workspace() + .get(crate_path) + .expect("crate path should map to workspace"); + let ws_abs = ws.canonicalize().unwrap(); + assert!(expected_ws_paths.contains(&ws_abs)); + assert!( + resolved.starts_with(&ws_abs), + "crate {resolved:?} should live under workspace {ws_abs:?}" + ); + } + } } diff --git a/src/analyzer/dependency_classifier.rs b/src/analyzer/dependency_classifier.rs index 865d41f..363ece9 100644 --- a/src/analyzer/dependency_classifier.rs +++ b/src/analyzer/dependency_classifier.rs @@ -90,7 +90,18 @@ impl DependencyClassifier { continue; } - if let Ok(dependency) = Self::create_dependency(&dep_name, &dep_type) { + let dependency_path = if CargoToml::is_workspace_dependency(&dep) { + workspace_deps.get(&dep_name).cloned() + } else { + CargoToml::extract_path(&dep).map(std::path::PathBuf::from) + }; + + if let Ok(dependency) = Self::create_dependency( + &dep_name, + &dep_type, + dependency_path, + CargoToml::is_workspace_dependency(&dep), + ) { classifier.add_dependency(dependency, dep_type); } } @@ -116,8 +127,16 @@ impl DependencyClassifier { fn create_dependency( dep_name: &str, dep_type: &TomlDependencyType, + path: Option, + is_workspace: bool, ) -> Result { - let mut builder = Dependency::builder().with_name(dep_name); + let mut builder = Dependency::builder() + .with_name(dep_name) + .with_is_workspace(is_workspace); + + if let Some(path) = path { + builder = builder.with_path(path); + } match dep_type { TomlDependencyType::Target(t) @@ -177,9 +196,13 @@ mod tests { #[test] fn test_create_dependency_normal() { - let dep = - DependencyClassifier::create_dependency("test-crate", &TomlDependencyType::Normal) - .expect("Failed to create dependency"); + let dep = DependencyClassifier::create_dependency( + "test-crate", + &TomlDependencyType::Normal, + None, + false, + ) + .expect("Failed to create dependency"); assert_eq!(dep.name(), "test-crate"); assert!(dep.target().is_none()); } @@ -189,6 +212,8 @@ mod tests { let dep = DependencyClassifier::create_dependency( "test-crate", &TomlDependencyType::Target("wasm32-unknown-unknown".to_string()), + None, + false, ) .expect("Failed to create dependency"); assert_eq!(dep.name(), "test-crate"); diff --git a/src/commands/affected.rs b/src/commands/affected.rs index b94ede4..2bb0a08 100644 --- a/src/commands/affected.rs +++ b/src/commands/affected.rs @@ -8,7 +8,7 @@ use petgraph::graph::{DiGraph, NodeIndex}; use petgraph::visit::EdgeRef; use serde::{Deserialize, Serialize}; -use crate::analyzer::WorkspaceInfo; +use crate::analyzer::{CratePathToWorkspaceMap, Dependency, WorkspaceInfo}; use crate::cli::Commands; use crate::common::FromCommand; use crate::config::AffectedConfig; @@ -38,6 +38,26 @@ pub struct AffectedCrate { pub is_standalone: bool, } +#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] +pub(crate) struct CrateId { + name: String, + path: PathBuf, +} + +impl CrateId { + fn new(name: String, path: PathBuf) -> Self { + Self { name, path } + } + + pub fn name(&self) -> &str { + &self.name + } + + pub fn path(&self) -> &Path { + &self.path + } +} + impl FromCommand for AffectedConfig { fn from_command(command: Commands) -> Result { match command { @@ -82,17 +102,16 @@ pub fn execute_affected_command(command: Commands) -> Result<()> { /// Analysis of affected crates and workspaces based on changed files pub struct AffectedAnalysis { - /// Map from (crate_name, crate_path) to workspace path - crate_path_to_workspace: HashMap<(String, PathBuf), PathBuf>, - /// Map from crate name to crate paths (can have multiple paths per crate - /// name) - crate_to_paths: HashMap>, + /// Map from crate identifier to its workspace path + crate_workspace_index: HashMap, + /// Map from crate path to crate identifier for quick lookup + crate_path_index: HashMap, /// Map from workspace path to workspace info workspaces: HashMap, - /// Crate-level dependency graph - crate_graph: DiGraph, - /// Map from crate name to node index in the graph - crate_node_indices: HashMap, + /// Crate-level dependency graph keyed by crate identifier + crate_graph: DiGraph, + /// Map from crate identifier to node index in the graph + crate_node_indices: HashMap, } impl AffectedAnalysis { @@ -102,75 +121,89 @@ impl AffectedAnalysis { pub fn new( workspaces: &HashMap, - _crate_to_workspace: &HashMap, - crate_to_paths: &HashMap>, + crate_path_to_workspace: &CratePathToWorkspaceMap, filter: DependencyFilter, ) -> Result { let mut crate_graph = DiGraph::new(); let mut crate_node_indices = HashMap::new(); - let mut crate_path_to_workspace = HashMap::new(); + let mut crate_workspace_index = HashMap::new(); + let mut crate_path_index = HashMap::new(); + let mut crate_ids_by_name: HashMap> = HashMap::new(); // First pass: create nodes for all crates and build proper mappings for (workspace_path, workspace_info) in workspaces { + let workspace_path = workspace_path.clone(); for member in workspace_info.members() { - // Add node to graph - let node_idx = crate_graph.add_node(member.name().to_string()); - crate_node_indices.insert(member.name().to_string(), node_idx); - - // Map (crate_name, crate_path) to workspace - crate_path_to_workspace.insert( - (member.name().to_string(), member.path().clone()), - workspace_path.clone(), + let crate_path = member.path().to_path_buf(); + let crate_id = CrateId::new(member.name().to_string(), crate_path.clone()); + let node_idx = crate_graph.add_node(crate_id.clone()); + crate_node_indices.insert(crate_id.clone(), node_idx); + + crate_workspace_index.insert( + crate_id.clone(), + crate_path_to_workspace + .get(&crate_path) + .cloned() + .unwrap_or_else(|| workspace_path.clone()), ); + + crate_path_index.insert(crate_path.clone(), crate_id.clone()); + + crate_ids_by_name + .entry(crate_id.name().to_string()) + .or_default() + .push(crate_id); } } // Second pass: add edges based on dependencies - for workspace_info in workspaces.values() { + for (workspace_path, workspace_info) in workspaces { for member in workspace_info.members() { - if let Some(&from_idx) = crate_node_indices.get(member.name()) { - // Add edges for all dependency types - for dep in member.dependencies() { - if let Some(&to_idx) = crate_node_indices.get(dep.name()) { - crate_graph.add_edge(from_idx, to_idx, ()); - } - } - - // Include dev dependencies unless excluded - if filter.include_dev() { - for dep in member.dev_dependencies() { - if let Some(&to_idx) = crate_node_indices.get(dep.name()) { - crate_graph.add_edge(from_idx, to_idx, ()); - } - } - } + let crate_path = member.path().to_path_buf(); + let Some(from_id) = crate_path_index.get(&crate_path).cloned() else { + continue; + }; + let &from_idx = crate_node_indices + .get(&from_id) + .expect("crate node must exist for analyzed member"); + + let mut ctx = DependencyGraphContext { + crate_graph: &mut crate_graph, + crate_node_indices: &crate_node_indices, + crate_ids_by_name: &crate_ids_by_name, + crate_path_index: &crate_path_index, + workspace_path: workspace_path.as_path(), + }; + + connect_dependencies(member.dependencies(), true, from_idx, &from_id, &mut ctx); + + connect_dependencies( + member.dev_dependencies(), + filter.include_dev(), + from_idx, + &from_id, + &mut ctx, + ); - // Include build dependencies unless excluded - if filter.include_build() { - for dep in member.build_dependencies() { - if let Some(&to_idx) = crate_node_indices.get(dep.name()) { - crate_graph.add_edge(from_idx, to_idx, ()); - } - } - } + connect_dependencies( + member.build_dependencies(), + filter.include_build(), + from_idx, + &from_id, + &mut ctx, + ); - // Include target-specific dependencies unless excluded - if filter.include_target() { - for target_deps in member.target_dependencies().values() { - for dep in target_deps { - if let Some(&to_idx) = crate_node_indices.get(dep.name()) { - crate_graph.add_edge(from_idx, to_idx, ()); - } - } - } + if filter.include_target() { + for deps in member.target_dependencies().values() { + connect_dependencies(deps, true, from_idx, &from_id, &mut ctx); } } } } Ok(Self { - crate_path_to_workspace, - crate_to_paths: crate_to_paths.clone(), + crate_workspace_index, + crate_path_index, workspaces: workspaces.clone(), crate_graph, crate_node_indices, @@ -182,8 +215,7 @@ impl AffectedAnalysis { &self, abs_file: &Path, cwd: &Path, - directly_affected_crates: &mut HashSet, - directly_affected_crate_paths: &mut HashSet<(String, PathBuf)>, + directly_affected_crates: &mut HashSet, ) -> bool { // Check if this file is at a workspace root for ws_path in self.workspaces.keys() { @@ -200,11 +232,12 @@ impl AffectedAnalysis { { // This is a workspace-level Cargo file // Mark all crates in this workspace as directly affected - for ((crate_name, crate_path), crate_ws_path) in &self.crate_path_to_workspace { - if crate_ws_path == ws_path { - directly_affected_crates.insert(crate_name.clone()); - directly_affected_crate_paths - .insert((crate_name.clone(), crate_path.clone())); + for (crate_id, crate_ws_path) in &self.crate_workspace_index { + let crate_ws_abs = crate_ws_path + .canonicalize() + .unwrap_or_else(|_| crate_ws_path.clone()); + if crate_ws_abs == abs_ws_path { + directly_affected_crates.insert(crate_id.clone()); } } return true; @@ -215,8 +248,7 @@ impl AffectedAnalysis { /// Analyze which crates and workspaces are affected by the given files pub fn analyze_affected_files(&self, files: &[String]) -> AffectedResult { - let mut directly_affected_crates = HashSet::new(); - let mut directly_affected_crate_paths = HashSet::new(); + let mut directly_affected_crates: HashSet = HashSet::new(); let mut unmatched_files = Vec::new(); // Get current directory once for efficiency @@ -232,7 +264,6 @@ impl AffectedAnalysis { } else { cwd.join(&file_path) }; - // Try to canonicalize to resolve symlinks (e.g., /private/var -> /var on macOS) let abs_file = abs_file.canonicalize().unwrap_or(abs_file); // Check if this is a Cargo.lock or Cargo.toml file @@ -241,53 +272,13 @@ impl AffectedAnalysis { // Handle workspace-level Cargo files if is_cargo_file - && self.handle_workspace_cargo_file( - &abs_file, - &cwd, - &mut directly_affected_crates, - &mut directly_affected_crate_paths, - ) + && self.handle_workspace_cargo_file(&abs_file, &cwd, &mut directly_affected_crates) { continue; } - // Try to find by checking if file is under any crate directory - // When multiple crates match, prefer the one with the longest matching path - let mut best_match: Option<(String, PathBuf, usize)> = None; - - for (crate_name, crate_paths) in &self.crate_to_paths { - for crate_path in crate_paths { - // Normalize the crate path to absolute and resolve symlinks - let abs_crate = if crate_path.is_absolute() { - crate_path.clone() - } else { - cwd.join(crate_path) - }; - // Try to canonicalize to resolve symlinks - let abs_crate = abs_crate.canonicalize().unwrap_or(abs_crate); - - // Check if the file is under this crate's directory - if abs_file.starts_with(&abs_crate) { - let match_len = abs_crate.as_os_str().len(); - match &best_match { - None => { - best_match = - Some((crate_name.clone(), crate_path.clone(), match_len)) - } - Some((_, _, best_len)) => { - if match_len > *best_len { - best_match = - Some((crate_name.clone(), crate_path.clone(), match_len)); - } - } - } - } - } - } - - if let Some((crate_name, crate_path, _)) = best_match { - directly_affected_crates.insert(crate_name.clone()); - directly_affected_crate_paths.insert((crate_name, crate_path)); + if let Some(crate_id) = self.find_crate_for_file(&abs_file) { + directly_affected_crates.insert(crate_id); } else { unmatched_files.push(file.clone()); } @@ -295,45 +286,20 @@ impl AffectedAnalysis { // Find all crates affected by reverse dependencies let mut all_affected_crates = directly_affected_crates.clone(); - for crate_name in &directly_affected_crates { - if let Some(&node_idx) = self.crate_node_indices.get(crate_name) { + for crate_id in directly_affected_crates.iter() { + if let Some(&node_idx) = self.crate_node_indices.get(crate_id) { self.find_reverse_dependencies(node_idx, &mut all_affected_crates); } } - // Map directly affected crates to workspaces using the exact paths that were - // matched - let directly_affected_workspaces: HashSet = directly_affected_crate_paths + let directly_affected_workspaces: HashSet = directly_affected_crates .iter() - .filter_map(|(crate_name, crate_path)| { - self.crate_path_to_workspace - .get(&(crate_name.clone(), crate_path.clone())) - .and_then(|ws_path| self.workspaces.get(ws_path)) - .map(|ws_info| ws_info.name().to_string()) - }) + .filter_map(|crate_id| self.workspace_name(crate_id)) .collect(); - // For all affected crates (including reverse dependencies), we need to find - // their workspaces let all_affected_workspaces: HashSet = all_affected_crates .iter() - .flat_map(|crate_name| { - // A crate might exist in multiple workspaces, so collect all of them - self.crate_to_paths - .get(crate_name) - .map(|paths| { - paths - .iter() - .filter_map(|path| { - self.crate_path_to_workspace - .get(&(crate_name.clone(), path.clone())) - .and_then(|ws_path| self.workspaces.get(ws_path)) - .map(|ws_info| ws_info.name().to_string()) - }) - .collect::>() - }) - .unwrap_or_default() - }) + .filter_map(|crate_id| self.workspace_name(crate_id)) .collect(); AffectedResult { @@ -345,7 +311,7 @@ impl AffectedAnalysis { } } - fn find_reverse_dependencies(&self, node_idx: NodeIndex, affected: &mut HashSet) { + fn find_reverse_dependencies(&self, node_idx: NodeIndex, affected: &mut HashSet) { use petgraph::Direction; for edge in self @@ -353,54 +319,169 @@ impl AffectedAnalysis { .edges_directed(node_idx, Direction::Incoming) { let source_idx = edge.source(); - let source_crate = &self.crate_graph[source_idx]; + let source_crate = self.crate_graph[source_idx].clone(); if affected.insert(source_crate.clone()) { // Recursively find more reverse dependencies self.find_reverse_dependencies(source_idx, affected); } } } + + fn find_crate_for_file(&self, abs_file: &Path) -> Option { + let canonical = abs_file + .canonicalize() + .unwrap_or_else(|_| abs_file.to_path_buf()); + + let mut best_match: Option<(usize, CrateId)> = None; + + for (crate_path, crate_id) in &self.crate_path_index { + let match_path = (canonical.starts_with(crate_path) + || abs_file.starts_with(crate_path)) + .then_some(crate_path); + + if let Some(path) = match_path { + let match_len = path.components().count(); + match &best_match { + None => best_match = Some((match_len, crate_id.clone())), + Some((best_len, _)) if match_len > *best_len => { + best_match = Some((match_len, crate_id.clone())) + } + _ => {} + } + } + } + + best_match.map(|(_, id)| id) + } + + pub(crate) fn workspace_name(&self, crate_id: &CrateId) -> Option { + self.crate_workspace_index + .get(crate_id) + .and_then(|ws_path| self.workspaces.get(ws_path)) + .map(|ws| ws.name().to_string()) + } +} + +struct DependencyGraphContext<'a> { + crate_graph: &'a mut DiGraph, + crate_node_indices: &'a HashMap, + crate_ids_by_name: &'a HashMap>, + crate_path_index: &'a HashMap, + workspace_path: &'a Path, +} + +fn connect_dependencies( + deps: &[Dependency], + include: bool, + from_idx: NodeIndex, + from_id: &CrateId, + ctx: &mut DependencyGraphContext<'_>, +) { + if !include { + return; + } + + for dep in deps { + if let Some(to_idx) = resolve_dependency_crate_id( + dep, + from_id, + ctx.workspace_path, + ctx.crate_ids_by_name, + ctx.crate_path_index, + ) + .and_then(|target_id| ctx.crate_node_indices.get(&target_id).copied()) + { + ctx.crate_graph.add_edge(from_idx, to_idx, ()); + } + } +} + +fn resolve_dependency_crate_id( + dep: &Dependency, + from_id: &CrateId, + workspace_path: &Path, + crate_ids_by_name: &HashMap>, + crate_path_index: &HashMap, +) -> Option { + if let Some(dep_path) = dep.path() { + let base = if dep.is_workspace() { + workspace_path + } else { + from_id.path() + }; + + let absolute = if dep_path.is_absolute() { + dep_path.clone() + } else { + base.join(dep_path) + }; + + let canonical = absolute.canonicalize().unwrap_or_else(|_| absolute.clone()); + + crate_path_index + .get(&canonical) + .or_else(|| crate_path_index.get(&absolute)) + .cloned() + .or_else(|| { + crate_path_index + .iter() + .find_map(|(candidate_path, candidate_id)| { + if canonical.starts_with(candidate_path) + || candidate_path.starts_with(&canonical) + { + Some(candidate_id.clone()) + } else { + None + } + }) + }) + } else { + crate_ids_by_name.get(dep.name()).and_then(|ids| { + if ids.len() == 1 { + Some(ids[0].clone()) + } else { + None + } + }) + } } pub struct AffectedResult { - pub directly_affected_crates: HashSet, - pub all_affected_crates: HashSet, - pub directly_affected_workspaces: HashSet, - pub all_affected_workspaces: HashSet, - pub unmatched_files: Vec, + pub(crate) directly_affected_crates: HashSet, + pub(crate) all_affected_crates: HashSet, + pub(crate) directly_affected_workspaces: HashSet, + pub(crate) all_affected_workspaces: HashSet, + pub(crate) unmatched_files: Vec, } impl AffectedResult { pub fn to_json_report(&self, analysis: &AffectedAnalysis) -> AffectedJsonReport { let mut affected_crates = Vec::new(); - for crate_name in &self.all_affected_crates { - // Get workspace info for this crate (prefer first path found) + for crate_id in &self.all_affected_crates { let workspace_info = analysis - .crate_to_paths - .get(crate_name) - .and_then(|paths| paths.first()) - .and_then(|path| { - analysis - .crate_path_to_workspace - .get(&(crate_name.clone(), path.clone())) - .and_then(|ws_path| analysis.workspaces.get(ws_path)) - }); + .crate_workspace_index + .get(crate_id) + .and_then(|ws_path| analysis.workspaces.get(ws_path)); let (workspace_name, is_standalone) = workspace_info .map(|ws| (ws.name().to_string(), ws.is_standalone())) .unwrap_or_else(|| ("unknown".to_string(), false)); affected_crates.push(AffectedCrate { - name: crate_name.clone(), + name: crate_id.name().to_string(), workspace: workspace_name, - is_directly_affected: self.directly_affected_crates.contains(crate_name), + is_directly_affected: self.directly_affected_crates.contains(crate_id), is_standalone, }); } // Sort affected crates by name for deterministic output - affected_crates.sort_by(|a, b| a.name.cmp(&b.name)); + affected_crates.sort_by(|a, b| { + a.name + .cmp(&b.name) + .then_with(|| a.workspace.cmp(&b.workspace)) + }); // Create affected workspace objects with paths let mut affected_workspaces: Vec = self @@ -423,9 +504,13 @@ impl AffectedResult { .collect(); affected_workspaces.sort_by(|a, b| a.name.cmp(&b.name)); - let mut directly_affected_crates: Vec = - self.directly_affected_crates.iter().cloned().collect(); + let mut directly_affected_crates: Vec = self + .directly_affected_crates + .iter() + .map(|crate_id| crate_id.name().to_string()) + .collect(); directly_affected_crates.sort(); + directly_affected_crates.dedup(); let mut directly_affected_workspaces: Vec = self .directly_affected_workspaces @@ -465,6 +550,14 @@ mod tests { use super::*; + fn contains_crate(crates: &HashSet, name: &str) -> bool { + crates.iter().any(|id| id.name() == name) + } + + fn count_crate(crates: &HashSet, name: &str) -> usize { + crates.iter().filter(|id| id.name() == name).count() + } + fn create_test_workspace_with_duplicates() -> TempDir { let temp = TempDir::new().unwrap(); let root = temp.path(); @@ -653,8 +746,7 @@ version = "0.1.0" AffectedAnalysis::new( analyzer.workspaces(), - analyzer.crate_to_workspace(), - analyzer.crate_to_paths(), + analyzer.crate_path_to_workspace(), crate::dependency_filter::DependencyFilter::default(), ) .unwrap() @@ -672,7 +764,14 @@ version = "0.1.0" )]; let result_a = analysis.analyze_affected_files(&files_a); - assert!(result_a.directly_affected_crates.contains("phoenix-v2-api")); + assert!(contains_crate( + &result_a.directly_affected_crates, + "phoenix-v2-api" + )); + assert_eq!( + count_crate(&result_a.directly_affected_crates, "phoenix-v2-api"), + 1 + ); assert!( result_a .directly_affected_workspaces @@ -691,7 +790,14 @@ version = "0.1.0" )]; let result_b = analysis.analyze_affected_files(&files_b); - assert!(result_b.directly_affected_crates.contains("phoenix-v2-api")); + assert!(contains_crate( + &result_b.directly_affected_crates, + "phoenix-v2-api" + )); + assert_eq!( + count_crate(&result_b.directly_affected_crates, "phoenix-v2-api"), + 1 + ); assert!( result_b .directly_affected_workspaces @@ -704,6 +810,32 @@ version = "0.1.0" ); } + #[test] + fn test_duplicate_crate_names_multiple_changes() { + let temp = create_test_workspace_with_duplicates(); + let analysis = build_test_analysis(temp.path()); + + let files = vec![ + format!( + "{}/workspace-a/phoenix-v2-api/src/lib.rs", + temp.path().display() + ), + format!( + "{}/workspace-b/phoenix-v2-api/src/main.rs", + temp.path().display() + ), + ]; + + let result = analysis.analyze_affected_files(&files); + + assert_eq!( + count_crate(&result.directly_affected_crates, "phoenix-v2-api"), + 2 + ); + assert!(result.directly_affected_workspaces.contains("workspace-a")); + assert!(result.directly_affected_workspaces.contains("workspace-b")); + } + #[test] fn test_reverse_dependencies() { let temp = create_simple_test_workspace(); @@ -717,12 +849,12 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // crate-b should be directly affected - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert_eq!(result.directly_affected_crates.len(), 1); // crate-a should be affected via reverse dependency - assert!(result.all_affected_crates.contains("crate-a")); - assert!(result.all_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.all_affected_crates, "crate-a")); + assert!(contains_crate(&result.all_affected_crates, "crate-b")); assert_eq!(result.all_affected_crates.len(), 2); } @@ -756,7 +888,7 @@ version = "0.1.0" let files = vec!["my-workspace/crate-a/src/lib.rs".to_string()]; let result = analysis.analyze_affected_files(&files); - assert!(result.directly_affected_crates.contains("crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); // Restore original directory std::env::set_current_dir(original_dir).unwrap(); @@ -811,7 +943,7 @@ version = "0.1.0" // Should only count crate-a once assert_eq!(result.directly_affected_crates.len(), 1); - assert!(result.directly_affected_crates.contains("crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); } #[test] @@ -827,7 +959,10 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // Only consumer-crate should be directly affected - assert!(result.directly_affected_crates.contains("consumer-crate")); + assert!(contains_crate( + &result.directly_affected_crates, + "consumer-crate" + )); // Workspace B should be affected assert!(result.directly_affected_workspaces.contains("workspace-b")); @@ -1150,8 +1285,8 @@ version = "0.1.0" // Both crates should be directly affected by their Cargo.toml files assert_eq!(result.directly_affected_crates.len(), 2); - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); // The workspace should be affected assert!(result.directly_affected_workspaces.contains("my-workspace")); @@ -1168,8 +1303,8 @@ version = "0.1.0" // Workspace Cargo.toml should affect all workspace members assert_eq!(result.directly_affected_crates.len(), 2); - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert!(result.directly_affected_workspaces.contains("my-workspace")); assert!(result.unmatched_files.is_empty()); } @@ -1187,15 +1322,14 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // Standalone crate's Cargo.lock should map to the crate - assert!( - result - .directly_affected_crates - .contains("standalone-test-crate") - ); + assert!(contains_crate( + &result.directly_affected_crates, + "standalone-test-crate", + )); // Workspace Cargo.lock should affect all workspace members - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); // No unmatched files assert!(result.unmatched_files.is_empty()); @@ -1211,8 +1345,8 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // All workspace members should be directly affected - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert_eq!(result.directly_affected_crates.len(), 2); // The workspace should be affected @@ -1232,8 +1366,8 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // All workspace members should be directly affected - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert_eq!(result.directly_affected_crates.len(), 2); // The workspace should be affected @@ -1256,16 +1390,15 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // Only the standalone crate should be affected - assert!( - result - .directly_affected_crates - .contains("standalone-test-crate") - ); + assert!(contains_crate( + &result.directly_affected_crates, + "standalone-test-crate", + )); assert_eq!(result.directly_affected_crates.len(), 1); // No workspace members should be affected - assert!(!result.directly_affected_crates.contains("crate-a")); - assert!(!result.directly_affected_crates.contains("crate-b")); + assert!(!contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(!contains_crate(&result.directly_affected_crates, "crate-b")); // No unmatched files assert!(result.unmatched_files.is_empty()); @@ -1284,13 +1417,13 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // crate-b should be directly affected - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert_eq!(result.directly_affected_crates.len(), 1); // crate-a depends on crate-b, so it should be affected through reverse // dependencies - assert!(result.all_affected_crates.contains("crate-a")); - assert!(result.all_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.all_affected_crates, "crate-a")); + assert!(contains_crate(&result.all_affected_crates, "crate-b")); assert_eq!(result.all_affected_crates.len(), 2); } @@ -1308,8 +1441,8 @@ version = "0.1.0" let result = analysis.analyze_affected_files(&files); // All crates should be directly affected - assert!(result.directly_affected_crates.contains("crate-a")); - assert!(result.directly_affected_crates.contains("crate-b")); + assert!(contains_crate(&result.directly_affected_crates, "crate-a")); + assert!(contains_crate(&result.directly_affected_crates, "crate-b")); assert_eq!(result.directly_affected_crates.len(), 2); // No unmatched files @@ -1410,9 +1543,18 @@ name = "outer-crate" )]; let result = analysis.analyze_affected_files(&files); - assert!(result.directly_affected_crates.contains("inner-crate-a")); - assert!(result.directly_affected_crates.contains("inner-crate-b")); - assert!(!result.directly_affected_crates.contains("outer-crate")); + assert!(contains_crate( + &result.directly_affected_crates, + "inner-crate-a" + )); + assert!(contains_crate( + &result.directly_affected_crates, + "inner-crate-b" + )); + assert!(!contains_crate( + &result.directly_affected_crates, + "outer-crate" + )); assert_eq!(result.directly_affected_crates.len(), 2); } } diff --git a/src/commands/deps.rs b/src/commands/deps.rs index 8c44eaa..7767f64 100644 --- a/src/commands/deps.rs +++ b/src/commands/deps.rs @@ -9,7 +9,7 @@ use petgraph::graph::{DiGraph, NodeIndex}; use petgraph::visit::{EdgeRef, IntoNodeReferences}; use serde::{Deserialize, Serialize}; -use crate::analyzer::WorkspaceInfo; +use crate::analyzer::{CrateWorkspaceMap, WorkspaceInfo}; use crate::cli::Commands; use crate::common::{ConfigBuilder, FromCommand}; use crate::config::WorkspaceDepsConfig; @@ -85,7 +85,7 @@ pub struct WorkspaceDependencyAnalysis { impl WorkspaceDependencyAnalysis { pub fn new( workspaces: &HashMap, - _crate_to_workspace: &HashMap, + _crate_to_workspace: &CrateWorkspaceMap, graph: &DiGraph, ) -> Self { // Build node index lookup @@ -458,17 +458,17 @@ mod tests { use petgraph::graph::DiGraph; use super::*; - use crate::analyzer::WorkspaceInfo; + use crate::analyzer::{CrateWorkspaceMap, WorkspaceInfo}; use crate::graph::{DependencyEdge, WorkspaceNode}; fn create_test_graph() -> ( DiGraph, HashMap, - HashMap, + CrateWorkspaceMap, ) { let mut graph = DiGraph::new(); let mut workspaces = HashMap::new(); - let crate_to_workspace = HashMap::new(); + let crate_to_workspace = CrateWorkspaceMap::new(); // Create workspace nodes let node_a = graph.add_node( diff --git a/src/common.rs b/src/common.rs index 954f804..f9903af 100644 --- a/src/common.rs +++ b/src/common.rs @@ -96,7 +96,7 @@ mod tests { let paths = args.get_paths(); assert_eq!(paths.len(), 1); // Should default to current directory - assert!(paths[0].is_absolute() || paths[0] == PathBuf::from(".")); + assert!(paths[0].is_absolute() || paths[0] == std::path::Path::new(".")); } #[test] diff --git a/src/executors/affected.rs b/src/executors/affected.rs index 42f853e..69060a8 100644 --- a/src/executors/affected.rs +++ b/src/executors/affected.rs @@ -43,6 +43,8 @@ impl CommandExecutor for AffectedExecutor { .build_cross_workspace_graph( analyzer.workspaces(), analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), progress.as_ref(), ) .wrap_err("Failed to build cross-workspace dependency graph")?; @@ -55,8 +57,7 @@ impl CommandExecutor for AffectedExecutor { ); let affected_analysis = AffectedAnalysis::new( analyzer.workspaces(), - analyzer.crate_to_workspace(), - analyzer.crate_to_paths(), + analyzer.crate_path_to_workspace(), filter, )?; @@ -94,6 +95,14 @@ fn generate_json_report( // For direct_only mode, use the to_json_report method but filter to only // directly affected let full_report = result.to_json_report(analysis); + let mut direct_crates: Vec = result + .directly_affected_crates + .iter() + .map(|crate_id| crate_id.name().to_string()) + .collect(); + direct_crates.sort(); + direct_crates.dedup(); + AffectedJsonReport { affected_crates: full_report .affected_crates @@ -105,7 +114,7 @@ fn generate_json_report( .into_iter() .filter(|ws| result.directly_affected_workspaces.contains(&ws.name)) .collect(), - directly_affected_crates: result.directly_affected_crates.iter().cloned().collect(), + directly_affected_crates: direct_crates, directly_affected_workspaces: full_report.directly_affected_workspaces, } } else { @@ -136,10 +145,21 @@ fn generate_human_report( " Crates: {}", result.directly_affected_crates.len() )?; - let mut sorted_crates: Vec<_> = result.directly_affected_crates.iter().collect(); - sorted_crates.sort(); - for crate_name in sorted_crates { - writeln!(output, " - {crate_name}")? + let mut sorted_crates: Vec<_> = result + .directly_affected_crates + .iter() + .map(|crate_id| { + ( + analysis + .workspace_name(crate_id) + .unwrap_or_else(|| "unknown".to_string()), + crate_id.name().to_string(), + ) + }) + .collect(); + sorted_crates.sort_by(|a, b| a.1.cmp(&b.1).then_with(|| a.0.cmp(&b.0))); + for (workspace, crate_name) in sorted_crates { + writeln!(output, " - {crate_name} ({workspace})")? } } writeln!( @@ -169,12 +189,22 @@ fn generate_human_report( )?; if config.show_crates { writeln!(output, " Crates: {}", result.all_affected_crates.len())?; - let mut sorted_all_crates: Vec<_> = result.all_affected_crates.iter().collect(); - sorted_all_crates.sort(); - for crate_name in sorted_all_crates { - if !result.directly_affected_crates.contains(crate_name) { - writeln!(output, " - {crate_name} (indirect)")? - } + let mut sorted_all_crates: Vec<_> = result + .all_affected_crates + .iter() + .filter(|crate_id| !result.directly_affected_crates.contains(*crate_id)) + .map(|crate_id| { + ( + analysis + .workspace_name(crate_id) + .unwrap_or_else(|| "unknown".to_string()), + crate_id.name().to_string(), + ) + }) + .collect(); + sorted_all_crates.sort_by(|a, b| a.1.cmp(&b.1).then_with(|| a.0.cmp(&b.0))); + for (workspace, crate_name) in sorted_all_crates { + writeln!(output, " - {crate_name} ({workspace})")?; } } writeln!( diff --git a/src/executors/analyze.rs b/src/executors/analyze.rs index fd4836d..db66c01 100644 --- a/src/executors/analyze.rs +++ b/src/executors/analyze.rs @@ -62,6 +62,8 @@ impl CommandExecutor for AnalyzeExecutor { .build_cross_workspace_graph( analyzer.workspaces(), analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), progress.as_ref(), ) .wrap_err("Failed to build cross-workspace dependency graph")?; diff --git a/src/executors/check.rs b/src/executors/check.rs index 2aab1ed..10af054 100644 --- a/src/executors/check.rs +++ b/src/executors/check.rs @@ -96,6 +96,8 @@ impl CommandExecutor for CheckExecutor { .build_cross_workspace_graph( analyzer.workspaces(), analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), progress.as_ref(), ) .wrap_err("Failed to build cross-workspace dependency graph")?; diff --git a/src/executors/deps.rs b/src/executors/deps.rs index 94f558e..c1b0016 100644 --- a/src/executors/deps.rs +++ b/src/executors/deps.rs @@ -50,6 +50,8 @@ impl CommandExecutor for DepsExecutor { .build_cross_workspace_graph( analyzer.workspaces(), analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), progress.as_ref(), ) .wrap_err("Failed to build cross-workspace dependency graph")?; diff --git a/src/executors/graph.rs b/src/executors/graph.rs index b40d40e..f0586e3 100644 --- a/src/executors/graph.rs +++ b/src/executors/graph.rs @@ -43,7 +43,13 @@ impl CommandExecutor for GraphExecutor { config.exclude_target, ); graph_builder - .build_cross_workspace_graph(analyzer.workspaces(), analyzer.crate_to_workspace(), None) + .build_cross_workspace_graph( + analyzer.workspaces(), + analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), + None, + ) .wrap_err("Failed to build dependency graph")?; // Detect cycles if highlighting is requested diff --git a/src/graph/builder.rs b/src/graph/builder.rs index 9d51f29..d9d755a 100644 --- a/src/graph/builder.rs +++ b/src/graph/builder.rs @@ -1,15 +1,16 @@ -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::path::{Path, PathBuf}; use miette::{Result, WrapErr}; use petgraph::graph::{DiGraph, NodeIndex}; use super::types::{DependencyEdge, DependencyType, WorkspaceNode}; -use crate::analyzer::{Dependency, DependencyBuilder, WorkspaceInfo}; +use crate::analyzer::{ + CratePathToWorkspaceMap, CrateWorkspaceMap, Dependency, DependencyBuilder, WorkspaceInfo, +}; use crate::common::ConfigBuilder; use crate::dependency_filter::DependencyFilter; use crate::progress::ProgressReporter; -use crate::toml_parser::CargoToml; /// Builder for constructing dependency graphs /// @@ -19,8 +20,14 @@ pub struct DependencyGraphBuilder { graph: DiGraph, workspace_indices: HashMap, filter: DependencyFilter, - // Cache for resolved dependencies - workspace_dependencies_cache: HashMap>, +} + +struct DependencyLookupContext<'a> { + crate_to_workspaces: &'a CrateWorkspaceMap, + crate_path_to_workspace: &'a CratePathToWorkspaceMap, + crate_to_paths: &'a HashMap>, + current_workspace_path: &'a Path, + from_crate_path: &'a Path, } // Types are now imported from the types module @@ -37,7 +44,6 @@ impl DependencyGraphBuilder { graph: DiGraph::new(), workspace_indices: HashMap::new(), filter: DependencyFilter::new(exclude_dev, exclude_build, exclude_target), - workspace_dependencies_cache: HashMap::new(), } } @@ -171,10 +177,80 @@ impl DependencyGraphBuilder { Ok(()) } + fn resolve_dependency_targets( + &self, + dep: &Dependency, + ctx: &DependencyLookupContext<'_>, + ) -> Vec { + let mut targets = BTreeSet::new(); + + if let Some(dep_path) = dep.path() { + let base_path = if dep.is_workspace() { + ctx.current_workspace_path + } else { + ctx.from_crate_path + }; + + let absolute_path = if dep_path.is_absolute() { + dep_path.clone() + } else { + base_path.join(dep_path) + }; + + let canonical = absolute_path + .canonicalize() + .unwrap_or_else(|_| absolute_path.clone()); + + if let Some(ws_path) = ctx.crate_path_to_workspace.get(&canonical) { + targets.insert(ws_path.clone()); + } else if let Some(ws_path) = ctx.crate_path_to_workspace.get(&absolute_path) { + targets.insert(ws_path.clone()); + } + + if targets.is_empty() + && let Some(candidate_paths) = ctx.crate_to_paths.get(dep.name()) + { + for candidate in candidate_paths { + let matches_candidate = + canonical.starts_with(candidate) || candidate.starts_with(&canonical); + + if matches_candidate + && let Some(ws) = ctx.crate_path_to_workspace.get(candidate) + { + targets.insert(ws.clone()); + continue; + } + + if let Ok(candidate_canon) = candidate.canonicalize() + && (canonical.starts_with(&candidate_canon) + || candidate_canon.starts_with(&canonical)) + && let Some(ws) = ctx.crate_path_to_workspace.get(&candidate_canon) + { + targets.insert(ws.clone()); + } + } + } + } + + if targets.is_empty() + && let Some(workspaces) = ctx.crate_to_workspaces.get(dep.name()) + && workspaces.len() == 1 + { + targets.extend(workspaces.iter().cloned()); + } + + targets + .into_iter() + .filter(|path| path != ctx.current_workspace_path) + .collect() + } + pub fn build_cross_workspace_graph( &mut self, workspaces: &HashMap, - crate_to_workspace: &HashMap, + crate_to_workspaces: &CrateWorkspaceMap, + crate_path_to_workspace: &CratePathToWorkspaceMap, + crate_to_paths: &HashMap>, progress: Option<&ProgressReporter>, ) -> Result<()> { // First, create nodes for all workspaces @@ -195,9 +271,6 @@ impl DependencyGraphBuilder { self.workspace_indices.insert(ws_path.clone(), idx); } - // Load workspace dependencies for each workspace - self.load_workspace_dependencies(workspaces)?; - // Then, analyze dependencies and create edges for (ws_path, ws_info) in workspaces { if let Some(p) = progress { @@ -208,6 +281,14 @@ impl DependencyGraphBuilder { // Check each crate in this workspace for member in ws_info.members() { + let lookup_ctx = DependencyLookupContext { + crate_to_workspaces, + crate_path_to_workspace, + crate_to_paths, + current_workspace_path: ws_path.as_path(), + from_crate_path: member.path(), + }; + // Process normal dependencies (always included) for dep in member.dependencies() { self.process_dependency( @@ -215,8 +296,7 @@ impl DependencyGraphBuilder { member.name(), dep, DependencyType::Normal, - crate_to_workspace, - ws_path, + &lookup_ctx, ) .wrap_err_with(|| { format!( @@ -235,8 +315,7 @@ impl DependencyGraphBuilder { member.name(), dep, DependencyType::Dev, - crate_to_workspace, - ws_path, + &lookup_ctx, ) .wrap_err_with(|| { format!( @@ -256,8 +335,7 @@ impl DependencyGraphBuilder { member.name(), dep, DependencyType::Build, - crate_to_workspace, - ws_path, + &lookup_ctx, ) .wrap_err_with(|| { format!( @@ -281,8 +359,7 @@ impl DependencyGraphBuilder { member.name(), &dep, DependencyType::Normal, - crate_to_workspace, - ws_path, + &lookup_ctx, ) .wrap_err_with(|| { format!( @@ -302,32 +379,13 @@ impl DependencyGraphBuilder { Ok(()) } - fn load_workspace_dependencies( - &mut self, - workspaces: &HashMap, - ) -> Result<()> { - // For each workspace, load its workspace-level dependencies - for ws_path in workspaces.keys() { - let workspace_toml_path = ws_path.join("Cargo.toml"); - if workspace_toml_path.exists() - && let Ok(cargo_toml) = CargoToml::parse_file(&workspace_toml_path) - { - let deps = cargo_toml.get_workspace_dependencies(); - self.workspace_dependencies_cache - .insert(ws_path.clone(), deps); - } - } - Ok(()) - } - fn process_dependency( &mut self, from_ws_idx: NodeIndex, from_crate: &str, dep: &Dependency, dep_type: DependencyType, - crate_to_workspace: &HashMap, - current_workspace_path: &Path, + ctx: &DependencyLookupContext<'_>, ) -> Result<()> { // Skip if this specific dependency should be filtered out (e.g., // target-specific) @@ -335,62 +393,21 @@ impl DependencyGraphBuilder { return Ok(()); } - // First check if this dependency directly maps to a known crate - if let Some(target_ws_path) = crate_to_workspace.get(dep.name()) { - // Get the target workspace index - if let Some(&to_ws_idx) = self.workspace_indices.get(target_ws_path) { - // Don't create self-edges (dependencies within the same workspace) - if from_ws_idx != to_ws_idx { - // Check if edge already exists, if not create it - let edge = DependencyEdge::builder() - .with_from_crate(from_crate) - .with_to_crate(dep.name()) - .with_dependency_type(dep_type) - .with_target(dep.target().map(|t| t.to_string())) - .build() - .wrap_err("Failed to build DependencyEdge")?; + let target_workspaces = self.resolve_dependency_targets(dep, ctx); - self.graph.add_edge(from_ws_idx, to_ws_idx, edge); - } - } - } else { - // If not found directly, it might be a workspace dependency - // Check if this workspace has a mapping for this dependency - if let Some(workspace_deps) = self - .workspace_dependencies_cache - .get(current_workspace_path) - && let Some(dep_path) = workspace_deps.get(dep.name()) + for target_ws_path in target_workspaces { + if let Some(&to_ws_idx) = self.workspace_indices.get(&target_ws_path) + && from_ws_idx != to_ws_idx { - // This is a workspace dependency with a path - // Resolve the absolute path - let abs_dep_path = if dep_path.is_relative() { - current_workspace_path.join(dep_path) - } else { - dep_path.clone() - }; + let edge = DependencyEdge::builder() + .with_from_crate(from_crate) + .with_to_crate(dep.name()) + .with_dependency_type(dep_type.clone()) + .with_target(dep.target().map(|t| t.to_string())) + .build() + .wrap_err("Failed to build DependencyEdge")?; - // Now check if this path maps to a known workspace - // We need to check if abs_dep_path is inside any workspace - for (crate_name, ws_path) in crate_to_workspace { - // Check if the dependency path contains a crate from this workspace - if (abs_dep_path - .to_string_lossy() - .contains(&crate_name.replace('-', "_")) - || abs_dep_path.to_string_lossy().contains(crate_name)) - && let Some(&to_ws_idx) = self.workspace_indices.get(ws_path) - && from_ws_idx != to_ws_idx - { - let edge = DependencyEdge::builder() - .with_from_crate(from_crate) - .with_to_crate(crate_name) - .with_dependency_type(dep_type.clone()) - .with_target(dep.target().map(|t| t.to_string())) - .build() - .wrap_err("Failed to build DependencyEdge")?; - self.graph.add_edge(from_ws_idx, to_ws_idx, edge); - break; - } - } + self.graph.add_edge(from_ws_idx, to_ws_idx, edge); } } @@ -404,8 +421,15 @@ impl DependencyGraphBuilder { #[cfg(test)] mod tests { + use std::fs; + + use petgraph::visit::EdgeRef; + use tempfile::TempDir; + use super::*; - use crate::analyzer::{CrateMember, WorkspaceInfo}; + use crate::analyzer::{ + CrateMember, CratePathToWorkspaceMap, CrateWorkspaceMap, WorkspaceAnalyzer, WorkspaceInfo, + }; // Helper function for creating test CrateMember using the builder fn test_crate_member( @@ -424,10 +448,14 @@ mod tests { #[test] fn test_build_simple_graph() { let mut workspaces = HashMap::new(); - let mut crate_to_workspace = HashMap::new(); + let mut crate_to_workspaces = CrateWorkspaceMap::new(); + let mut crate_path_to_workspace = CratePathToWorkspaceMap::new(); + let mut crate_to_paths: HashMap> = HashMap::new(); // Workspace A let ws_a_path = PathBuf::from("/test/workspace-a"); + let crate_a_path = ws_a_path.join("crate-a"); + let crate_b_path = PathBuf::from("/test/workspace-b/crate-b"); workspaces.insert( ws_a_path.clone(), WorkspaceInfo::builder() @@ -435,15 +463,30 @@ mod tests { .with_members(vec![test_crate_member( "crate-a", &ws_a_path, - vec![Dependency::builder().with_name("crate-b").build().unwrap()], + vec![ + Dependency::builder() + .with_name("crate-b") + .with_path(crate_b_path.clone()) + .build() + .unwrap(), + ], )]) .build() .unwrap(), ); - crate_to_workspace.insert("crate-a".to_string(), ws_a_path.clone()); + crate_to_workspaces + .entry("crate-a".to_string()) + .or_default() + .insert(ws_a_path.clone()); + crate_path_to_workspace.insert(crate_a_path.clone(), ws_a_path.clone()); + crate_to_paths + .entry("crate-a".to_string()) + .or_default() + .push(crate_a_path); // Workspace B let ws_b_path = PathBuf::from("/test/workspace-b"); + let ws_b_crate_path = ws_b_path.join("crate-b"); workspaces.insert( ws_b_path.clone(), WorkspaceInfo::builder() @@ -452,11 +495,25 @@ mod tests { .build() .unwrap(), ); - crate_to_workspace.insert("crate-b".to_string(), ws_b_path.clone()); + crate_to_workspaces + .entry("crate-b".to_string()) + .or_default() + .insert(ws_b_path.clone()); + crate_path_to_workspace.insert(ws_b_crate_path.clone(), ws_b_path.clone()); + crate_to_paths + .entry("crate-b".to_string()) + .or_default() + .push(ws_b_crate_path); let mut builder = DependencyGraphBuilder::new(false, false, false); builder - .build_cross_workspace_graph(&workspaces, &crate_to_workspace, None) + .build_cross_workspace_graph( + &workspaces, + &crate_to_workspaces, + &crate_path_to_workspace, + &crate_to_paths, + None, + ) .unwrap(); assert_eq!(builder.graph.node_count(), 2); @@ -834,4 +891,70 @@ mod tests { assert!(node_names.contains(&"workspace-b/crate-b1".to_string())); assert!(node_names.contains(&"workspace-b/crate-b2".to_string())); } + + #[test] + fn test_workspace_dependency_resolution_with_custom_path() { + let temp = TempDir::new().unwrap(); + let root = temp.path(); + + let ws_b_path = root.join("workspace-b"); + fs::create_dir_all(ws_b_path.join("libs/tool/src")).unwrap(); + fs::write( + ws_b_path.join("Cargo.toml"), + "[workspace]\nmembers = [\"libs/tool\"]\n", + ) + .unwrap(); + fs::write( + ws_b_path.join("libs/tool/Cargo.toml"), + "[package]\nname = \"custom-lib\"\n", + ) + .unwrap(); + fs::write(ws_b_path.join("libs/tool/src/lib.rs"), "pub fn tool() {}").unwrap(); + + let ws_a_path = root.join("workspace-a"); + fs::create_dir_all(ws_a_path.join("consumer/src")).unwrap(); + fs::write( + ws_a_path.join("Cargo.toml"), + "[workspace]\nmembers = [\"consumer\"]\n\n[workspace.dependencies]\ncustom-lib = { \ + path = \"../workspace-b/libs/tool\" }\n", + ) + .unwrap(); + fs::write( + ws_a_path.join("consumer/Cargo.toml"), + "[package]\nname = \"consumer\"\n\n[dependencies]\ncustom-lib = { workspace = true }\n", + ) + .unwrap(); + fs::write(ws_a_path.join("consumer/src/lib.rs"), "pub fn consume() {}").unwrap(); + + let mut analyzer = WorkspaceAnalyzer::new(); + analyzer + .discover_workspaces(&[root.to_path_buf()], None) + .unwrap(); + + let mut builder = DependencyGraphBuilder::new(false, false, false); + builder + .build_cross_workspace_graph( + analyzer.workspaces(), + analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), + None, + ) + .unwrap(); + + let matching_edges: Vec<_> = builder + .graph() + .edge_references() + .filter(|edge| edge.weight().to_crate() == "custom-lib") + .collect(); + + assert_eq!(matching_edges.len(), 1); + + let edge = matching_edges[0]; + let from_node = &builder.graph()[edge.source()]; + let to_node = &builder.graph()[edge.target()]; + + assert_eq!(from_node.name(), "workspace-a"); + assert_eq!(to_node.name(), "workspace-b"); + } } diff --git a/src/graph/renderer.rs b/src/graph/renderer.rs index 2eb039f..24a4b4f 100644 --- a/src/graph/renderer.rs +++ b/src/graph/renderer.rs @@ -59,13 +59,11 @@ impl GraphRenderer { output: &mut dyn Write, ) -> Result<()> { if graph.node_count() == 0 { - writeln_out!(output, "No workspaces found to visualize") - .map_err(FerrisWheelError::from)?; + writeln_out!(output, "No workspaces found to visualize")?; return Ok(()); } - writeln_out!(output, "\nšŸ“Š Workspace Dependency Graph\n") - .map_err(FerrisWheelError::from)?; + writeln_out!(output, "\nšŸ“Š Workspace Dependency Graph\n")?; // Build sets of workspace names involved in cycles for easy lookup let cycles_ws_names: Vec> = cycles @@ -88,20 +86,16 @@ impl GraphRenderer { // Print workspace header with cycle indicator if in_cycle && self.highlight_cycles { - writeln_out!(output, "ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”") - .map_err(FerrisWheelError::from)?; - writeln_out!(output, "ā”‚ {} āš ļø IN CYCLE", ws_name) - .map_err(FerrisWheelError::from)?; - writeln_out!(output, "ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜") - .map_err(FerrisWheelError::from)?; + writeln_out!(output, "ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”")?; + writeln_out!(output, "ā”‚ {} āš ļø IN CYCLE", ws_name)?; + writeln_out!(output, "ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜")?; } else { - writeln_out!(output, "{}", ws_name).map_err(FerrisWheelError::from)?; + writeln_out!(output, "{}", ws_name)?; } // Show crates in this workspace if requested if self.show_crates && !node.crates().is_empty() { - writeln_out!(output, " šŸ“¦ Crates: {}", node.crates().join(", ")) - .map_err(FerrisWheelError::from)?; + writeln_out!(output, " šŸ“¦ Crates: {}", node.crates().join(", "))?; } // Aggregate edges by target and dependency type @@ -115,8 +109,7 @@ impl GraphRenderer { } if edge_groups.is_empty() { - writeln_out!(output, " ā””ā”€ā”€ (no cross-workspace dependencies)") - .map_err(FerrisWheelError::from)?; + writeln_out!(output, " ā””ā”€ā”€ (no cross-workspace dependencies)")?; } else { // Sort groups by target workspace name and dependency type let mut groups: Vec<_> = edge_groups.into_iter().collect(); diff --git a/src/lib.rs b/src/lib.rs index 8150e02..cc711f8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,6 +43,8 @@ //! graph_builder.build_cross_workspace_graph( //! analyzer.workspaces(), //! analyzer.crate_to_workspace(), +//! analyzer.crate_path_to_workspace(), +//! analyzer.crate_to_paths(), //! None, // no progress reporter //! )?; //! @@ -93,6 +95,8 @@ //! # graph_builder.build_cross_workspace_graph( //! # analyzer.workspaces(), //! # analyzer.crate_to_workspace(), +//! # analyzer.crate_path_to_workspace(), +//! # analyzer.crate_to_paths(), //! # None, //! # )?; //! # let mut detector = CycleDetector::new(); @@ -144,6 +148,8 @@ //! graph_builder.build_cross_workspace_graph( //! analyzer.workspaces(), //! analyzer.crate_to_workspace(), +//! analyzer.crate_path_to_workspace(), +//! analyzer.crate_to_paths(), //! None, //! )?; //! @@ -171,6 +177,8 @@ //! # graph_builder.build_cross_workspace_graph( //! # analyzer.workspaces(), //! # analyzer.crate_to_workspace(), +//! # analyzer.crate_path_to_workspace(), +//! # analyzer.crate_to_paths(), //! # None, //! # )?; //! # let mut detector = CycleDetector::new(); diff --git a/tests/integration_test.rs b/tests/integration_test.rs index e3efa5f..eebad7f 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -177,7 +177,13 @@ fn test_readme_example_generation() { // Build dependency graph let mut graph_builder = DependencyGraphBuilder::new(false, false, false); graph_builder - .build_cross_workspace_graph(analyzer.workspaces(), analyzer.crate_to_workspace(), None) + .build_cross_workspace_graph( + analyzer.workspaces(), + analyzer.crate_to_workspace(), + analyzer.crate_path_to_workspace(), + analyzer.crate_to_paths(), + None, + ) .unwrap(); // Detect cycles