From a715757d9b26f9ac04972858ec80ffa1f2d4e467 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 20 Nov 2024 22:56:04 -0500 Subject: [PATCH 1/2] chore: failing tests root member --- .../dependencies_root_not_cycle/__test__.jsonc | 5 +++++ .../task/dependencies_root_not_cycle/deno.json | 10 ++++++++++ .../dependencies_root_not_cycle/member/deno.json | 8 ++++++++ .../task/dependencies_root_not_cycle/task.out | 6 ++++++ .../dependencies_shadowed_root_name/__test__.jsonc | 14 ++++++++++++++ .../dependencies_shadowed_root_name/deno.jsonc | 12 ++++++++++++ .../member/deno.jsonc | 12 ++++++++++++ .../member_depending_root_and_member.out | 10 ++++++++++ .../root_dependending_root.out | 4 ++++ 9 files changed, 81 insertions(+) create mode 100644 tests/specs/task/dependencies_root_not_cycle/__test__.jsonc create mode 100644 tests/specs/task/dependencies_root_not_cycle/deno.json create mode 100644 tests/specs/task/dependencies_root_not_cycle/member/deno.json create mode 100644 tests/specs/task/dependencies_root_not_cycle/task.out create mode 100644 tests/specs/task/dependencies_shadowed_root_name/__test__.jsonc create mode 100644 tests/specs/task/dependencies_shadowed_root_name/deno.jsonc create mode 100644 tests/specs/task/dependencies_shadowed_root_name/member/deno.jsonc create mode 100644 tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out create mode 100644 tests/specs/task/dependencies_shadowed_root_name/root_dependending_root.out diff --git a/tests/specs/task/dependencies_root_not_cycle/__test__.jsonc b/tests/specs/task/dependencies_root_not_cycle/__test__.jsonc new file mode 100644 index 00000000000000..073210358209a1 --- /dev/null +++ b/tests/specs/task/dependencies_root_not_cycle/__test__.jsonc @@ -0,0 +1,5 @@ +{ + "args": "task a", + "cwd": "member", + "output": "task.out" +} diff --git a/tests/specs/task/dependencies_root_not_cycle/deno.json b/tests/specs/task/dependencies_root_not_cycle/deno.json new file mode 100644 index 00000000000000..43a4952f938732 --- /dev/null +++ b/tests/specs/task/dependencies_root_not_cycle/deno.json @@ -0,0 +1,10 @@ +{ + "tasks": { + "a": "echo root-a", + "b": { + "dependencies": ["a"], + "command": "echo b" + } + }, + "workspace": ["./member"] +} diff --git a/tests/specs/task/dependencies_root_not_cycle/member/deno.json b/tests/specs/task/dependencies_root_not_cycle/member/deno.json new file mode 100644 index 00000000000000..08c3493c73a1da --- /dev/null +++ b/tests/specs/task/dependencies_root_not_cycle/member/deno.json @@ -0,0 +1,8 @@ +{ + "tasks": { + "a": { + "dependencies": ["b"], + "command": "echo a" + } + } +} diff --git a/tests/specs/task/dependencies_root_not_cycle/task.out b/tests/specs/task/dependencies_root_not_cycle/task.out new file mode 100644 index 00000000000000..65688381acff2d --- /dev/null +++ b/tests/specs/task/dependencies_root_not_cycle/task.out @@ -0,0 +1,6 @@ +Task a echo root-a +a +Task b echo b +b +Task a echo a +a diff --git a/tests/specs/task/dependencies_shadowed_root_name/__test__.jsonc b/tests/specs/task/dependencies_shadowed_root_name/__test__.jsonc new file mode 100644 index 00000000000000..9d3d50582c5f11 --- /dev/null +++ b/tests/specs/task/dependencies_shadowed_root_name/__test__.jsonc @@ -0,0 +1,14 @@ +{ + "tests": { + "root_dependending_root": { + "args": "task root-depending-root", + "cwd": "member", + "output": "root_dependending_root.out" + }, + "member_depending_root_and_member": { + "args": "task member-dependending-root-and-member", + "cwd": "member", + "output": "member_depending_root_and_member.out" + } + } +} diff --git a/tests/specs/task/dependencies_shadowed_root_name/deno.jsonc b/tests/specs/task/dependencies_shadowed_root_name/deno.jsonc new file mode 100644 index 00000000000000..88f0ee4e9eef09 --- /dev/null +++ b/tests/specs/task/dependencies_shadowed_root_name/deno.jsonc @@ -0,0 +1,12 @@ +{ + "tasks": { + "build": "echo root", + "root-depending-root": { + "dependencies": [ + "build" + ], + "command": "echo test" + } + }, + "workspace": ["./member"] +} diff --git a/tests/specs/task/dependencies_shadowed_root_name/member/deno.jsonc b/tests/specs/task/dependencies_shadowed_root_name/member/deno.jsonc new file mode 100644 index 00000000000000..df5ac047a3842d --- /dev/null +++ b/tests/specs/task/dependencies_shadowed_root_name/member/deno.jsonc @@ -0,0 +1,12 @@ +{ + "tasks": { + "build": "echo member", + "member-dependending-root-and-member": { + "dependencies": [ + "build", + "root-depending-root" + ], + "command": "echo member-test" + } + } +} diff --git a/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out b/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out new file mode 100644 index 00000000000000..e8c87f2ff2c0b6 --- /dev/null +++ b/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out @@ -0,0 +1,10 @@ +[UNORDERED_START] +Task build echo member +member +Task root-depending-root echo test +test +Task root echo root +root +[UNORDERED_END] +Task member-depending-root-and-member echo member-test +member-test diff --git a/tests/specs/task/dependencies_shadowed_root_name/root_dependending_root.out b/tests/specs/task/dependencies_shadowed_root_name/root_dependending_root.out new file mode 100644 index 00000000000000..2b8d9d5efebf22 --- /dev/null +++ b/tests/specs/task/dependencies_shadowed_root_name/root_dependending_root.out @@ -0,0 +1,4 @@ +Task build echo root +root +Task root-depending-root echo test +test From d7948d0ce95adfb37470aae8def78decc650afcc Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 21 Nov 2024 00:12:16 -0500 Subject: [PATCH 2/2] fix --- cli/tools/task.rs | 188 +++++++++++------- .../task/dependencies_root_not_cycle/task.out | 2 +- .../member_depending_root_and_member.out | 6 +- 3 files changed, 125 insertions(+), 71 deletions(-) diff --git a/cli/tools/task.rs b/cli/tools/task.rs index b0c290adc0fcf2..478853f4e64f61 100644 --- a/cli/tools/task.rs +++ b/cli/tools/task.rs @@ -12,6 +12,7 @@ use deno_config::workspace::FolderConfigs; use deno_config::workspace::TaskDefinition; use deno_config::workspace::TaskOrScript; use deno_config::workspace::WorkspaceDirectory; +use deno_config::workspace::WorkspaceMemberTasksConfig; use deno_config::workspace::WorkspaceTasksConfig; use deno_core::anyhow::anyhow; use deno_core::anyhow::bail; @@ -270,11 +271,7 @@ impl<'a> TaskRunner<'a> { pkg_tasks_config: &PackageTaskInfo, ) -> Result { match sort_tasks_topo(pkg_tasks_config) { - Ok(sorted) => { - self - .run_tasks_in_parallel(&pkg_tasks_config.tasks_config, sorted) - .await - } + Ok(sorted) => self.run_tasks_in_parallel(sorted).await, Err(err) => match err { TaskError::NotFound(name) => { if self.task_flags.is_run { @@ -308,64 +305,62 @@ impl<'a> TaskRunner<'a> { async fn run_tasks_in_parallel( &self, - tasks_config: &WorkspaceTasksConfig, - task_names: Vec, + tasks: Vec>, ) -> Result { - struct PendingTasksContext { - completed: HashSet, - running: HashSet, - task_names: Vec, + struct PendingTasksContext<'a> { + completed: HashSet, + running: HashSet, + tasks: &'a [ResolvedTask<'a>], } - impl PendingTasksContext { + impl<'a> PendingTasksContext<'a> { fn has_remaining_tasks(&self) -> bool { - self.completed.len() < self.task_names.len() + self.completed.len() < self.tasks.len() } - fn mark_complete(&mut self, task_name: String) { - self.running.remove(&task_name); - self.completed.insert(task_name); + fn mark_complete(&mut self, task: &ResolvedTask) { + self.running.remove(&task.id); + self.completed.insert(task.id); } - fn get_next_task<'a>( + fn get_next_task<'b>( &mut self, - runner: &'a TaskRunner<'a>, - tasks_config: &'a WorkspaceTasksConfig, - ) -> Option>> { - for name in &self.task_names { - if self.completed.contains(name) || self.running.contains(name) { + runner: &'b TaskRunner<'b>, + ) -> Option< + LocalBoxFuture<'b, Result<(i32, &'a ResolvedTask<'a>), AnyError>>, + > + where + 'a: 'b, + { + for task in self.tasks.iter() { + if self.completed.contains(&task.id) + || self.running.contains(&task.id) + { continue; } - let Some((folder_url, task_or_script)) = tasks_config.task(name) - else { - continue; - }; - let should_run = match task_or_script { - TaskOrScript::Task(_, def) => def - .dependencies - .iter() - .all(|dep| self.completed.contains(dep)), - TaskOrScript::Script(_, _) => true, - }; - + let should_run = task + .dependencies + .iter() + .all(|dep_id| self.completed.contains(dep_id)); if !should_run { continue; } - self.running.insert(name.clone()); - let name = name.clone(); + self.running.insert(task.id); return Some( async move { - match task_or_script { + match task.task_or_script { TaskOrScript::Task(_, def) => { - runner.run_deno_task(folder_url, &name, def).await + runner.run_deno_task(task.folder_url, task.name, def).await } TaskOrScript::Script(scripts, _) => { - runner.run_npm_script(folder_url, &name, scripts).await + runner + .run_npm_script(task.folder_url, task.name, scripts) + .await } } - .map(|exit_code| (exit_code, name)) + .map(|exit_code| (exit_code, task)) } .boxed_local(), ); @@ -375,16 +370,16 @@ impl<'a> TaskRunner<'a> { } let mut context = PendingTasksContext { - completed: HashSet::with_capacity(task_names.len()), + completed: HashSet::with_capacity(tasks.len()), running: HashSet::with_capacity(self.concurrency), - task_names, + tasks: &tasks, }; let mut queue = futures_unordered::FuturesUnordered::new(); while context.has_remaining_tasks() { while queue.len() < self.concurrency { - if let Some(task) = context.get_next_task(self, tasks_config) { + if let Some(task) = context.get_next_task(self) { queue.push(task); } else { break; @@ -393,7 +388,7 @@ impl<'a> TaskRunner<'a> { // If queue is empty at this point, then there are no more tasks in the queue. let Some(result) = queue.next().await else { - debug_assert_eq!(context.task_names.len(), 0); + debug_assert_eq!(context.tasks.len(), 0); break; }; @@ -521,46 +516,105 @@ enum TaskError { TaskDepCycle { path: Vec }, } -fn sort_tasks_topo( - pkg_task_config: &PackageTaskInfo, -) -> Result, TaskError> { +struct ResolvedTask<'a> { + id: usize, + name: &'a str, + folder_url: &'a Url, + task_or_script: TaskOrScript<'a>, + dependencies: Vec, +} + +fn sort_tasks_topo<'a>( + pkg_task_config: &'a PackageTaskInfo, +) -> Result>, TaskError> { + trait TasksConfig { + fn task( + &self, + name: &str, + ) -> Option<(&Url, TaskOrScript, &dyn TasksConfig)>; + } + + impl TasksConfig for WorkspaceTasksConfig { + fn task( + &self, + name: &str, + ) -> Option<(&Url, TaskOrScript, &dyn TasksConfig)> { + if let Some(member) = &self.member { + if let Some((dir_url, task_or_script)) = member.task(name) { + return Some((dir_url, task_or_script, self as &dyn TasksConfig)); + } + } + if let Some(root) = &self.root { + if let Some((dir_url, task_or_script)) = root.task(name) { + // switch to only using the root tasks for the dependencies + return Some((dir_url, task_or_script, root as &dyn TasksConfig)); + } + } + None + } + } + + impl TasksConfig for WorkspaceMemberTasksConfig { + fn task( + &self, + name: &str, + ) -> Option<(&Url, TaskOrScript, &dyn TasksConfig)> { + self.task(name).map(|(dir_url, task_or_script)| { + (dir_url, task_or_script, self as &dyn TasksConfig) + }) + } + } + fn sort_visit<'a>( name: &'a str, - sorted: &mut Vec, - mut path: Vec<&'a str>, - tasks_config: &'a WorkspaceTasksConfig, - ) -> Result<(), TaskError> { - // Already sorted - if sorted.iter().any(|sorted_name| sorted_name == name) { - return Ok(()); + sorted: &mut Vec>, + mut path: Vec<(&'a Url, &'a str)>, + tasks_config: &'a dyn TasksConfig, + ) -> Result { + let Some((folder_url, task_or_script, tasks_config)) = + tasks_config.task(name) + else { + return Err(TaskError::NotFound(name.to_string())); + }; + + if let Some(existing_task) = sorted + .iter() + .find(|task| task.name == name && task.folder_url == folder_url) + { + // already exists + return Ok(existing_task.id); } - // Graph has a cycle - if path.contains(&name) { - path.push(name); + if path.contains(&(folder_url, name)) { + path.push((folder_url, name)); return Err(TaskError::TaskDepCycle { - path: path.iter().map(|s| s.to_string()).collect(), + path: path.iter().map(|(_, s)| s.to_string()).collect(), }); } - let Some((_, task_or_script)) = tasks_config.task(name) else { - return Err(TaskError::NotFound(name.to_string())); - }; - + let mut dependencies: Vec = Vec::new(); if let TaskOrScript::Task(_, task) = task_or_script { + dependencies.reserve(task.dependencies.len()); for dep in &task.dependencies { let mut path = path.clone(); - path.push(name); - sort_visit(dep, sorted, path, tasks_config)? + path.push((folder_url, name)); + dependencies.push(sort_visit(dep, sorted, path, tasks_config)?); } } - sorted.push(name.to_string()); + let id = sorted.len(); + sorted.push(ResolvedTask { + id, + name, + folder_url, + task_or_script, + dependencies, + }); - Ok(()) + Ok(id) } - let mut sorted: Vec = vec![]; + let mut sorted: Vec> = vec![]; for name in &pkg_task_config.matched_tasks { sort_visit(name, &mut sorted, Vec::new(), &pkg_task_config.tasks_config)?; diff --git a/tests/specs/task/dependencies_root_not_cycle/task.out b/tests/specs/task/dependencies_root_not_cycle/task.out index 65688381acff2d..0194f31e4e3fad 100644 --- a/tests/specs/task/dependencies_root_not_cycle/task.out +++ b/tests/specs/task/dependencies_root_not_cycle/task.out @@ -1,5 +1,5 @@ Task a echo root-a -a +root-a Task b echo b b Task a echo a diff --git a/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out b/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out index e8c87f2ff2c0b6..3b6fd0e0a440f0 100644 --- a/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out +++ b/tests/specs/task/dependencies_shadowed_root_name/member_depending_root_and_member.out @@ -1,10 +1,10 @@ [UNORDERED_START] Task build echo member member +Task build echo root +root Task root-depending-root echo test test -Task root echo root -root [UNORDERED_END] -Task member-depending-root-and-member echo member-test +Task member-dependending-root-and-member echo member-test member-test