diff --git a/CHANGELOG.md b/CHANGELOG.md index 975b845de8..a78138a3d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## Unreleased + +#### 🐞 Fixes + +- Fixed an issue where dependencies/dependents of an affected task would be skipped in the action + graph if they were also not affected. + +#### ⚙️ Internal + +- Improved task dependent resolution in the action graph. + ## 1.30.1 #### 🐞 Fixes diff --git a/crates/action-graph/src/action_graph_builder.rs b/crates/action-graph/src/action_graph_builder.rs index 661c42a14b..e26d1e9c87 100644 --- a/crates/action-graph/src/action_graph_builder.rs +++ b/crates/action-graph/src/action_graph_builder.rs @@ -257,6 +257,17 @@ impl<'app> ActionGraphBuilder<'app> { reqs: &RunRequirements, config: Option<&TaskDependencyConfig>, ) -> miette::Result> { + // Create a new requirements object as we don't have our dependencies/ + // dependents to check for affected or run their own dependents! + let child_reqs = RunRequirements { + ci: reqs.ci, + ci_check: reqs.ci_check, + dependents: false, + interactive: reqs.interactive, + skip_affected: true, + ..Default::default() + }; + // Only apply checks when requested. This applies to `moon ci`, // but not `moon run`, since the latter should be able to // manually run local tasks in CI (deploys, etc). @@ -280,7 +291,7 @@ impl<'app> ActionGraphBuilder<'app> { ); if affected.is_task_marked(task) { - self.run_task_dependents(task, reqs)?; + self.run_task_dependents(task, &child_reqs)?; } } } @@ -323,7 +334,7 @@ impl<'app> ActionGraphBuilder<'app> { // Compare against touched files if provided if let Some(affected) = &mut self.affected { - if !affected.is_task_marked(task) { + if !reqs.skip_affected && !affected.is_task_marked(task) { return Ok(None); } } @@ -348,14 +359,14 @@ impl<'app> ActionGraphBuilder<'app> { "Linking dependencies for task", ); - edges.extend(self.run_task_dependencies(task, reqs)?); + edges.extend(self.run_task_dependencies(task, &child_reqs)?); } self.link_requirements(index, edges); // And possibly dependents if reqs.dependents { - self.run_task_dependents(task, reqs)?; + self.run_task_dependents(task, &child_reqs)?; } Ok(Some(index)) @@ -367,14 +378,9 @@ impl<'app> ActionGraphBuilder<'app> { pub fn run_task_dependencies( &mut self, task: &Task, - child_reqs: &RunRequirements, + reqs: &RunRequirements, ) -> miette::Result> { let parallel = task.options.run_deps_in_parallel; - let reqs = RunRequirements { - ci: child_reqs.ci, - ..Default::default() - }; - let mut indices = vec![]; let mut previous_target_index = None; @@ -405,60 +411,19 @@ impl<'app> ActionGraphBuilder<'app> { Ok(indices) } - // This is costly, is there a better way to do this? #[instrument(skip_all)] pub fn run_task_dependents( &mut self, task: &Task, - parent_reqs: &RunRequirements, + reqs: &RunRequirements, ) -> miette::Result> { let mut indices = vec![]; - // Create a new requirements object as we only want direct - // dependents, and shouldn't recursively create. - let reqs = RunRequirements { - ci: parent_reqs.ci, - interactive: parent_reqs.interactive, - skip_affected: true, - ..Default::default() - }; - - if let TargetScope::Project(project_locator) = &task.target.scope { - let mut projects_to_build = vec![]; - - // From self project - let self_project = self.workspace_graph.get_project(project_locator)?; - - projects_to_build.push(self_project.clone()); + for dep_target in self.workspace_graph.tasks.dependents_of(task) { + let (_, dep_indices) = self.run_task_by_target(dep_target, reqs)?; - // From other projects - for dependent_id in self.workspace_graph.projects.dependents_of(&self_project) { - projects_to_build.push(self.workspace_graph.get_project(&dependent_id)?); - } - - for project in projects_to_build { - // Don't skip internal tasks, since they are a dependency of the parent - // task, and must still run! They just can't be ran manually. - for dep_task in self.workspace_graph.get_tasks_from_project(&project.id)? { - // But do skip persistent tasks! - if dep_task.is_persistent() { - continue; - } - - // Since these are transient tasks, we should always filter out tasks - // that should not run in CI, as we don't know what side-effects it - // will cause. This applies to both `moon ci` and `moon run`. - if parent_reqs.ci && !dep_task.should_run_in_ci() { - self.passthrough_targets.insert(dep_task.target.clone()); - continue; - } - - if dep_task.deps.iter().any(|dep| dep.target == task.target) { - if let Some(index) = self.run_task(&project, &dep_task, &reqs)? { - indices.push(index); - } - } - } + for dep_index in dep_indices { + indices.push(dep_index); } } @@ -713,3 +678,12 @@ impl<'app> ActionGraphBuilder<'app> { index } } + +#[cfg(debug_assertions)] +impl<'app> ActionGraphBuilder<'app> { + pub fn mock_affected(&mut self, mut op: impl FnMut(&mut AffectedTracker<'app>)) { + if let Some(affected) = self.affected.as_mut() { + op(affected); + } + } +} diff --git a/crates/action-graph/tests/__fixtures__/tasks/deps-affected/moon.yml b/crates/action-graph/tests/__fixtures__/tasks/deps-affected/moon.yml new file mode 100644 index 0000000000..2a500feb22 --- /dev/null +++ b/crates/action-graph/tests/__fixtures__/tasks/deps-affected/moon.yml @@ -0,0 +1,12 @@ +tasks: + a: + command: 'a' + inputs: ['a.txt'] + deps: ['b'] + b: + command: 'b' + inputs: ['b.txt'] + deps: ['c'] + c: + command: 'c' + inputs: ['c.txt'] diff --git a/crates/action-graph/tests/action_graph_test.rs b/crates/action-graph/tests/action_graph_test.rs index ff21740959..453caeecea 100644 --- a/crates/action-graph/tests/action_graph_test.rs +++ b/crates/action-graph/tests/action_graph_test.rs @@ -333,68 +333,6 @@ mod action_graph { ); } - #[tokio::test] - async fn doesnt_graph_if_not_affected_by_touched_files() { - let sandbox = create_sandbox("projects"); - let container = ActionGraphContainer::new(sandbox.path()).await; - let mut builder = container.create_builder(); - - let project = container.workspace_graph.get_project("bar").unwrap(); - - let mut task = create_task("build", "bar"); - task.platform = PlatformType::Node; - - // Empty set works fine, just needs to be some - let touched_files = FxHashSet::default(); - builder.set_touched_files(&touched_files).unwrap(); - - builder - .run_task(&project, &task, &RunRequirements::default()) - .unwrap(); - - let graph = builder.build(); - - assert!(!topo(graph).into_iter().any(|node| { - if let ActionNode::RunTask(inner) = &node { - inner.target.as_str() == "bar:build" - } else { - false - } - })); - } - - #[tokio::test] - async fn graphs_if_affected_by_touched_files() { - let sandbox = create_sandbox("projects"); - let container = ActionGraphContainer::new(sandbox.path()).await; - let mut builder = container.create_builder(); - - let file = WorkspaceRelativePathBuf::from("bar/file.js"); - - let project = container.workspace_graph.get_project("bar").unwrap(); - - let mut task = create_task("build", "bar"); - task.platform = PlatformType::Node; - task.input_files.insert(file.clone()); - - let touched_files = FxHashSet::from_iter([file]); - builder.set_touched_files(&touched_files).unwrap(); - builder - .affected - .as_mut() - .unwrap() - .mark_task_affected(&task, moon_affected::AffectedBy::AlwaysAffected) - .unwrap(); - - builder - .run_task(&project, &task, &RunRequirements::default()) - .unwrap(); - - let graph = builder.build(); - - assert!(!topo(graph).is_empty()); - } - #[tokio::test] async fn task_can_have_a_diff_platform_from_project() { let sandbox = create_sandbox("projects"); @@ -947,6 +885,137 @@ mod action_graph { ); } + mod affected { + use super::*; + use moon_affected::AffectedBy; + use moon_test_utils2::pretty_assertions::assert_eq; + + #[tokio::test] + async fn doesnt_graph_if_not_affected_by_touched_files() { + let sandbox = create_sandbox("projects"); + let container = ActionGraphContainer::new(sandbox.path()).await; + let mut builder = container.create_builder(); + + let project = container.workspace_graph.get_project("bar").unwrap(); + + let mut task = create_task("build", "bar"); + task.platform = PlatformType::Node; + + // Empty set works fine, just needs to be some + let touched_files = FxHashSet::default(); + builder.set_touched_files(&touched_files).unwrap(); + + builder + .run_task(&project, &task, &RunRequirements::default()) + .unwrap(); + + let graph = builder.build(); + + assert!(!topo(graph).into_iter().any(|node| { + if let ActionNode::RunTask(inner) = &node { + inner.target.as_str() == "bar:build" + } else { + false + } + })); + } + + #[tokio::test] + async fn graphs_if_affected_by_touched_files() { + let sandbox = create_sandbox("projects"); + let container = ActionGraphContainer::new(sandbox.path()).await; + let mut builder = container.create_builder(); + + let file = WorkspaceRelativePathBuf::from("bar/file.js"); + + let project = container.workspace_graph.get_project("bar").unwrap(); + + let mut task = create_task("build", "bar"); + task.platform = PlatformType::Node; + task.input_files.insert(file.clone()); + + let touched_files = FxHashSet::from_iter([file]); + builder.set_touched_files(&touched_files).unwrap(); + builder.mock_affected(|affected| { + affected + .mark_task_affected(&task, AffectedBy::AlwaysAffected) + .unwrap(); + }); + + builder + .run_task(&project, &task, &RunRequirements::default()) + .unwrap(); + + let graph = builder.build(); + + assert!(!topo(graph).is_empty()); + } + + #[tokio::test] + async fn includes_deps_if_owning_task_is_affected() { + let sandbox = create_sandbox("tasks"); + let container = ActionGraphContainer::new(sandbox.path()).await; + let mut builder = container.create_builder(); + + let project = container + .workspace_graph + .get_project("deps-affected") + .unwrap(); + let task = container + .workspace_graph + .get_task_from_project("deps-affected", "b") + .unwrap(); + + let touched_files = + FxHashSet::from_iter([WorkspaceRelativePathBuf::from("deps-affected/b.txt")]); + builder.set_touched_files(&touched_files).unwrap(); + builder.mock_affected(|affected| { + affected + .mark_task_affected(&task, AffectedBy::AlwaysAffected) + .unwrap(); + }); + + builder + .run_task( + &project, + &task, + &RunRequirements { + dependents: true, + ..Default::default() + }, + ) + .unwrap(); + + let graph = builder.build(); + + assert_eq!( + topo(graph), + vec![ + ActionNode::sync_workspace(), + ActionNode::setup_toolchain(SetupToolchainNode { + runtime: Runtime::system() + }), + ActionNode::sync_project(SyncProjectNode { + project: Id::raw("deps-affected"), + runtime: Runtime::system() + }), + ActionNode::run_task(RunTaskNode::new( + Target::parse("deps-affected:c").unwrap(), + Runtime::system() + )), + ActionNode::run_task(RunTaskNode::new( + Target::parse("deps-affected:b").unwrap(), + Runtime::system() + )), + ActionNode::run_task(RunTaskNode::new( + Target::parse("deps-affected:a").unwrap(), + Runtime::system() + )), + ] + ); + } + } + mod run_in_ci { use super::*; diff --git a/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents.snap b/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents.snap index 7867749b50..5a9c051fe7 100644 --- a/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents.snap +++ b/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents.snap @@ -7,19 +7,19 @@ digraph { 1 [ label="SetupToolchain(system)" ] 2 [ label="SyncProject(system, deps)" ] 3 [ label="RunTask(deps:base)" ] - 4 [ label="RunTask(deps:parent1)" ] - 5 [ label="RunTask(deps:parent2)" ] - 6 [ label="SyncProject(system, deps-external)" ] - 7 [ label="RunTask(deps-external:external)" ] + 4 [ label="SyncProject(system, deps-external)" ] + 5 [ label="RunTask(deps-external:external)" ] + 6 [ label="RunTask(deps:parent2)" ] + 7 [ label="RunTask(deps:parent1)" ] 1 -> 0 [ ] 2 -> 1 [ ] 3 -> 2 [ ] + 4 -> 1 [ ] 4 -> 2 [ ] - 4 -> 3 [ ] - 5 -> 2 [ ] + 5 -> 4 [ ] 5 -> 3 [ ] - 6 -> 1 [ ] 6 -> 2 [ ] - 7 -> 6 [ ] + 6 -> 3 [ ] + 7 -> 2 [ ] 7 -> 3 [ ] } diff --git a/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents_for_ci.snap b/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents_for_ci.snap index 670bb4a279..5a9c051fe7 100644 --- a/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents_for_ci.snap +++ b/crates/action-graph/tests/snapshots/action_graph_test__action_graph__run_task_dependencies__includes_dependents_for_ci.snap @@ -7,16 +7,19 @@ digraph { 1 [ label="SetupToolchain(system)" ] 2 [ label="SyncProject(system, deps)" ] 3 [ label="RunTask(deps:base)" ] - 4 [ label="RunTask(deps:parent1)" ] - 5 [ label="SyncProject(system, deps-external)" ] - 6 [ label="RunTask(deps-external:external)" ] + 4 [ label="SyncProject(system, deps-external)" ] + 5 [ label="RunTask(deps-external:external)" ] + 6 [ label="RunTask(deps:parent2)" ] + 7 [ label="RunTask(deps:parent1)" ] 1 -> 0 [ ] 2 -> 1 [ ] 3 -> 2 [ ] + 4 -> 1 [ ] 4 -> 2 [ ] - 4 -> 3 [ ] - 5 -> 1 [ ] - 5 -> 2 [ ] - 6 -> 5 [ ] + 5 -> 4 [ ] + 5 -> 3 [ ] + 6 -> 2 [ ] 6 -> 3 [ ] + 7 -> 2 [ ] + 7 -> 3 [ ] }