fix: Fix action graph regressions. (#1623)

* Rework APIs.

* Fix tests.

* Add more tests.

* Fix touched dependents.

* Fix test.

.yarn/versions/93e1ba19.yml

@@ -0,0 +1,9 @@
+releases:
+  "@moonrepo/cli": patch
+  "@moonrepo/core-linux-arm64-gnu": patch
+  "@moonrepo/core-linux-arm64-musl": patch
+  "@moonrepo/core-linux-x64-gnu": patch
+  "@moonrepo/core-linux-x64-musl": patch
+  "@moonrepo/core-macos-arm64": patch
+  "@moonrepo/core-macos-x64": patch
+  "@moonrepo/core-windows-x64-msvc": patch

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+## Unreleased
+
+#### 🐞 Fixes
+
+- Fixed a regression where the "primary target" detection would include far too many targets. This
+  would result in passthrough arguments being unintentionally passed deeper.
+- Fixed a regression where dependent tasks would always run in CI.
+
 ## 1.27.8
 
 #### 🚀 Updates

crates/action-context/src/lib.rs

@@ -1,6 +1,6 @@
 use clap::ValueEnum;
 use moon_common::path::WorkspaceRelativePathBuf;
-use moon_target::{Target, TargetLocator};
+use moon_target::Target;
 use rustc_hash::{FxHashMap, FxHashSet};
 use serde::{Deserialize, Serialize};
 use std::sync::Arc;
@@ -42,7 +42,7 @@ pub struct ActionContext {
     pub affected_only: bool,
 
     /// Initial target locators passed to `moon run`, `moon ci`, etc.
-    pub initial_targets: FxHashSet<TargetLocator>,
+    pub initial_targets: FxHashSet<Target>,
 
     /// Active mutexes for tasks to acquire locks against.
     /// @mutable
@@ -110,15 +110,9 @@ impl ActionContext {
         }
 
         // :task == scope:task
-        for locator in &self.initial_targets {
-            // if target.is_all_task(locator.as_str()) {
-            //     return true;
-            // }
-
-            if let TargetLocator::Qualified(inner) = locator {
-                if inner.is_all_task(&target.task_id) {
-                    return true;
-                }
+        for other_target in &self.initial_targets {
+            if other_target.is_all_task(&target.task_id) {
+                return true;
             }
         }
 

crates/action-graph/src/action_graph_builder.rs

@@ -24,16 +24,14 @@ pub struct RunRequirements<'app> {
     pub ci: bool,         // Are we in a CI environment
     pub ci_check: bool,   // Check the `runInCI` option
     pub dependents: bool, // Run dependent tasks as well
-    pub initial_locators: Vec<&'app TargetLocator>,
-    pub resolved_locators: Vec<TargetLocator>,
     pub interactive: bool,
+    pub target_locators: FxHashSet<TargetLocator>,
     pub touched_files: Option<&'app TouchedFilePaths>,
 }
 
 impl<'app> RunRequirements<'app> {
     pub fn has_target(&self, target: &Target) -> bool {
-        self.resolved_locators.iter().any(|loc| loc == target)
-            || self.initial_locators.iter().any(|loc| *loc == target)
+        self.target_locators.iter().any(|loc| loc == target)
     }
 }
 
@@ -45,6 +43,7 @@ pub struct ActionGraphBuilder<'app> {
     project_graph: &'app ProjectGraph,
 
     // Target tracking
+    initial_targets: FxHashSet<Target>,
     passthrough_targets: FxHashSet<Target>,
     primary_targets: FxHashSet<Target>,
 }
@@ -64,6 +63,7 @@ impl<'app> ActionGraphBuilder<'app> {
             all_query: None,
             graph: DiGraph::new(),
             indices: FxHashMap::default(),
+            initial_targets: FxHashSet::default(),
             passthrough_targets: FxHashSet::default(),
             platform_manager,
             primary_targets: FxHashSet::default(),
@@ -78,6 +78,10 @@ impl<'app> ActionGraphBuilder<'app> {
     pub fn build_context(&mut self) -> ActionContext {
         let mut context = ActionContext::default();
 
+        if !self.initial_targets.is_empty() {
+            context.initial_targets = mem::take(&mut self.initial_targets);
+        }
+
         if !self.passthrough_targets.is_empty() {
             for target in mem::take(&mut self.passthrough_targets) {
                 context.set_target_state(target, TargetState::Passthrough);
@@ -223,7 +227,7 @@ impl<'app> ActionGraphBuilder<'app> {
 
             // Dependents may still want to run though!
             if reqs.dependents {
-                self.run_task_dependents(task, reqs)?;
+                self.run_task_dependents(task, reqs, true)?;
             }
 
             return Ok(None);
@@ -296,7 +300,7 @@ impl<'app> ActionGraphBuilder<'app> {
 
         // And possibly dependents
         if reqs.dependents {
-            self.run_task_dependents(task, reqs)?;
+            self.run_task_dependents(task, reqs, false)?;
         }
 
         Ok(Some(index))
@@ -312,7 +316,10 @@ impl<'app> ActionGraphBuilder<'app> {
         let mut previous_target_index = None;
 
         for dep in &task.deps {
-            for dep_index in self.run_task_by_target_with_config(&dep.target, &reqs, Some(dep))? {
+            let (_, dep_indices) =
+                self.run_task_by_target_with_config(&dep.target, &reqs, Some(dep))?;
+
+            for dep_index in dep_indices {
                 // When parallel, parent depends on child
                 if parallel {
                     indices.push(dep_index);
@@ -341,12 +348,22 @@ impl<'app> ActionGraphBuilder<'app> {
         &mut self,
         task: &Task,
         parent_reqs: &RunRequirements<'app>,
+        with_touched: bool,
     ) -> miette::Result<Vec<NodeIndex>> {
         let mut indices = vec![];
 
         // Create a new requirements object as we only want direct
         // dependents, and shouldn't recursively create.
-        let reqs = RunRequirements::default();
+        let reqs = RunRequirements {
+            ci: parent_reqs.ci,
+            interactive: parent_reqs.interactive,
+            touched_files: if with_touched {
+                parent_reqs.touched_files
+            } else {
+                None
+            },
+            ..Default::default()
+        };
 
         if let TargetScope::Project(project_locator) = &task.target.scope {
             let mut projects_to_build = vec![];
@@ -394,35 +411,16 @@ impl<'app> ActionGraphBuilder<'app> {
         &mut self,
         target: T,
         reqs: &RunRequirements<'app>,
-    ) -> miette::Result<FxHashSet<NodeIndex>> {
+    ) -> miette::Result<(FxHashSet<Target>, FxHashSet<NodeIndex>)> {
         self.run_task_by_target_with_config(target, reqs, None)
     }
 
-    pub fn run_task_by_target_locator<T: AsRef<TargetLocator>>(
-        &mut self,
-        target_locator: T,
-        reqs: &mut RunRequirements<'app>,
-    ) -> miette::Result<FxHashSet<NodeIndex>> {
-        match target_locator.as_ref() {
-            TargetLocator::Qualified(target) => self.run_task_by_target(target, reqs),
-            TargetLocator::TaskFromWorkingDir(task_id) => {
-                let project = self.project_graph.get_from_path(None)?;
-                let target = Target::new(&project.id, task_id)?;
-
-                reqs.resolved_locators
-                    .push(TargetLocator::Qualified(target.clone()));
-
-                self.run_task_by_target(target, reqs)
-            }
-        }
-    }
-
     pub fn run_task_by_target_with_config<T: AsRef<Target>>(
         &mut self,
         target: T,
         reqs: &RunRequirements<'app>,
         config: Option<&TaskDependencyConfig>,
-    ) -> miette::Result<FxHashSet<NodeIndex>> {
+    ) -> miette::Result<(FxHashSet<Target>, FxHashSet<NodeIndex>)> {
         let target = target.as_ref();
         let mut inserted_targets = FxHashSet::default();
         let mut inserted_indices = FxHashSet::default();
@@ -505,9 +503,37 @@ impl<'app> ActionGraphBuilder<'app> {
             }
         };
 
-        self.primary_targets.extend(inserted_targets);
+        Ok((inserted_targets, inserted_indices))
+    }
+
+    #[instrument(skip_all)]
+    pub fn run_from_requirements(
+        &mut self,
+        reqs: RunRequirements<'app>,
+    ) -> miette::Result<Vec<NodeIndex>> {
+        let mut inserted_nodes = vec![];
+
+        for locator in reqs.target_locators.clone() {
+            let target = match locator {
+                TargetLocator::Qualified(target) => target,
+                TargetLocator::TaskFromWorkingDir(task_id) => {
+                    Target::new(&self.project_graph.get_from_path(None)?.id, task_id)?
+                }
+            };
+
+            // Track the qualified as an initial target
+            self.initial_targets.insert(target.clone());
+
+            // Run the target
+            let (inserted_targets, inserted_indicies) = self.run_task_by_target(target, &reqs)?;
+
+            // Track the primary targets
+            self.primary_targets.extend(inserted_targets);
+
+            inserted_nodes.extend(inserted_indicies);
+        }
 
-        Ok(inserted_indices)
+        Ok(inserted_nodes)
     }
 
     #[instrument(skip_all)]

crates/action-graph/tests/action_graph_test.rs

@@ -1479,7 +1479,7 @@ mod action_graph {
                 .run_task_by_target(
                     Target::parse("common:internal").unwrap(),
                     &RunRequirements {
-                        initial_locators: vec![&locator],
+                        target_locators: FxHashSet::from_iter([locator]),
                         ..RunRequirements::default()
                     },
                 )
@@ -1516,7 +1516,7 @@ mod action_graph {
                 .run_task_by_target(
                     Target::parse("misc:requiresInternal").unwrap(),
                     &RunRequirements {
-                        initial_locators: vec![&locator],
+                        target_locators: FxHashSet::from_iter([locator]),
                         ..RunRequirements::default()
                     },
                 )
@@ -1582,7 +1582,7 @@ mod action_graph {
         }
     }
 
-    mod run_task_by_target_locator {
+    mod run_from_requirements {
         use super::*;
 
         #[tokio::test]
@@ -1592,10 +1592,12 @@ mod action_graph {
             let mut builder = container.create_builder();
 
             builder
-                .run_task_by_target_locator(
-                    TargetLocator::Qualified(Target::parse("server:build").unwrap()),
-                    &mut RunRequirements::default(),
-                )
+                .run_from_requirements(RunRequirements {
+                    target_locators: FxHashSet::from_iter([TargetLocator::Qualified(
+                        Target::parse("server:build").unwrap(),
+                    )]),
+                    ..Default::default()
+                })
                 .unwrap();
 
             let graph = builder.build();
@@ -1613,10 +1615,12 @@ mod action_graph {
             let mut builder = container.create_builder();
 
             builder
-                .run_task_by_target_locator(
-                    TargetLocator::TaskFromWorkingDir(Id::raw("lint")),
-                    &mut RunRequirements::default(),
-                )
+                .run_from_requirements(RunRequirements {
+                    target_locators: FxHashSet::from_iter([TargetLocator::TaskFromWorkingDir(
+                        Id::raw("lint"),
+                    )]),
+                    ..Default::default()
+                })
                 .unwrap();
 
             let graph = builder.build();
@@ -1635,11 +1639,73 @@ mod action_graph {
             let mut builder = container.create_builder();
 
             builder
-                .run_task_by_target_locator(
-                    TargetLocator::TaskFromWorkingDir(Id::raw("lint")),
-                    &mut RunRequirements::default(),
-                )
+                .run_from_requirements(RunRequirements {
+                    target_locators: FxHashSet::from_iter([TargetLocator::TaskFromWorkingDir(
+                        Id::raw("lint"),
+                    )]),
+                    ..Default::default()
+                })
+                .unwrap();
+        }
+
+        #[tokio::test]
+        async fn computes_context() {
+            let sandbox = create_sandbox("tasks");
+            let container = ActionGraphContainer::new(sandbox.path()).await;
+            let mut builder = container.create_builder();
+
+            let target = Target::parse("client:build").unwrap();
+
+            builder
+                .run_from_requirements(RunRequirements {
+                    target_locators: FxHashSet::from_iter([TargetLocator::Qualified(
+                        target.clone(),
+                    )]),
+                    ..Default::default()
+                })
+                .unwrap();
+
+            let context = builder.build_context();
+
+            assert_eq!(
+                context.initial_targets,
+                FxHashSet::from_iter([target.clone()])
+            );
+            assert_eq!(context.primary_targets, FxHashSet::from_iter([target]));
+        }
+
+        #[tokio::test]
+        async fn computes_context_for_all() {
+            let sandbox = create_sandbox("tasks");
+            let container = ActionGraphContainer::new(sandbox.path()).await;
+            let mut builder = container.create_builder();
+
+            let target = Target::parse(":build").unwrap();
+
+            builder
+                .run_from_requirements(RunRequirements {
+                    target_locators: FxHashSet::from_iter([TargetLocator::Qualified(
+                        target.clone(),
+                    )]),
+                    ..Default::default()
+                })
                 .unwrap();
+
+            let context = builder.build_context();
+
+            assert_eq!(
+                context.initial_targets,
+                FxHashSet::from_iter([target.clone()])
+            );
+            assert_eq!(
+                context.primary_targets,
+                FxHashSet::from_iter([
+                    Target::parse("client:build").unwrap(),
+                    Target::parse("common:build").unwrap(),
+                    Target::parse("server:build").unwrap(),
+                    Target::parse("base:build").unwrap(),
+                ])
+            );
         }
     }