From ec9573dc9ad2133dd3b53f2eab5b29877afcb32d Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Wed, 27 Nov 2024 16:22:12 -0800 Subject: [PATCH] fix: Fix affected detection aborting early. --- .../action-graph/src/action_graph_builder.rs | 44 +++++++++-------- .../action-graph/tests/action_graph_test.rs | 9 +++- crates/affected/src/affected.rs | 6 +-- crates/affected/src/affected_tracker.rs | 48 +++++++++++++------ crates/app/src/commands/run.rs | 9 +--- 5 files changed, 68 insertions(+), 48 deletions(-) diff --git a/crates/action-graph/src/action_graph_builder.rs b/crates/action-graph/src/action_graph_builder.rs index fc09ce3a00..bd8136a028 100644 --- a/crates/action-graph/src/action_graph_builder.rs +++ b/crates/action-graph/src/action_graph_builder.rs @@ -327,7 +327,28 @@ impl<'app> ActionGraphBuilder<'app> { return Ok(Some(*index)); } - // Compare against touched files if provided + // We should install deps & sync projects *before* running targets + let mut edges = vec![]; + + if let Some(install_deps_index) = self.install_deps(project, Some(task))? { + edges.push(install_deps_index); + } + + edges.push(self.sync_project(project)?); + + // And we also need to create edges for task dependencies + if !task.deps.is_empty() { + trace!( + task_target = task.target.as_str(), + dep_targets = ?task.deps.iter().map(|d| d.target.as_str()).collect::>(), + "Linking dependencies for task", + ); + + edges.extend(self.run_task_dependencies(task)?); + } + + // Compare against touched files if provided. We need to run this *after* + // dependencies, so that they're affected status has been marked! if let Some(affected) = &mut self.affected { if let Some(downstream) = &reqs.downstream_target { affected @@ -352,28 +373,9 @@ impl<'app> ActionGraphBuilder<'app> { } } - // We should install deps & sync projects *before* running targets - let mut edges = vec![]; - - if let Some(install_deps_index) = self.install_deps(project, Some(task))? { - edges.push(install_deps_index); - } - - edges.push(self.sync_project(project)?); - + // Insert the node and create edges let index = self.insert_node(node); - // And we also need to create edges for task dependencies - if !task.deps.is_empty() { - trace!( - task_target = task.target.as_str(), - dep_targets = ?task.deps.iter().map(|d| d.target.as_str()).collect::>(), - "Linking dependencies for task", - ); - - edges.extend(self.run_task_dependencies(task)?); - } - self.link_requirements(index, edges); // And possibly dependents diff --git a/crates/action-graph/tests/action_graph_test.rs b/crates/action-graph/tests/action_graph_test.rs index 33fb048d0e..573befe737 100644 --- a/crates/action-graph/tests/action_graph_test.rs +++ b/crates/action-graph/tests/action_graph_test.rs @@ -354,7 +354,14 @@ mod action_graph { let graph = builder.build(); - assert!(topo(graph).is_empty()); + assert!(topo(graph) + .into_iter() + .find(|node| if let ActionNode::RunTask(inner) = &node { + inner.target.as_str() == "bar:build" + } else { + false + }) + .is_none()); } #[tokio::test] diff --git a/crates/affected/src/affected.rs b/crates/affected/src/affected.rs index 039334397d..db1f38df5f 100644 --- a/crates/affected/src/affected.rs +++ b/crates/affected/src/affected.rs @@ -6,7 +6,7 @@ use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use std::fmt; -#[derive(Debug, PartialEq)] +#[derive(Debug, Eq, Hash, PartialEq)] pub enum AffectedBy { AlreadyMarked, AlwaysAffected, @@ -84,7 +84,7 @@ pub struct AffectedProjectState { } impl AffectedProjectState { - pub fn from(list: Vec) -> Self { + pub fn from(list: FxHashSet) -> Self { let mut state = Self::default(); for by in list { @@ -130,7 +130,7 @@ pub struct AffectedTaskState { } impl AffectedTaskState { - pub fn from(list: Vec) -> Self { + pub fn from(list: FxHashSet) -> Self { let mut state = Self::default(); for by in list { diff --git a/crates/affected/src/affected_tracker.rs b/crates/affected/src/affected_tracker.rs index b74e06abf4..49b7740f06 100644 --- a/crates/affected/src/affected_tracker.rs +++ b/crates/affected/src/affected_tracker.rs @@ -6,17 +6,18 @@ use moon_task::{Target, Task}; use moon_workspace_graph::{GraphConnections, WorkspaceGraph}; use rustc_hash::{FxHashMap, FxHashSet}; use std::env; +use std::fmt; use tracing::{debug, trace}; pub struct AffectedTracker<'app> { workspace_graph: &'app WorkspaceGraph, touched_files: &'app FxHashSet, - projects: FxHashMap>, + projects: FxHashMap>, project_downstream: DownstreamScope, project_upstream: UpstreamScope, - tasks: FxHashMap>, + tasks: FxHashMap>, task_downstream: DownstreamScope, task_upstream: UpstreamScope, } @@ -167,7 +168,7 @@ impl<'app> AffectedTracker<'app> { self.projects .entry(project.id.clone()) .or_default() - .push(affected); + .insert(affected); self.track_project_dependencies(project, 0)?; self.track_project_dependents(project, 0)?; @@ -203,7 +204,7 @@ impl<'app> AffectedTracker<'app> { self.projects .entry(dep_config.id.clone()) .or_default() - .push(AffectedBy::DownstreamProject(project.id.clone())); + .insert(AffectedBy::DownstreamProject(project.id.clone())); if depth == 0 && self.project_upstream == UpstreamScope::Direct { continue; @@ -221,7 +222,7 @@ impl<'app> AffectedTracker<'app> { if self.project_downstream == DownstreamScope::None { trace!( project_id = project.id.as_str(), - "Not tracking dependents as downstream scope is none" + "Not tracking project dependents as downstream scope is none" ); return Ok(()); @@ -231,10 +232,13 @@ impl<'app> AffectedTracker<'app> { if self.project_downstream == DownstreamScope::Direct { trace!( project_id = project.id.as_str(), - "Tracking direct dependents" + "Tracking direct project dependents" ); } else { - trace!(project_id = project.id.as_str(), "Tracking deep dependents"); + trace!( + project_id = project.id.as_str(), + "Tracking deep project dependents" + ); } } @@ -242,7 +246,7 @@ impl<'app> AffectedTracker<'app> { self.projects .entry(dep_id.clone()) .or_default() - .push(AffectedBy::UpstreamProject(project.id.clone())); + .insert(AffectedBy::UpstreamProject(project.id.clone())); if depth == 0 && self.project_downstream == DownstreamScope::Direct { continue; @@ -331,7 +335,7 @@ impl<'app> AffectedTracker<'app> { self.tasks .entry(task.target.clone()) .or_default() - .push(affected); + .insert(affected); self.track_task_dependencies(task, 0)?; self.track_task_dependents(task, 0)?; @@ -340,7 +344,7 @@ impl<'app> AffectedTracker<'app> { self.projects .entry(project_id.to_owned()) .or_default() - .push(AffectedBy::Task(task.target.clone())); + .insert(AffectedBy::Task(task.target.clone())); } Ok(()) @@ -374,7 +378,7 @@ impl<'app> AffectedTracker<'app> { self.tasks .entry(dep_config.target.clone()) .or_default() - .push(AffectedBy::DownstreamTask(task.target.clone())); + .insert(AffectedBy::DownstreamTask(task.target.clone())); if depth == 0 && self.task_upstream == UpstreamScope::Direct { continue; @@ -392,7 +396,7 @@ impl<'app> AffectedTracker<'app> { if self.task_downstream == DownstreamScope::None { trace!( task_target = task.target.as_str(), - "Not tracking dependents as downstream scope is none" + "Not tracking task dependents as downstream scope is none" ); return Ok(()); @@ -402,12 +406,12 @@ impl<'app> AffectedTracker<'app> { if self.task_downstream == DownstreamScope::Direct { trace!( task_target = task.target.as_str(), - "Tracking direct dependents" + "Tracking direct task dependents" ); } else { trace!( task_target = task.target.as_str(), - "Tracking deep dependents" + "Tracking deep task dependents" ); } } @@ -416,7 +420,7 @@ impl<'app> AffectedTracker<'app> { self.tasks .entry(dep_target.clone()) .or_default() - .push(AffectedBy::UpstreamTask(task.target.clone())); + .insert(AffectedBy::UpstreamTask(task.target.clone())); if depth == 0 && self.task_downstream == DownstreamScope::Direct { continue; @@ -430,3 +434,17 @@ impl<'app> AffectedTracker<'app> { Ok(()) } } + +impl<'app> fmt::Debug for AffectedTracker<'app> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("AffectedTracker") + .field("touched_files", &self.touched_files) + .field("projects", &self.projects) + .field("project_downstream", &self.project_downstream) + .field("project_upstream", &self.project_upstream) + .field("tasks", &self.tasks) + .field("task_downstream", &self.task_downstream) + .field("task_upstream", &self.task_upstream) + .finish() + } +} diff --git a/crates/app/src/commands/run.rs b/crates/app/src/commands/run.rs index 8296f6393a..64262b53c9 100644 --- a/crates/app/src/commands/run.rs +++ b/crates/app/src/commands/run.rs @@ -152,14 +152,7 @@ pub async fn run_target( if should_run_affected { action_graph_builder.set_touched_files(&touched_files)?; - action_graph_builder.set_affected_scopes( - UpstreamScope::Deep, - if args.dependents { - DownstreamScope::Direct - } else { - DownstreamScope::None - }, - )?; + action_graph_builder.set_affected_scopes(UpstreamScope::Deep, DownstreamScope::Deep)?; } // Run targets, optionally based on affected files