From 94d3c12013963f289a34a9e0edf07556cfb5faa2 Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Thu, 28 Sep 2023 16:10:46 -0700 Subject: [PATCH 1/3] Fix extending. --- nextgen/config/src/inherited_tasks_config.rs | 4 +- nextgen/config/src/project_config.rs | 21 +-------- nextgen/config/tests/project_config_test.rs | 17 ------- nextgen/task-builder/src/lib.rs | 2 + nextgen/task-builder/src/tasks_builder.rs | 44 +++++++++++++------ .../task-builder/src/tasks_builder_error.rs | 14 ++++++ 6 files changed, 50 insertions(+), 52 deletions(-) create mode 100644 nextgen/task-builder/src/tasks_builder_error.rs diff --git a/nextgen/config/src/inherited_tasks_config.rs b/nextgen/config/src/inherited_tasks_config.rs index 842ec202244..9be70a8c52c 100644 --- a/nextgen/config/src/inherited_tasks_config.rs +++ b/nextgen/config/src/inherited_tasks_config.rs @@ -1,6 +1,6 @@ use crate::language_platform::{LanguageType, PlatformType}; use crate::project::{validate_deps, TaskConfig}; -use crate::project_config::{validate_tasks, ProjectType}; +use crate::project_config::ProjectType; use crate::shapes::InputPath; use crate::validate::check_yml_extension; use moon_common::path::standardize_separators; @@ -51,7 +51,7 @@ cacheable!( #[setting(merge = merge::append_vec)] pub implicit_inputs: Vec, - #[setting(nested, merge = merge::merge_btreemap, validate = validate_tasks)] + #[setting(nested, merge = merge::merge_btreemap)] pub tasks: BTreeMap, } ); diff --git a/nextgen/config/src/project_config.rs b/nextgen/config/src/project_config.rs index d49e0c56455..4aa7bd6847a 100644 --- a/nextgen/config/src/project_config.rs +++ b/nextgen/config/src/project_config.rs @@ -19,25 +19,6 @@ fn validate_channel(value: &str, _data: &D, _ctx: &C) -> Result<(), Valida Ok(()) } -pub fn validate_tasks( - tasks: &BTreeMap, - _data: &D, - _ctx: &C, -) -> Result<(), ValidateError> { - for (id, config) in tasks { - if let Some(extends_from) = &config.extends { - if !tasks.contains_key(extends_from) { - return Err(ValidateError::new(format!( - "task {} is extending an unknown task {}", - id, extends_from - ))); - } - } - } - - Ok(()) -} - derive_enum!( #[derive(ConfigEnum, Copy, Default)] pub enum ProjectType { @@ -108,7 +89,7 @@ cacheable!( pub tags: Vec, - #[setting(nested, validate = validate_tasks)] + #[setting(nested)] pub tasks: BTreeMap, #[setting(nested)] diff --git a/nextgen/config/tests/project_config_test.rs b/nextgen/config/tests/project_config_test.rs index f0f34b2d52d..4d35e1f8076 100644 --- a/nextgen/config/tests/project_config_test.rs +++ b/nextgen/config/tests/project_config_test.rs @@ -578,23 +578,6 @@ tasks: assert!(config.tasks.contains_key("base")); assert!(config.tasks.contains_key("extender")); } - - #[test] - #[should_panic(expected = "task extender is extending an unknown task unknown")] - fn errors_if_extending_unknown_task() { - test_load_config( - CONFIG_PROJECT_FILENAME, - r" -tasks: - base: - command: 'base' - extender: - extends: 'unknown' - args: '--more' -", - |path| ProjectConfig::load_from(path, "."), - ); - } } mod toolchain { diff --git a/nextgen/task-builder/src/lib.rs b/nextgen/task-builder/src/lib.rs index ec931c60acf..1ca4da4d144 100644 --- a/nextgen/task-builder/src/lib.rs +++ b/nextgen/task-builder/src/lib.rs @@ -1,3 +1,5 @@ mod tasks_builder; +mod tasks_builder_error; pub use tasks_builder::*; +pub use tasks_builder_error::*; diff --git a/nextgen/task-builder/src/tasks_builder.rs b/nextgen/task-builder/src/tasks_builder.rs index 2ff7582ae0a..ae1e123ccdb 100644 --- a/nextgen/task-builder/src/tasks_builder.rs +++ b/nextgen/task-builder/src/tasks_builder.rs @@ -1,5 +1,6 @@ #![allow(dead_code)] +use crate::tasks_builder_error::TasksBuilderError; use moon_args::split_args; use moon_common::{color, Id}; use moon_config::{ @@ -23,21 +24,31 @@ struct ConfigChain<'proj> { // This is a standalone function as recursive closures are not possible! fn extract_config<'builder, 'proj>( - task_id: &'builder Id, - tasks_map: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, configs: &'builder mut Vec>, - global: bool, -) { - if let Some(config) = tasks_map.get(task_id) { + task_id: &'builder Id, + all_task_configs: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, + global_task_ids: &'builder FxHashSet<&'proj Id>, +) -> miette::Result<()> { + if let Some(config) = all_task_configs.get(task_id) { if let Some(extend_task_id) = &config.extends { - extract_config(extend_task_id, tasks_map, configs, global); + if !all_task_configs.contains_key(extend_task_id) { + return Err(TasksBuilderError::UnknownExtendsSource { + source_id: task_id.to_owned(), + target_id: extend_task_id.to_owned(), + } + .into()); + } + + extract_config(configs, extend_task_id, all_task_configs, global_task_ids)?; } configs.push(ConfigChain { config, - inherited: global, + inherited: global_task_ids.contains(task_id), }); } + + Ok(()) } #[derive(Debug)] @@ -230,7 +241,7 @@ impl<'proj> TasksBuilder<'proj> { ); let mut task = Task::default(); - let chain = self.get_config_inherit_chain(id); + let chain = self.get_config_inherit_chain(id)?; // Determine command and args before building options and the task, // as we need to figure out if we're running in local mode or not. @@ -413,7 +424,7 @@ impl<'proj> TasksBuilder<'proj> { }; let configs = self - .get_config_inherit_chain(id) + .get_config_inherit_chain(id)? .iter() .map(|link| &link.config.options) .collect::>(); @@ -589,13 +600,20 @@ impl<'proj> TasksBuilder<'proj> { Ok((command, args)) } - fn get_config_inherit_chain(&self, id: &Id) -> Vec { + fn get_config_inherit_chain(&self, id: &Id) -> miette::Result> { let mut configs = vec![]; - extract_config(id, &self.global_tasks, &mut configs, true); - extract_config(id, &self.local_tasks, &mut configs, false); + // Merge maps so that "extending" can resolve the correct source + let mut all_tasks = FxHashMap::default(); + all_tasks.extend(&self.global_tasks); + all_tasks.extend(&self.local_tasks); + + let mut global_ids = FxHashSet::default(); + global_ids.extend(self.global_tasks.keys()); + + extract_config(&mut configs, id, &all_tasks, &global_ids)?; - configs + Ok(configs) } fn apply_filters_to_deps(&self, deps: Vec) -> Vec { diff --git a/nextgen/task-builder/src/tasks_builder_error.rs b/nextgen/task-builder/src/tasks_builder_error.rs new file mode 100644 index 00000000000..8f3e20afdbd --- /dev/null +++ b/nextgen/task-builder/src/tasks_builder_error.rs @@ -0,0 +1,14 @@ +use miette::Diagnostic; +use moon_common::{Id, Style, Stylize}; +use thiserror::Error; + +#[derive(Error, Debug, Diagnostic)] +pub enum TasksBuilderError { + #[diagnostic(code(task_builder::unknown_extends))] + #[error( + "Task {} is extending an unknown task {}.", + .source_id.style(Style::Id), + .target_id.style(Style::Id), + )] + UnknownExtendsSource { source_id: Id, target_id: Id }, +} From 738744a8437c4d92691712e2b96adcb499b6138b Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Fri, 29 Sep 2023 09:45:12 -0700 Subject: [PATCH 2/3] More work. --- nextgen/task-builder/src/tasks_builder.rs | 89 +++++++++++++++---- .../builder/extends-unknown/moon.yml | 5 ++ .../__fixtures__/builder/extends/moon.yml | 8 +- .../task-builder/tests/tasks_builder_test.rs | 27 ++++++ 4 files changed, 110 insertions(+), 19 deletions(-) create mode 100644 nextgen/task-builder/tests/__fixtures__/builder/extends-unknown/moon.yml diff --git a/nextgen/task-builder/src/tasks_builder.rs b/nextgen/task-builder/src/tasks_builder.rs index ae1e123ccdb..3e3f90fa752 100644 --- a/nextgen/task-builder/src/tasks_builder.rs +++ b/nextgen/task-builder/src/tasks_builder.rs @@ -26,12 +26,12 @@ struct ConfigChain<'proj> { fn extract_config<'builder, 'proj>( configs: &'builder mut Vec>, task_id: &'builder Id, - all_task_configs: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, - global_task_ids: &'builder FxHashSet<&'proj Id>, + tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, + inherited: bool, ) -> miette::Result<()> { - if let Some(config) = all_task_configs.get(task_id) { + if let Some(config) = tasks.get(task_id) { if let Some(extend_task_id) = &config.extends { - if !all_task_configs.contains_key(extend_task_id) { + if !tasks.contains_key(extend_task_id) { return Err(TasksBuilderError::UnknownExtendsSource { source_id: task_id.to_owned(), target_id: extend_task_id.to_owned(), @@ -39,18 +39,74 @@ fn extract_config<'builder, 'proj>( .into()); } - extract_config(configs, extend_task_id, all_task_configs, global_task_ids)?; + extract_config(configs, extend_task_id, tasks, inherited)?; } - configs.push(ConfigChain { - config, - inherited: global_task_ids.contains(task_id), - }); + configs.push(ConfigChain { config, inherited }); } Ok(()) } +// fn extract_config<'builder, 'proj>( +// configs: &'builder mut Vec>, +// task_id: &'builder Id, +// local_tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, +// global_tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, +// ) -> miette::Result<()> { +// let (inherited, config) = if let Some(cfg) = local_tasks.get(task_id) { +// (false, cfg) +// } else if let Some(cfg) = global_tasks.get(task_id) { +// (true, cfg) +// } else { +// return Ok(()); +// }; + +// if let Some(extend_task_id) = &config.extends { +// if !local_tasks.contains_key(extend_task_id) && !global_tasks.contains_key(extend_task_id) { +// return Err(TasksBuilderError::UnknownExtendsSource { +// source_id: task_id.to_owned(), +// target_id: extend_task_id.to_owned(), +// } +// .into()); +// } + +// extract_config(configs, extend_task_id, local_tasks, global_tasks)?; +// } + +// configs.push(ConfigChain { config, inherited }); + +// Ok(()) +// } + +// fn extract_config<'builder, 'proj>( +// configs: &'builder mut Vec>, +// task_id: &'builder Id, +// all_task_configs: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, +// global_task_ids: &'builder FxHashSet<&'proj Id>, +// ) -> miette::Result<()> { +// if let Some(config) = all_task_configs.get(task_id) { +// if let Some(extend_task_id) = &config.extends { +// if !all_task_configs.contains_key(extend_task_id) { +// return Err(TasksBuilderError::UnknownExtendsSource { +// source_id: task_id.to_owned(), +// target_id: extend_task_id.to_owned(), +// } +// .into()); +// } + +// extract_config(configs, extend_task_id, all_task_configs, global_task_ids)?; +// } + +// configs.push(ConfigChain { +// config, +// inherited: global_task_ids.contains(task_id), +// }); +// } + +// Ok(()) +// } + #[derive(Debug)] pub struct DetectPlatformEvent { pub enabled_platforms: Vec, @@ -602,16 +658,15 @@ impl<'proj> TasksBuilder<'proj> { fn get_config_inherit_chain(&self, id: &Id) -> miette::Result> { let mut configs = vec![]; + let mut tasks = FxHashMap::default(); - // Merge maps so that "extending" can resolve the correct source - let mut all_tasks = FxHashMap::default(); - all_tasks.extend(&self.global_tasks); - all_tasks.extend(&self.local_tasks); - - let mut global_ids = FxHashSet::default(); - global_ids.extend(self.global_tasks.keys()); + // Inherit all global first + tasks.extend(&self.global_tasks); + extract_config(&mut configs, id, &tasks, true)?; - extract_config(&mut configs, id, &all_tasks, &global_ids)?; + // Then all local + tasks.extend(&self.local_tasks); + extract_config(&mut configs, id, &tasks, false)?; Ok(configs) } diff --git a/nextgen/task-builder/tests/__fixtures__/builder/extends-unknown/moon.yml b/nextgen/task-builder/tests/__fixtures__/builder/extends-unknown/moon.yml new file mode 100644 index 00000000000..0341d87fed7 --- /dev/null +++ b/nextgen/task-builder/tests/__fixtures__/builder/extends-unknown/moon.yml @@ -0,0 +1,5 @@ +tags: [extends] + +tasks: + base: + extends: unknown diff --git a/nextgen/task-builder/tests/__fixtures__/builder/extends/moon.yml b/nextgen/task-builder/tests/__fixtures__/builder/extends/moon.yml index 2ba37affad0..5fc008309ca 100644 --- a/nextgen/task-builder/tests/__fixtures__/builder/extends/moon.yml +++ b/nextgen/task-builder/tests/__fixtures__/builder/extends/moon.yml @@ -36,8 +36,6 @@ tasks: extends: base local: false - # Tests global inheritance - local-base: inputs: - 'local-base' @@ -52,3 +50,9 @@ tasks: - 'local-extender' options: mergeArgs: 'prepend' + + local-extends-global: + extends: 'global-base' + inputs: + - 'local-extender' + diff --git a/nextgen/task-builder/tests/tasks_builder_test.rs b/nextgen/task-builder/tests/tasks_builder_test.rs index 217d78fc5ea..76e54401f00 100644 --- a/nextgen/task-builder/tests/tasks_builder_test.rs +++ b/nextgen/task-builder/tests/tasks_builder_test.rs @@ -1212,6 +1212,15 @@ mod tasks_builder { mod extending { use super::*; + #[tokio::test] + #[should_panic(expected = "Task base is extending an unknown task unknown.")] + async fn errors_for_unknown_extend_task() { + let sandbox = create_sandbox("builder"); + let tasks = build_tasks(sandbox.path(), "extends-unknown/moon.yml").await; + + tasks.get("base").unwrap(); + } + #[tokio::test] async fn handles_args() { let sandbox = create_sandbox("builder"); @@ -1286,6 +1295,24 @@ mod tasks_builder { assert_eq!(task.options.retry_count, 3); } + #[tokio::test] + async fn can_extend_a_global_from_local() { + let sandbox = create_sandbox("builder"); + let tasks = build_tasks(sandbox.path(), "extends/moon.yml").await; + let task = tasks.get("local-extends-global").unwrap(); + + assert_eq!(task.command, "global-base"); + assert_eq!( + task.inputs, + vec![ + InputPath::ProjectFile("global-base".into()), + InputPath::ProjectFile("local-extender".into()), + InputPath::WorkspaceGlob(".moon/*.yml".into()), + InputPath::WorkspaceFile("global/tasks/tag-extends.yml".into()), + ] + ); + } + #[tokio::test] async fn can_create_extends_chains() { let sandbox = create_sandbox("builder"); From 15749404aa56f9860e536d04c10e32600997c051 Mon Sep 17 00:00:00 2001 From: Miles Johnson Date: Fri, 29 Sep 2023 10:08:11 -0700 Subject: [PATCH 3/3] Fix tests. --- nextgen/task-builder/src/tasks_builder.rs | 105 ++++++++-------------- packages/cli/CHANGELOG.md | 4 + 2 files changed, 42 insertions(+), 67 deletions(-) diff --git a/nextgen/task-builder/src/tasks_builder.rs b/nextgen/task-builder/src/tasks_builder.rs index 3e3f90fa752..ae4058348a0 100644 --- a/nextgen/task-builder/src/tasks_builder.rs +++ b/nextgen/task-builder/src/tasks_builder.rs @@ -27,11 +27,19 @@ fn extract_config<'builder, 'proj>( configs: &'builder mut Vec>, task_id: &'builder Id, tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, + extendable_tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, inherited: bool, + extended: bool, ) -> miette::Result<()> { - if let Some(config) = tasks.get(task_id) { + let config = if extended { + extendable_tasks.get(task_id) + } else { + tasks.get(task_id) + }; + + if let Some(config) = config { if let Some(extend_task_id) = &config.extends { - if !tasks.contains_key(extend_task_id) { + if !extendable_tasks.contains_key(extend_task_id) { return Err(TasksBuilderError::UnknownExtendsSource { source_id: task_id.to_owned(), target_id: extend_task_id.to_owned(), @@ -39,7 +47,14 @@ fn extract_config<'builder, 'proj>( .into()); } - extract_config(configs, extend_task_id, tasks, inherited)?; + extract_config( + configs, + extend_task_id, + tasks, + extendable_tasks, + inherited, + true, + )?; } configs.push(ConfigChain { config, inherited }); @@ -48,65 +63,6 @@ fn extract_config<'builder, 'proj>( Ok(()) } -// fn extract_config<'builder, 'proj>( -// configs: &'builder mut Vec>, -// task_id: &'builder Id, -// local_tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, -// global_tasks: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, -// ) -> miette::Result<()> { -// let (inherited, config) = if let Some(cfg) = local_tasks.get(task_id) { -// (false, cfg) -// } else if let Some(cfg) = global_tasks.get(task_id) { -// (true, cfg) -// } else { -// return Ok(()); -// }; - -// if let Some(extend_task_id) = &config.extends { -// if !local_tasks.contains_key(extend_task_id) && !global_tasks.contains_key(extend_task_id) { -// return Err(TasksBuilderError::UnknownExtendsSource { -// source_id: task_id.to_owned(), -// target_id: extend_task_id.to_owned(), -// } -// .into()); -// } - -// extract_config(configs, extend_task_id, local_tasks, global_tasks)?; -// } - -// configs.push(ConfigChain { config, inherited }); - -// Ok(()) -// } - -// fn extract_config<'builder, 'proj>( -// configs: &'builder mut Vec>, -// task_id: &'builder Id, -// all_task_configs: &'builder FxHashMap<&'proj Id, &'proj TaskConfig>, -// global_task_ids: &'builder FxHashSet<&'proj Id>, -// ) -> miette::Result<()> { -// if let Some(config) = all_task_configs.get(task_id) { -// if let Some(extend_task_id) = &config.extends { -// if !all_task_configs.contains_key(extend_task_id) { -// return Err(TasksBuilderError::UnknownExtendsSource { -// source_id: task_id.to_owned(), -// target_id: extend_task_id.to_owned(), -// } -// .into()); -// } - -// extract_config(configs, extend_task_id, all_task_configs, global_task_ids)?; -// } - -// configs.push(ConfigChain { -// config, -// inherited: global_task_ids.contains(task_id), -// }); -// } - -// Ok(()) -// } - #[derive(Debug)] pub struct DetectPlatformEvent { pub enabled_platforms: Vec, @@ -658,15 +614,30 @@ impl<'proj> TasksBuilder<'proj> { fn get_config_inherit_chain(&self, id: &Id) -> miette::Result> { let mut configs = vec![]; - let mut tasks = FxHashMap::default(); + + let mut extendable_tasks = FxHashMap::default(); + extendable_tasks.extend(&self.global_tasks); + extendable_tasks.extend(&self.local_tasks); // Inherit all global first - tasks.extend(&self.global_tasks); - extract_config(&mut configs, id, &tasks, true)?; + extract_config( + &mut configs, + id, + &self.global_tasks, + &extendable_tasks, + true, + false, + )?; // Then all local - tasks.extend(&self.local_tasks); - extract_config(&mut configs, id, &tasks, false)?; + extract_config( + &mut configs, + id, + &self.local_tasks, + &extendable_tasks, + false, + false, + )?; Ok(configs) } diff --git a/packages/cli/CHANGELOG.md b/packages/cli/CHANGELOG.md index 32db7728580..e6213b99683 100644 --- a/packages/cli/CHANGELOG.md +++ b/packages/cli/CHANGELOG.md @@ -16,6 +16,10 @@ - Updated `moon dep-graph` to support a task in closest project, similar to `moon run`. +#### 🐞 Fixes + +- Fixed an issue where local tasks could not extend global tasks using the `extends` setting. + ## 1.14.2 #### 🐞 Fixes