From 220c2a37fab4345c4d743a771ff87e31abe27051 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 29 Jan 2025 16:35:31 -0500 Subject: [PATCH 01/16] Add boundaries config to turbo.json --- crates/turborepo-lib/src/boundaries/config.rs | 19 +++++++++++++++++++ .../src/{boundaries.rs => boundaries/mod.rs} | 3 +++ crates/turborepo-lib/src/turbo_json/mod.rs | 6 ++++++ 3 files changed, 28 insertions(+) create mode 100644 crates/turborepo-lib/src/boundaries/config.rs rename crates/turborepo-lib/src/{boundaries.rs => boundaries/mod.rs} (99%) diff --git a/crates/turborepo-lib/src/boundaries/config.rs b/crates/turborepo-lib/src/boundaries/config.rs new file mode 100644 index 0000000000000..85bf98da3f58f --- /dev/null +++ b/crates/turborepo-lib/src/boundaries/config.rs @@ -0,0 +1,19 @@ +use biome_deserialize_macros::Deserializable; +use serde::Serialize; +use struct_iterable::Iterable; +use turborepo_errors::Spanned; + +#[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct Permissions { + allow: Option>>>, + deny: Option>>>, +} + +#[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] +#[serde(rename_all = "camelCase")] +pub struct BoundariesConfig { + tags: Option>>>, + dependencies: Option>, + dependents: Option>, +} diff --git a/crates/turborepo-lib/src/boundaries.rs b/crates/turborepo-lib/src/boundaries/mod.rs similarity index 99% rename from crates/turborepo-lib/src/boundaries.rs rename to crates/turborepo-lib/src/boundaries/mod.rs index 86923ecc51107..cb880f24f0b90 100644 --- a/crates/turborepo-lib/src/boundaries.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -1,8 +1,11 @@ +mod config; + use std::{ collections::{BTreeMap, HashSet}, sync::{Arc, LazyLock, Mutex}, }; +pub use config::{BoundariesConfig, Permissions}; use git2::Repository; use globwalk::Settings; use itertools::Itertools; diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index 894db94c1d11e..58848ea16f335 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -32,6 +32,7 @@ pub mod parser; pub use loader::TurboJsonLoader; use crate::config::UnnecessaryPackageTaskSyntaxError; +use crate::boundaries::BoundariesConfig; #[derive(Serialize, Deserialize, Debug, Default, PartialEq, Clone, Deserializable)] #[serde(rename_all = "camelCase")] @@ -53,6 +54,7 @@ pub struct SpacesJson { pub struct TurboJson { text: Option>, path: Option>, + boundaries: Option>, pub(crate) extends: Spanned>, pub(crate) global_deps: Vec, pub(crate) global_env: Vec, @@ -146,6 +148,9 @@ pub struct RawTurboJson { #[serde(skip_serializing_if = "Option::is_none")] pub cache_dir: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub boundaries: Option>, + #[deserializable(rename = "//")] #[serde(skip)] _comment: Option, @@ -593,6 +598,7 @@ impl TryFrom for TurboJson { .extends .unwrap_or_default() .map(|s| s.into_iter().map(|s| s.into()).collect()), + boundaries: raw_turbo.boundaries, // Spaces and Remote Cache config is handled through layered config }) } From 17c1b2d00f5c2495d9257bbb166cede5e44de89b Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 30 Jan 2025 10:12:22 -0500 Subject: [PATCH 02/16] Add tests for boundaries parsing --- crates/turborepo-lib/src/turbo_json/mod.rs | 24 +++++++++++++++++++ ...po_lib__turbo_json__tests__dependents.snap | 16 +++++++++++++ ...json__tests__deserialize_boundaries-2.snap | 11 +++++++++ ...o_json__tests__deserialize_boundaries.snap | 9 +++++++ ...rborepo_lib__turbo_json__tests__empty.snap | 9 +++++++ ...epo_lib__turbo_json__tests__just tags.snap | 11 +++++++++ ...epo_lib__turbo_json__tests__just_tags.snap | 11 +++++++++ ...bo_json__tests__tags_and_dependencies.snap | 16 +++++++++++++ ..._json__tests__tags_and_dependencies_2.snap | 18 ++++++++++++++ ...urbo_json__tests__tags_and_dependents.snap | 18 ++++++++++++++ 10 files changed, 143 insertions(+) create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__dependents.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries-2.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just tags.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just_tags.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index 58848ea16f335..c4bb479c7c64e 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -770,12 +770,36 @@ mod tests { use super::{RawTurboJson, Spanned, TurboJson, UIMode}; use crate::{ + boundaries::BoundariesConfig, cli::OutputLogsMode, run::task_id::TaskName, task_graph::{TaskDefinition, TaskOutputs}, turbo_json::RawTaskDefinition, }; + #[test_case("{}", "empty")] + #[test_case(r#"{"tags": ["my-tag"] }"#, "just tags")] + #[test_case( + r#"{"tags": ["my-tag"], "dependencies": { "allow": ["my-package"] } }"#, + "tags and dependencies" + )] + #[test_case(r#"{"tags": ["my-tag"], "dependencies": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, "tags and dependencies 2")] + #[test_case(r#"{"tags": ["my-tag"], "dependents": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, "tags and dependents")] + #[test_case( + r#"{ "dependents": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, + "dependents" + )] + fn test_deserialize_boundaries(json: &str, name: &str) { + let deserialized_result = deserialize_from_json_str( + json, + JsonParserOptions::default().with_allow_comments(), + "turbo.json", + ); + let raw_task_definition: BoundariesConfig = + deserialized_result.into_deserialized().unwrap(); + insta::assert_json_snapshot!(name.replace(' ', "_"), raw_task_definition); + } + #[test_case( "{}", RawTaskDefinition::default(), diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__dependents.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__dependents.snap new file mode 100644 index 0000000000000..5b951aa8f4d10 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__dependents.snap @@ -0,0 +1,16 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": null, + "dependencies": null, + "dependents": { + "allow": [ + "my-package" + ], + "deny": [ + "my-other-package" + ] + } +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries-2.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries-2.snap new file mode 100644 index 0000000000000..5b648fe6e4aaa --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries-2.snap @@ -0,0 +1,11 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": null, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries.snap new file mode 100644 index 0000000000000..36fa44c506a17 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__deserialize_boundaries.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": null, + "dependencies": null, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap new file mode 100644 index 0000000000000..36fa44c506a17 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap @@ -0,0 +1,9 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": null, + "dependencies": null, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just tags.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just tags.snap new file mode 100644 index 0000000000000..5b648fe6e4aaa --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just tags.snap @@ -0,0 +1,11 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": null, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just_tags.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just_tags.snap new file mode 100644 index 0000000000000..5b648fe6e4aaa --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__just_tags.snap @@ -0,0 +1,11 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": null, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap new file mode 100644 index 0000000000000..eb34fd32a32c3 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap @@ -0,0 +1,16 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": { + "allow": [ + "my-package" + ], + "deny": null + }, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap new file mode 100644 index 0000000000000..4b958821b3ee3 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap @@ -0,0 +1,18 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": { + "allow": [ + "my-package" + ], + "deny": [ + "my-other-package" + ] + }, + "dependents": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap new file mode 100644 index 0000000000000..685df5bd029b7 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap @@ -0,0 +1,18 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": [ + "my-tag" + ], + "dependencies": null, + "dependents": { + "allow": [ + "my-package" + ], + "deny": [ + "my-other-package" + ] + } +} From 6c783ec0121c7390b03547ce0f9dd2912b3640d5 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 31 Jan 2025 10:58:37 -0500 Subject: [PATCH 03/16] Implementing core logic for checking tags --- crates/turborepo-errors/src/lib.rs | 19 +++ crates/turborepo-lib/src/boundaries/config.rs | 10 +- crates/turborepo-lib/src/boundaries/mod.rs | 133 +++++++++++++++++- .../turborepo-lib/src/commands/boundaries.rs | 2 +- crates/turborepo-lib/src/engine/builder.rs | 4 +- crates/turborepo-lib/src/run/builder.rs | 7 +- crates/turborepo-lib/src/run/mod.rs | 7 +- crates/turborepo-lib/src/turbo_json/loader.rs | 2 +- crates/turborepo-lib/src/turbo_json/mod.rs | 2 +- 9 files changed, 169 insertions(+), 17 deletions(-) diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index fe854b744e512..f332c0b0a8c30 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -1,5 +1,7 @@ use std::{ fmt::Display, + iter, + iter::Once, ops::{Deref, Range}, sync::Arc, }; @@ -76,6 +78,23 @@ pub struct Spanned { pub text: Option>, } +impl IntoIterator for Spanned { + type Item = T; + type IntoIter = Once; + + fn into_iter(self) -> Self::IntoIter { + iter::once(self.value) + } +} + +impl<'a, T> Iterator for &'a Spanned { + type Item = &'a T; + + fn next(&mut self) -> Option { + Some(&self.value) + } +} + impl Deserializable for Spanned { fn deserialize( value: &impl DeserializableValue, diff --git a/crates/turborepo-lib/src/boundaries/config.rs b/crates/turborepo-lib/src/boundaries/config.rs index 85bf98da3f58f..88bedf97a20ed 100644 --- a/crates/turborepo-lib/src/boundaries/config.rs +++ b/crates/turborepo-lib/src/boundaries/config.rs @@ -6,14 +6,14 @@ use turborepo_errors::Spanned; #[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] #[serde(rename_all = "camelCase")] pub struct Permissions { - allow: Option>>>, - deny: Option>>>, + pub allow: Option>>>, + pub deny: Option>>>, } #[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] #[serde(rename_all = "camelCase")] pub struct BoundariesConfig { - tags: Option>>>, - dependencies: Option>, - dependents: Option>, + pub tags: Option>>>, + pub dependencies: Option>, + pub dependents: Option>, } diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index cb880f24f0b90..e6613839ba6ca 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -28,10 +28,27 @@ use turborepo_repository::{ }; use turborepo_ui::{color, ColorConfig, BOLD_GREEN, BOLD_RED}; -use crate::run::Run; +use crate::{run::Run, turbo_json::TurboJson}; #[derive(Clone, Debug, Error, Diagnostic)] pub enum BoundariesDiagnostic { + #[error("Package `{package_name}` found without any tag listed in allowlist")] + NotAllowedTag { + package_name: PackageName, + #[label("tag not found here")] + span: Option, + #[source_code] + text: Arc, + }, + #[error("Package `{package_name}` found with tag listed in denylist: `{tag}`")] + DeniedTag { + package_name: PackageName, + tag: String, + #[label("tag found here")] + span: Option, + #[source_code] + text: Arc, + }, #[error( "importing from a type declaration package, but import is not declared as a type-only \ import" @@ -66,6 +83,8 @@ pub enum BoundariesDiagnostic { #[derive(Debug, Error, Diagnostic)] pub enum Error { + #[error(transparent)] + Config(#[from] crate::config::Error), #[error("file `{0}` does not have a parent directory")] NoParentDir(AbsoluteSystemPathBuf), #[error(transparent)] @@ -133,7 +152,7 @@ impl BoundariesResult { } impl Run { - pub async fn check_boundaries(&self) -> Result { + pub async fn check_boundaries(&mut self) -> Result { let packages = self.pkg_dep_graph().packages(); let repo = Repository::discover(self.repo_root()).ok().map(Mutex::new); let mut diagnostics = vec![]; @@ -146,8 +165,8 @@ impl Run { continue; } + let turbo_json = self.turbo_json_loader().uncached_load(package_name)?; let package_root = self.repo_root().resolve(package_info.package_path()); - let internal_dependencies = self .pkg_dep_graph() .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) @@ -160,6 +179,7 @@ impl Run { &repo, &package_root, &package_info.package_json, + &turbo_json, internal_dependencies, unresolved_external_dependencies, &source_map, @@ -183,6 +203,112 @@ impl Run { PACKAGE_NAME_REGEX.is_match(import) } + fn validate_relation( + &mut self, + dependency: &PackageName, + allow_list: &Option>, + deny_list: &Option>, + ) -> Result, Error> { + let dep_turbo_json = match self.turbo_json_loader().load(dependency) { + Ok(turbo_json) => turbo_json, + // If there is no turbo.json, then there are no rules to check + Err(crate::config::Error::NoTurboJSON) => &TurboJson::default(), + Err(e) => return Err(e.into()), + }; + + let dep_tags = dep_turbo_json + .boundaries + .as_ref() + .and_then(|b| b.tags.as_ref()) + .unwrap_or_default(); + + // If there is no allow list, then we allow all tags + let mut is_allowed = allow_list.is_none(); + let mut diagnostics = Vec::new(); + for tag in dep_tags.as_inner() { + if let Some(allow_list) = allow_list { + if allow_list.contains(tag.as_inner()) { + is_allowed = true; + } + } + + if let Some(deny_list) = deny_list { + if deny_list.contains(tag.as_inner()) { + let (span, text) = tag.span_and_text("turbo.json"); + diagnostics.push(BoundariesDiagnostic::DeniedTag { + package_name: dependency.clone(), + tag: tag.as_inner().to_string(), + span, + text: Arc::new(text), + }); + } + } + } + + if !is_allowed { + let (span, text) = dep_tags.span_and_text("turbo.json"); + diagnostics.push(BoundariesDiagnostic::NotAllowedTag { + package_name: dependency.clone(), + span, + text: Arc::new(text), + }); + } + + Ok(diagnostics) + } + + async fn check_package_tags( + &mut self, + repo: &Option>, + package_name: &PackageName, + package_root: &AbsoluteSystemPath, + package_json: &PackageJson, + boundaries_config: &BoundariesConfig, + ) -> Result, Error> { + let (dependency_allow_list, dependency_deny_list) = + if let Some(dependency_rules) = boundaries_config.dependencies.as_ref() { + // If list is `None`, we allow all dependencies. If it's `Some`, we allow only + // the listed dependencies. + let allow_list = dependency_rules + .allow + .as_ref() + .map(|allow| allow.iter().flatten().collect::>()); + + let deny_list = dependency_rules + .deny + .as_ref() + .map(|deny| deny.iter().flatten().collect::>()); + + (allow_list, deny_list) + } else { + (None, None) + }; + + let (dependent_allow_list, dependent_deny_list) = + if let Some(dependent_rules) = boundaries_config.dependents.as_ref() { + // If list is `None`, we allow all dependentes. If it's `Some`, we allow only + // the listed dependentes. + let allow_list = dependent_rules + .allow + .as_ref() + .map(|allow| allow.iter().flatten().collect::>()); + + let deny_list = dependent_rules + .deny + .as_ref() + .map(|deny| deny.iter().flatten().collect::>()); + + (allow_list, deny_list) + } else { + (None, None) + }; + + for dependency in self.pkg_dep_graph().dependencies(package_name) { + let diagnostics = + self.validate_relation(&dependency, &dependency_allow_list, &dependency_deny_list); + } + } + /// Either returns a list of errors and number of files checked or a single, /// fatal error async fn check_package( @@ -190,6 +316,7 @@ impl Run { repo: &Option>, package_root: &AbsoluteSystemPath, package_json: &PackageJson, + turbo_json: &TurboJson, internal_dependencies: HashSet<&PackageNode>, unresolved_external_dependencies: Option<&BTreeMap>, source_map: &SourceMap, diff --git a/crates/turborepo-lib/src/commands/boundaries.rs b/crates/turborepo-lib/src/commands/boundaries.rs index b5bf517290d1b..7d0e5e2a62797 100644 --- a/crates/turborepo-lib/src/commands/boundaries.rs +++ b/crates/turborepo-lib/src/commands/boundaries.rs @@ -11,7 +11,7 @@ pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result { repo_root: &'a AbsoluteSystemPath, package_graph: &'a PackageGraph, - turbo_json_loader: Option, + turbo_json_loader: Option<&'a mut TurboJsonLoader>, is_single: bool, workspaces: Vec, tasks: Vec>>, @@ -132,7 +132,7 @@ impl<'a> EngineBuilder<'a> { pub fn new( repo_root: &'a AbsoluteSystemPath, package_graph: &'a PackageGraph, - turbo_json_loader: TurboJsonLoader, + turbo_json_loader: &'a mut TurboJsonLoader, is_single: bool, ) -> Self { Self { diff --git a/crates/turborepo-lib/src/run/builder.rs b/crates/turborepo-lib/src/run/builder.rs index f0a7a46bd319a..3802a534c964b 100644 --- a/crates/turborepo-lib/src/run/builder.rs +++ b/crates/turborepo-lib/src/run/builder.rs @@ -436,7 +436,7 @@ impl RunBuilder { &pkg_dep_graph, &root_turbo_json, filtered_pkgs.keys(), - turbo_json_loader.clone(), + &mut turbo_json_loader, )?; if self.opts.run_opts.parallel { @@ -445,7 +445,7 @@ impl RunBuilder { &pkg_dep_graph, &root_turbo_json, filtered_pkgs.keys(), - turbo_json_loader, + &mut turbo_json_loader, )?; } @@ -480,6 +480,7 @@ impl RunBuilder { env_at_execution_start, filtered_pkgs: filtered_pkgs.keys().cloned().collect(), pkg_dep_graph: Arc::new(pkg_dep_graph), + turbo_json_loader, root_turbo_json, scm, engine: Arc::new(engine), @@ -496,7 +497,7 @@ impl RunBuilder { pkg_dep_graph: &PackageGraph, root_turbo_json: &TurboJson, filtered_pkgs: impl Iterator, - turbo_json_loader: TurboJsonLoader, + turbo_json_loader: &mut TurboJsonLoader, ) -> Result { let tasks = self.opts.run_opts.tasks.iter().map(|task| { // TODO: Pull span info from command diff --git a/crates/turborepo-lib/src/run/mod.rs b/crates/turborepo-lib/src/run/mod.rs index 7ea0389e711fc..020acb4906b04 100644 --- a/crates/turborepo-lib/src/run/mod.rs +++ b/crates/turborepo-lib/src/run/mod.rs @@ -48,7 +48,7 @@ use crate::{ signal::SignalHandler, task_graph::Visitor, task_hash::{get_external_deps_hash, get_internal_deps_hash, PackageInputsHashes}, - turbo_json::{TurboJson, UIMode}, + turbo_json::{TurboJson, TurboJsonLoader, UIMode}, DaemonClient, DaemonConnector, }; @@ -66,6 +66,7 @@ pub struct Run { env_at_execution_start: EnvironmentVariableMap, filtered_pkgs: HashSet, pkg_dep_graph: Arc, + pub(crate) turbo_json_loader: TurboJsonLoader, root_turbo_json: TurboJson, scm: SCM, run_cache: Arc, @@ -122,6 +123,10 @@ impl Run { } } + pub fn turbo_json_loader(&self) -> &TurboJsonLoader { + &self.turbo_json_loader + } + pub fn opts(&self) -> &Opts { &self.opts } diff --git a/crates/turborepo-lib/src/turbo_json/loader.rs b/crates/turborepo-lib/src/turbo_json/loader.rs index 01bc26f1e61b5..b4f20fb776e1d 100644 --- a/crates/turborepo-lib/src/turbo_json/loader.rs +++ b/crates/turborepo-lib/src/turbo_json/loader.rs @@ -163,7 +163,7 @@ impl TurboJsonLoader { .expect("just inserted value for this key")) } - fn uncached_load(&self, package: &PackageName) -> Result { + pub fn uncached_load(&self, package: &PackageName) -> Result { match &self.strategy { Strategy::SinglePackage { package_json, diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index c4bb479c7c64e..2bcacdb235c49 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -54,7 +54,7 @@ pub struct SpacesJson { pub struct TurboJson { text: Option>, path: Option>, - boundaries: Option>, + pub(crate) boundaries: Option>, pub(crate) extends: Spanned>, pub(crate) global_deps: Vec, pub(crate) global_env: Vec, From edd4364c7a71a48c342796d0dde9baae5146988b Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 4 Feb 2025 16:09:15 -0500 Subject: [PATCH 04/16] Finished first pass at boundaries tags --- crates/turborepo-errors/src/lib.rs | 9 +- crates/turborepo-lib/src/boundaries/mod.rs | 174 +++++++++++++----- .../turborepo-lib/src/commands/boundaries.rs | 2 +- crates/turborepo-lib/src/query/boundaries.rs | 26 +++ 4 files changed, 158 insertions(+), 53 deletions(-) diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index f332c0b0a8c30..1504afbdc80df 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -2,7 +2,7 @@ use std::{ fmt::Display, iter, iter::Once, - ops::{Deref, Range}, + ops::{Deref, DerefMut, Range}, sync::Arc, }; @@ -223,6 +223,13 @@ impl Deref for Spanned { &self.value } } + +impl DerefMut for Spanned { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.value + } +} + pub trait WithMetadata { fn add_text(&mut self, text: Arc); fn add_path(&mut self, path: Arc); diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index e6613839ba6ca..afeb8260963d1 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -1,11 +1,11 @@ mod config; use std::{ - collections::{BTreeMap, HashSet}, + collections::{BTreeMap, HashMap, HashSet}, sync::{Arc, LazyLock, Mutex}, }; -pub use config::{BoundariesConfig, Permissions}; +pub use config::BoundariesConfig; use git2::Repository; use globwalk::Settings; use itertools::Itertools; @@ -152,7 +152,8 @@ impl BoundariesResult { } impl Run { - pub async fn check_boundaries(&mut self) -> Result { + pub async fn check_boundaries(&self) -> Result { + let boundaries_configs = self.get_boundaries_configs(); let packages = self.pkg_dep_graph().packages(); let repo = Repository::discover(self.repo_root()).ok().map(Mutex::new); let mut diagnostics = vec![]; @@ -165,7 +166,6 @@ impl Run { continue; } - let turbo_json = self.turbo_json_loader().uncached_load(package_name)?; let package_root = self.repo_root().resolve(package_info.package_path()); let internal_dependencies = self .pkg_dep_graph() @@ -178,11 +178,12 @@ impl Run { .check_package( &repo, &package_root, + package_name, &package_info.package_json, - &turbo_json, internal_dependencies, unresolved_external_dependencies, &source_map, + &boundaries_configs, ) .await?; @@ -199,33 +200,47 @@ impl Run { }) } + fn get_boundaries_configs(&self) -> HashMap { + let mut boundaries_configs = HashMap::new(); + for (package, _) in self.pkg_dep_graph().packages() { + if let Ok(TurboJson { + boundaries: Some(boundaries_config), + .. + }) = self.turbo_json_loader().uncached_load(package) + { + boundaries_configs.insert(package.clone(), boundaries_config.into_inner()); + } + } + + boundaries_configs + } + fn is_potential_package_name(import: &str) -> bool { PACKAGE_NAME_REGEX.is_match(import) } fn validate_relation( - &mut self, + &self, dependency: &PackageName, allow_list: &Option>, deny_list: &Option>, + boundaries_config: Option<&BoundariesConfig>, ) -> Result, Error> { - let dep_turbo_json = match self.turbo_json_loader().load(dependency) { - Ok(turbo_json) => turbo_json, - // If there is no turbo.json, then there are no rules to check - Err(crate::config::Error::NoTurboJSON) => &TurboJson::default(), - Err(e) => return Err(e.into()), - }; - - let dep_tags = dep_turbo_json - .boundaries - .as_ref() - .and_then(|b| b.tags.as_ref()) - .unwrap_or_default(); - // If there is no allow list, then we allow all tags let mut is_allowed = allow_list.is_none(); let mut diagnostics = Vec::new(); - for tag in dep_tags.as_inner() { + let dep_tags = boundaries_config + .and_then(|b| b.tags.as_ref()) + .map(|tags| tags.as_inner()) + .into_iter() + .flatten() + .collect::>(); + + let dep_tags_span = boundaries_config + .and_then(|b| b.tags.as_ref()) + .map_or(Default::default(), |b| b.to(())); + + for tag in dep_tags { if let Some(allow_list) = allow_list { if allow_list.contains(tag.as_inner()) { is_allowed = true; @@ -246,7 +261,7 @@ impl Run { } if !is_allowed { - let (span, text) = dep_tags.span_and_text("turbo.json"); + let (span, text) = dep_tags_span.span_and_text("turbo.json"); diagnostics.push(BoundariesDiagnostic::NotAllowedTag { package_name: dependency.clone(), span, @@ -257,27 +272,28 @@ impl Run { Ok(diagnostics) } - async fn check_package_tags( - &mut self, - repo: &Option>, - package_name: &PackageName, - package_root: &AbsoluteSystemPath, - package_json: &PackageJson, - boundaries_config: &BoundariesConfig, + fn check_package_tags( + &self, + pkg: PackageNode, + package_boundaries_config: BoundariesConfig, + boundaries_configs: &HashMap, ) -> Result, Error> { let (dependency_allow_list, dependency_deny_list) = - if let Some(dependency_rules) = boundaries_config.dependencies.as_ref() { + if let Some(mut dependency_rules) = package_boundaries_config.dependencies { + let allow_rules = dependency_rules.allow.take(); + let deny_rules = dependency_rules.deny.take(); // If list is `None`, we allow all dependencies. If it's `Some`, we allow only // the listed dependencies. - let allow_list = dependency_rules - .allow - .as_ref() - .map(|allow| allow.iter().flatten().collect::>()); + let allow_list = allow_rules.map(|allow| { + allow + .into_iter() + .flatten() + .flatten() + .collect::>() + }); - let deny_list = dependency_rules - .deny - .as_ref() - .map(|deny| deny.iter().flatten().collect::>()); + let deny_list = deny_rules + .map(|deny| deny.into_iter().flatten().flatten().collect::>()); (allow_list, deny_list) } else { @@ -285,38 +301,94 @@ impl Run { }; let (dependent_allow_list, dependent_deny_list) = - if let Some(dependent_rules) = boundaries_config.dependents.as_ref() { + if let Some(mut dependent_rules) = package_boundaries_config.dependents { + let allow_rules = dependent_rules.allow.take(); + let deny_rules = dependent_rules.deny.take(); + // If list is `None`, we allow all dependentes. If it's `Some`, we allow only // the listed dependentes. - let allow_list = dependent_rules - .allow - .as_ref() - .map(|allow| allow.iter().flatten().collect::>()); - - let deny_list = dependent_rules - .deny - .as_ref() - .map(|deny| deny.iter().flatten().collect::>()); + let allow_list = allow_rules.map(|allow| { + allow + .into_iter() + .flatten() + .flatten() + .collect::>() + }); + + let deny_list = deny_rules.map(|deny| { + deny.into_iter() + .flatten() + .flatten() + .collect::>() + }); (allow_list, deny_list) } else { (None, None) }; - for dependency in self.pkg_dep_graph().dependencies(package_name) { - let diagnostics = - self.validate_relation(&dependency, &dependency_allow_list, &dependency_deny_list); + let mut diagnostics = Vec::new(); + for dependency in self.pkg_dep_graph().dependencies(&pkg) { + diagnostics.extend(self.validate_relation( + dependency.as_package_name(), + &dependency_allow_list, + &dependency_deny_list, + boundaries_configs.get(dependency.as_package_name()), + )?); } + + for dependent in self.pkg_dep_graph().ancestors(&pkg) { + diagnostics.extend(self.validate_relation( + dependent.as_package_name(), + &dependent_allow_list, + &dependent_deny_list, + boundaries_configs.get(dependent.as_package_name()), + )?); + } + + Ok(diagnostics) } /// Either returns a list of errors and number of files checked or a single, /// fatal error async fn check_package( + &self, + repo: &Option>, + package_root: &AbsoluteSystemPath, + package_name: &PackageName, + package_json: &PackageJson, + internal_dependencies: HashSet<&PackageNode>, + unresolved_external_dependencies: Option<&BTreeMap>, + source_map: &SourceMap, + boundaries_configs: &HashMap, + ) -> Result, Error> { + let mut diagnostics = self + .check_package_files( + repo, + package_root, + package_json, + internal_dependencies, + unresolved_external_dependencies, + source_map, + ) + .await?; + + if let Some(boundaries_config) = boundaries_configs.get(package_name) { + diagnostics.extend(self.check_package_tags( + PackageNode::Workspace(package_name.clone()), + boundaries_config.clone(), + boundaries_configs, + )?); + } + + Ok(diagnostics) + } + + async fn check_package_files( &self, repo: &Option>, package_root: &AbsoluteSystemPath, package_json: &PackageJson, - turbo_json: &TurboJson, internal_dependencies: HashSet<&PackageNode>, unresolved_external_dependencies: Option<&BTreeMap>, source_map: &SourceMap, diff --git a/crates/turborepo-lib/src/commands/boundaries.rs b/crates/turborepo-lib/src/commands/boundaries.rs index 7d0e5e2a62797..b5bf517290d1b 100644 --- a/crates/turborepo-lib/src/commands/boundaries.rs +++ b/crates/turborepo-lib/src/commands/boundaries.rs @@ -11,7 +11,7 @@ pub async fn run(base: CommandBase, telemetry: CommandEventBuilder) -> Result for Diagnostic { path: None, reason: None, }, + + BoundariesDiagnostic::NotAllowedTag { + package_name, + span, + text, + } => Diagnostic { + message, + path: Some(text.name().to_string()), + start: span.map(|span| span.offset()), + end: span.map(|span| span.offset() + span.len()), + import: Some(package_name.to_string()), + reason: None, + }, + BoundariesDiagnostic::DeniedTag { + package_name, + tag, + span, + text, + } => Diagnostic { + message, + path: Some(text.name().to_string()), + start: span.map(|span| span.offset()), + end: span.map(|span| span.offset() + span.len()), + import: Some(package_name.to_string()), + reason: Some(tag), + }, } } } From f7792a97a62c288903680571ed47458e71d337cb Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 4 Feb 2025 17:45:24 -0500 Subject: [PATCH 05/16] Adding tests, iterating on error message and UX --- crates/turborepo-lib/src/boundaries/mod.rs | 70 ++++++++++++++----- crates/turborepo-lib/src/query/boundaries.rs | 3 + crates/turborepo-lib/src/turbo_json/parser.rs | 58 +++++++++++++++ ...get_boundaries_lints_(npm@10.5.0).snap.new | 37 ++++++++++ .../boundaries/apps/my-app/package.json | 3 +- .../boundaries/apps/my-app/turbo.json | 15 ++++ .../boundaries/packages/ship-types/turbo.json | 7 ++ .../packages/unsafe-package/package.json | 3 + .../packages/unsafe-package/turbo.json | 7 ++ 9 files changed, 185 insertions(+), 18 deletions(-) create mode 100644 crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new create mode 100644 turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index afeb8260963d1..6b6348d013b7b 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -5,7 +5,7 @@ use std::{ sync::{Arc, LazyLock, Mutex}, }; -pub use config::BoundariesConfig; +pub use config::{BoundariesConfig, Permissions}; use git2::Repository; use globwalk::Settings; use itertools::Itertools; @@ -22,6 +22,7 @@ use thiserror::Error; use tracing::log::warn; use turbo_trace::{ImportFinder, ImportType, Tracer}; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathRelation, RelativeUnixPath}; +use turborepo_errors::Spanned; use turborepo_repository::{ package_graph::{PackageName, PackageNode}, package_json::PackageJson, @@ -32,16 +33,28 @@ use crate::{run::Run, turbo_json::TurboJson}; #[derive(Clone, Debug, Error, Diagnostic)] pub enum BoundariesDiagnostic { - #[error("Package `{package_name}` found without any tag listed in allowlist")] + #[error( + "Package `{package_name}` found without any tag listed in allowlist for \ + `{source_package_name}`" + )] NotAllowedTag { + // The package that is declaring the allowlist + source_package_name: PackageName, + // The package that is either a dependency or dependent of the source package package_name: PackageName, #[label("tag not found here")] span: Option, + #[help] + help: Option, #[source_code] text: Arc, }, - #[error("Package `{package_name}` found with tag listed in denylist: `{tag}`")] + #[error( + "Package `{package_name}` found with tag listed in denylist for `{source_package_name}`: \ + `{tag}`" + )] DeniedTag { + source_package_name: PackageName, package_name: PackageName, tag: String, #[label("tag found here")] @@ -200,7 +213,7 @@ impl Run { }) } - fn get_boundaries_configs(&self) -> HashMap { + fn get_boundaries_configs(&self) -> HashMap> { let mut boundaries_configs = HashMap::new(); for (package, _) in self.pkg_dep_graph().packages() { if let Ok(TurboJson { @@ -208,7 +221,7 @@ impl Run { .. }) = self.turbo_json_loader().uncached_load(package) { - boundaries_configs.insert(package.clone(), boundaries_config.into_inner()); + boundaries_configs.insert(package.clone(), boundaries_config); } } @@ -221,14 +234,15 @@ impl Run { fn validate_relation( &self, + package_name: &PackageName, dependency: &PackageName, allow_list: &Option>, deny_list: &Option>, - boundaries_config: Option<&BoundariesConfig>, - ) -> Result, Error> { + boundaries_config: Option<&Spanned>, + ) -> Result, Error> { // If there is no allow list, then we allow all tags let mut is_allowed = allow_list.is_none(); - let mut diagnostics = Vec::new(); + let dep_tags = boundaries_config .and_then(|b| b.tags.as_ref()) .map(|tags| tags.as_inner()) @@ -238,7 +252,9 @@ impl Run { let dep_tags_span = boundaries_config .and_then(|b| b.tags.as_ref()) - .map_or(Default::default(), |b| b.to(())); + .map(|b| b.to(())) + .or_else(|| boundaries_config.map(|b| b.to(()))) + .unwrap_or_default(); for tag in dep_tags { if let Some(allow_list) = allow_list { @@ -250,33 +266,43 @@ impl Run { if let Some(deny_list) = deny_list { if deny_list.contains(tag.as_inner()) { let (span, text) = tag.span_and_text("turbo.json"); - diagnostics.push(BoundariesDiagnostic::DeniedTag { + return Ok(Some(BoundariesDiagnostic::DeniedTag { + source_package_name: package_name.clone(), package_name: dependency.clone(), tag: tag.as_inner().to_string(), span, text: Arc::new(text), - }); + })); } } } if !is_allowed { let (span, text) = dep_tags_span.span_and_text("turbo.json"); - diagnostics.push(BoundariesDiagnostic::NotAllowedTag { + let help = span.is_none().then(|| { + format!( + "`{}` doesn't any boundaries config in its `turbo.json` file", + dependency + ) + }); + + return Ok(Some(BoundariesDiagnostic::NotAllowedTag { + source_package_name: package_name.clone(), package_name: dependency.clone(), + help, span, text: Arc::new(text), - }); + })); } - Ok(diagnostics) + Ok(None) } fn check_package_tags( &self, pkg: PackageNode, package_boundaries_config: BoundariesConfig, - boundaries_configs: &HashMap, + boundaries_configs: &HashMap>, ) -> Result, Error> { let (dependency_allow_list, dependency_deny_list) = if let Some(mut dependency_rules) = package_boundaries_config.dependencies { @@ -329,7 +355,12 @@ impl Run { let mut diagnostics = Vec::new(); for dependency in self.pkg_dep_graph().dependencies(&pkg) { + if matches!(dependency, PackageNode::Root) { + continue; + } + diagnostics.extend(self.validate_relation( + pkg.as_package_name(), dependency.as_package_name(), &dependency_allow_list, &dependency_deny_list, @@ -338,7 +369,12 @@ impl Run { } for dependent in self.pkg_dep_graph().ancestors(&pkg) { + if matches!(dependent, PackageNode::Root) { + continue; + } + diagnostics.extend(self.validate_relation( + pkg.as_package_name(), dependent.as_package_name(), &dependent_allow_list, &dependent_deny_list, @@ -360,7 +396,7 @@ impl Run { internal_dependencies: HashSet<&PackageNode>, unresolved_external_dependencies: Option<&BTreeMap>, source_map: &SourceMap, - boundaries_configs: &HashMap, + boundaries_configs: &HashMap>, ) -> Result, Error> { let mut diagnostics = self .check_package_files( @@ -376,7 +412,7 @@ impl Run { if let Some(boundaries_config) = boundaries_configs.get(package_name) { diagnostics.extend(self.check_package_tags( PackageNode::Workspace(package_name.clone()), - boundaries_config.clone(), + boundaries_config.as_inner().clone(), boundaries_configs, )?); } diff --git a/crates/turborepo-lib/src/query/boundaries.rs b/crates/turborepo-lib/src/query/boundaries.rs index 0c244d4505723..526644b6d5f18 100644 --- a/crates/turborepo-lib/src/query/boundaries.rs +++ b/crates/turborepo-lib/src/query/boundaries.rs @@ -39,6 +39,8 @@ impl From for Diagnostic { }, BoundariesDiagnostic::NotAllowedTag { + source_package_name: _, + help: _, package_name, span, text, @@ -51,6 +53,7 @@ impl From for Diagnostic { reason: None, }, BoundariesDiagnostic::DeniedTag { + source_package_name: _, package_name, tag, span, diff --git a/crates/turborepo-lib/src/turbo_json/parser.rs b/crates/turborepo-lib/src/turbo_json/parser.rs index 456d5a8b21ec3..87b3e711af50e 100644 --- a/crates/turborepo-lib/src/turbo_json/parser.rs +++ b/crates/turborepo-lib/src/turbo_json/parser.rs @@ -15,6 +15,7 @@ use turborepo_errors::{ParseDiagnostic, WithMetadata}; use turborepo_unescape::UnescapedString; use crate::{ + boundaries::{BoundariesConfig, Permissions}, run::task_id::TaskName, turbo_json::{Pipeline, RawTaskDefinition, RawTurboJson, Spanned}, }; @@ -103,6 +104,11 @@ impl WithMetadata for RawTurboJson { self.global_dependencies.add_text(text.clone()); self.global_env.add_text(text.clone()); self.global_pass_through_env.add_text(text.clone()); + self.boundaries.add_text(text.clone()); + self.boundaries + .as_mut() + .map(|boundaries| boundaries.value.add_text(text.clone())); + self.tasks.add_text(text.clone()); self.cache_dir.add_text(text.clone()); self.pipeline.add_text(text); @@ -114,6 +120,10 @@ impl WithMetadata for RawTurboJson { self.global_dependencies.add_path(path.clone()); self.global_env.add_path(path.clone()); self.global_pass_through_env.add_path(path.clone()); + self.boundaries.add_path(path.clone()); + self.boundaries + .as_mut() + .map(|boundaries| boundaries.value.add_path(path.clone())); self.tasks.add_path(path.clone()); self.cache_dir.add_path(path.clone()); self.pipeline.add_path(path); @@ -136,6 +146,54 @@ impl WithMetadata for Pipeline { } } +impl WithMetadata for BoundariesConfig { + fn add_text(&mut self, text: Arc) { + self.tags.add_text(text.clone()); + self.tags + .as_mut() + .map(|tags| tags.value.add_text(text.clone())); + self.dependencies.add_text(text.clone()); + self.dependencies + .as_mut() + .map(|dependencies| dependencies.value.add_text(text.clone())); + self.dependents.add_text(text.clone()); + self.dependents + .as_mut() + .map(|dependents| dependents.value.add_text(text.clone())); + } + + fn add_path(&mut self, path: Arc) { + self.tags.add_path(path.clone()); + self.tags + .as_mut() + .map(|tags| tags.value.add_path(path.clone())); + self.dependencies.add_path(path.clone()); + self.dependencies + .as_mut() + .map(|dependencies| dependencies.value.add_path(path.clone())); + self.dependents.add_path(path.clone()); + self.dependents + .as_mut() + .map(|dependents| dependents.value.add_path(path.clone())); + } +} + +impl WithMetadata for Permissions { + fn add_text(&mut self, text: Arc) { + self.allow.add_text(text.clone()); + self.allow.as_mut().map(|s| s.value.add_text(text.clone())); + self.deny.add_text(text.clone()); + self.deny.as_mut().map(|s| s.value.add_text(text)); + } + + fn add_path(&mut self, path: Arc) { + self.allow.add_path(path.clone()); + self.allow.as_mut().map(|s| s.value.add_path(path.clone())); + self.deny.add_path(path.clone()); + self.deny.as_mut().map(|s| s.value.add_path(path)); + } +} + impl WithMetadata for RawTaskDefinition { fn add_text(&mut self, text: Arc) { self.depends_on.add_text(text.clone()); diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new new file mode 100644 index 0000000000000..5006334c5f398 --- /dev/null +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new @@ -0,0 +1,37 @@ +--- +source: crates/turborepo/tests/boundaries.rs +assertion_line: 5 +expression: query_output +--- +{ + "data": { + "boundaries": { + "items": [ + { + "message": "cannot import file `../../packages/another/index.jsx` because it leaves the package", + "import": "../../packages/another/index.jsx" + }, + { + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "ship" + }, + { + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "@types/ship" + }, + { + "message": "cannot import package `module-package` because it is not a dependency", + "import": "module-package" + }, + { + "message": "Package `@vercel/unsafe-package` found with tag listed in denylist for `my-app`: `unsafe`", + "import": "@vercel/unsafe-package" + }, + { + "message": "Package `module-package` found without any tag listed in allowlist for `my-app`", + "import": "module-package" + } + ] + } + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json index 1597bcae5865f..d7d03805d3fa5 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/package.json @@ -5,6 +5,7 @@ "maybefails": "exit 4" }, "dependencies": { - "@types/ship": "*" + "@types/ship": "*", + "@vercel/unsafe-package": "*" } } diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json new file mode 100644 index 0000000000000..58080a56d40f9 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json @@ -0,0 +1,15 @@ +{ + "boundaries": { + "tags": [ + "web" + ], + "dependencies": { + "allow": [ + "types" + ], + "deny": [ + "unsafe" + ] + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json new file mode 100644 index 0000000000000..fe98316624e4f --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json @@ -0,0 +1,7 @@ +{ + "boundaries": { + "tags": [ + "types" + ] + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/package.json b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/package.json new file mode 100644 index 0000000000000..e6ec58f2e2b71 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/package.json @@ -0,0 +1,3 @@ +{ + "name": "@vercel/unsafe-package" +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json new file mode 100644 index 0000000000000..d2613b5cd9815 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json @@ -0,0 +1,7 @@ +{ + "boundaries": { + "tags": [ + "unsafe" + ] + } +} \ No newline at end of file From 2b5ce026baee4e2b354923a2bd0edd7a3f152f65 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 4 Feb 2025 17:51:16 -0500 Subject: [PATCH 06/16] Updating tests --- crates/turborepo-lib/src/boundaries/mod.rs | 6 +-- crates/turborepo/tests/boundaries.rs | 12 ++++++ ...epo_get_boundaries_lints_(npm@10.5.0).snap | 11 ++++++ ...ies_get_boundaries_lints_(npm@10.5.0).snap | 8 ++++ ...get_boundaries_lints_(npm@10.5.0).snap.new | 37 ------------------- 5 files changed, 34 insertions(+), 40 deletions(-) create mode 100644 crates/turborepo/tests/snapshots/boundaries__basic_monorepo_get_boundaries_lints_(npm@10.5.0).snap delete mode 100644 crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 6b6348d013b7b..24c0f2c7746f6 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -397,8 +397,8 @@ impl Run { unresolved_external_dependencies: Option<&BTreeMap>, source_map: &SourceMap, boundaries_configs: &HashMap>, - ) -> Result, Error> { - let mut diagnostics = self + ) -> Result<(usize, Vec), Error> { + let (files_checked, mut diagnostics) = self .check_package_files( repo, package_root, @@ -417,7 +417,7 @@ impl Run { )?); } - Ok(diagnostics) + Ok((files_checked, diagnostics)) } async fn check_package_files( diff --git a/crates/turborepo/tests/boundaries.rs b/crates/turborepo/tests/boundaries.rs index 69a1cedf96ef8..2dafaa75d2bd5 100644 --- a/crates/turborepo/tests/boundaries.rs +++ b/crates/turborepo/tests/boundaries.rs @@ -11,3 +11,15 @@ fn test_boundaries() -> Result<(), anyhow::Error> { Ok(()) } + +#[test] +fn test_boundaries_on_basic_monorepo() -> Result<(), anyhow::Error> { + check_json!( + "basic_monorepo", + "npm@10.5.0", + "query", + "get boundaries lints" => "query { boundaries { items { message import } } }", + ); + + Ok(()) +} diff --git a/crates/turborepo/tests/snapshots/boundaries__basic_monorepo_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__basic_monorepo_get_boundaries_lints_(npm@10.5.0).snap new file mode 100644 index 0000000000000..4f81559be0ee9 --- /dev/null +++ b/crates/turborepo/tests/snapshots/boundaries__basic_monorepo_get_boundaries_lints_(npm@10.5.0).snap @@ -0,0 +1,11 @@ +--- +source: crates/turborepo/tests/boundaries.rs +expression: query_output +--- +{ + "data": { + "boundaries": { + "items": [] + } + } +} diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap index d1dc57842e99e..d8e2128d06ff6 100644 --- a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap @@ -21,6 +21,14 @@ expression: query_output { "message": "cannot import package `module-package` because it is not a dependency", "import": "module-package" + }, + { + "message": "Package `module-package` found without any tag listed in allowlist for `my-app`", + "import": "module-package" + }, + { + "message": "Package `@vercel/unsafe-package` found with tag listed in denylist for `my-app`: `unsafe`", + "import": "@vercel/unsafe-package" } ] } diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new deleted file mode 100644 index 5006334c5f398..0000000000000 --- a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap.new +++ /dev/null @@ -1,37 +0,0 @@ ---- -source: crates/turborepo/tests/boundaries.rs -assertion_line: 5 -expression: query_output ---- -{ - "data": { - "boundaries": { - "items": [ - { - "message": "cannot import file `../../packages/another/index.jsx` because it leaves the package", - "import": "../../packages/another/index.jsx" - }, - { - "message": "importing from a type declaration package, but import is not declared as a type-only import", - "import": "ship" - }, - { - "message": "importing from a type declaration package, but import is not declared as a type-only import", - "import": "@types/ship" - }, - { - "message": "cannot import package `module-package` because it is not a dependency", - "import": "module-package" - }, - { - "message": "Package `@vercel/unsafe-package` found with tag listed in denylist for `my-app`: `unsafe`", - "import": "@vercel/unsafe-package" - }, - { - "message": "Package `module-package` found without any tag listed in allowlist for `my-app`", - "import": "module-package" - } - ] - } - } -} From c124ba75284ac59bd5878f234185f6518e02ff68 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 5 Feb 2025 10:41:00 -0500 Subject: [PATCH 07/16] Some light clean up - Remove unused code - Rename slightly confusing variables --- crates/turborepo-errors/src/lib.rs | 8 -------- crates/turborepo-lib/src/boundaries/mod.rs | 8 ++++---- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index 1504afbdc80df..814799502012d 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -87,14 +87,6 @@ impl IntoIterator for Spanned { } } -impl<'a, T> Iterator for &'a Spanned { - type Item = &'a T; - - fn next(&mut self) -> Option { - Some(&self.value) - } -} - impl Deserializable for Spanned { fn deserialize( value: &impl DeserializableValue, diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 24c0f2c7746f6..a1c0b64bdd358 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -240,8 +240,8 @@ impl Run { deny_list: &Option>, boundaries_config: Option<&Spanned>, ) -> Result, Error> { - // If there is no allow list, then we allow all tags - let mut is_allowed = allow_list.is_none(); + // If there is no allow list, then we vacuously have a tag in the allow list + let mut has_tag_in_allowlist = allow_list.is_none(); let dep_tags = boundaries_config .and_then(|b| b.tags.as_ref()) @@ -259,7 +259,7 @@ impl Run { for tag in dep_tags { if let Some(allow_list) = allow_list { if allow_list.contains(tag.as_inner()) { - is_allowed = true; + has_tag_in_allowlist = true; } } @@ -277,7 +277,7 @@ impl Run { } } - if !is_allowed { + if !has_tag_in_allowlist { let (span, text) = dep_tags_span.span_and_text("turbo.json"); let help = span.is_none().then(|| { format!( From 69f392a099fd25f2a82e2244f420dc63f62e5c48 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 5 Feb 2025 11:02:48 -0500 Subject: [PATCH 08/16] Some more testing --- crates/turborepo-lib/src/boundaries/mod.rs | 2 ++ crates/turborepo-lib/src/query/mod.rs | 8 ++++++- crates/turborepo/tests/boundaries.rs | 12 ++++++++++ ...ies_get_boundaries_lints_(npm@10.5.0).snap | 20 ++++++++-------- ...ags_get_boundaries_lints_(npm@10.5.0).snap | 24 +++++++++++++++++++ .../fixtures/boundaries_tags/.gitignore | 3 +++ .../boundaries_tags/apps/my-app/package.json | 11 +++++++++ .../boundaries_tags/apps/my-app/turbo.json | 15 ++++++++++++ .../fixtures/boundaries_tags/package.json | 14 +++++++++++ .../packages/empty-allowlist/package.json | 7 ++++++ .../packages/empty-allowlist/turbo.json | 13 ++++++++++ .../packages/no-allowlist/package.json | 9 +++++++ .../packages/no-allowlist/turbo.json | 18 ++++++++++++++ .../fixtures/boundaries_tags/turbo.json | 1 + 14 files changed, 146 insertions(+), 11 deletions(-) create mode 100644 crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/.gitignore create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/turbo.json diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index a1c0b64bdd358..edb5144d56647 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -250,6 +250,8 @@ impl Run { .flatten() .collect::>(); + // Try our best to find a span for the tags. If the tags exist, we use that + // span, if they don't exist, we use the boundaries config span. let dep_tags_span = boundaries_config .and_then(|b| b.tags.as_ref()) .map(|b| b.to(())) diff --git a/crates/turborepo-lib/src/query/mod.rs b/crates/turborepo-lib/src/query/mod.rs index 34bdf06103037..c7e3278ab2efa 100644 --- a/crates/turborepo-lib/src/query/mod.rs +++ b/crates/turborepo-lib/src/query/mod.rs @@ -15,6 +15,7 @@ use std::{ use async_graphql::{http::GraphiQLSource, *}; use axum::{response, response::IntoResponse}; use external_package::ExternalPackage; +use itertools::Itertools; use package::Package; use package_graph::{Edge, PackageGraph}; pub use server::run_server; @@ -569,7 +570,12 @@ impl RepositoryQuery { /// Check boundaries for all packages. async fn boundaries(&self) -> Result, Error> { match self.run.check_boundaries().await { - Ok(result) => Ok(result.diagnostics.into_iter().map(|b| b.into()).collect()), + Ok(result) => Ok(result + .diagnostics + .into_iter() + .map(|b| b.into()) + .sorted_by(|a: &Diagnostic, b: &Diagnostic| a.message.cmp(&b.message)) + .collect()), Err(err) => Err(Error::Boundaries(err)), } } diff --git a/crates/turborepo/tests/boundaries.rs b/crates/turborepo/tests/boundaries.rs index 2dafaa75d2bd5..465b22cf5e690 100644 --- a/crates/turborepo/tests/boundaries.rs +++ b/crates/turborepo/tests/boundaries.rs @@ -12,6 +12,18 @@ fn test_boundaries() -> Result<(), anyhow::Error> { Ok(()) } +#[test] +fn test_boundaries_tags() -> Result<(), anyhow::Error> { + check_json!( + "boundaries_tags", + "npm@10.5.0", + "query", + "get boundaries lints" => "query { boundaries { items { message import } } }", + ); + + Ok(()) +} + #[test] fn test_boundaries_on_basic_monorepo() -> Result<(), anyhow::Error> { check_json!( diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap index d8e2128d06ff6..c316bd3f40724 100644 --- a/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_get_boundaries_lints_(npm@10.5.0).snap @@ -7,28 +7,28 @@ expression: query_output "boundaries": { "items": [ { - "message": "cannot import file `../../packages/another/index.jsx` because it leaves the package", - "import": "../../packages/another/index.jsx" + "message": "Package `@vercel/unsafe-package` found with tag listed in denylist for `my-app`: `unsafe`", + "import": "@vercel/unsafe-package" }, { - "message": "importing from a type declaration package, but import is not declared as a type-only import", - "import": "ship" + "message": "Package `module-package` found without any tag listed in allowlist for `my-app`", + "import": "module-package" }, { - "message": "importing from a type declaration package, but import is not declared as a type-only import", - "import": "@types/ship" + "message": "cannot import file `../../packages/another/index.jsx` because it leaves the package", + "import": "../../packages/another/index.jsx" }, { "message": "cannot import package `module-package` because it is not a dependency", "import": "module-package" }, { - "message": "Package `module-package` found without any tag listed in allowlist for `my-app`", - "import": "module-package" + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "ship" }, { - "message": "Package `@vercel/unsafe-package` found with tag listed in denylist for `my-app`: `unsafe`", - "import": "@vercel/unsafe-package" + "message": "importing from a type declaration package, but import is not declared as a type-only import", + "import": "@types/ship" } ] } diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap new file mode 100644 index 0000000000000..307ee46e25563 --- /dev/null +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap @@ -0,0 +1,24 @@ +--- +source: crates/turborepo/tests/boundaries.rs +expression: query_output +--- +{ + "data": { + "boundaries": { + "items": [ + { + "message": "Package `@vercel/no-allowlist` found with tag listed in denylist for `my-app`: `unsafe`", + "import": "@vercel/no-allowlist" + }, + { + "message": "Package `@vercel/no-allowlist` found without any tag listed in allowlist for `@vercel/empty-allowlist`", + "import": "@vercel/no-allowlist" + }, + { + "message": "Package `my-app` found without any tag listed in allowlist for `@vercel/empty-allowlist`", + "import": "my-app" + } + ] + } + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/.gitignore b/turborepo-tests/integration/fixtures/boundaries_tags/.gitignore new file mode 100644 index 0000000000000..77af9fc60321d --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/.gitignore @@ -0,0 +1,3 @@ +node_modules/ +.turbo +.npmrc diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json new file mode 100644 index 0000000000000..6c83d0411549a --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json @@ -0,0 +1,11 @@ +{ + "name": "my-app", + "scripts": { + "build": "echo building", + "maybefails": "exit 4" + }, + "dependencies": { + "@vercel/empty-allowlist": "*", + "@vercel/no-allowlist": "*" + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json new file mode 100644 index 0000000000000..58080a56d40f9 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json @@ -0,0 +1,15 @@ +{ + "boundaries": { + "tags": [ + "web" + ], + "dependencies": { + "allow": [ + "types" + ], + "deny": [ + "unsafe" + ] + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/package.json new file mode 100644 index 0000000000000..404d3338e68cf --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/package.json @@ -0,0 +1,14 @@ +{ + "name": "monorepo", + "scripts": { + "something": "turbo run build" + }, + "dependencies": { + "module-package": "*" + }, + "packageManager": "npm@10.5.0", + "workspaces": [ + "apps/*", + "packages/*" + ] +} diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json new file mode 100644 index 0000000000000..18c8c7b191348 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json @@ -0,0 +1,7 @@ +{ + "name": "@vercel/empty-allowlist", + "module": "my-module.mjs", + "dependencies": { + "@vercel/no-allowlist": "*" + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json new file mode 100644 index 0000000000000..b09a30385aaa5 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json @@ -0,0 +1,13 @@ +{ + "boundaries": { + "tags": [ + "types" + ], + "dependencies": { + "allow": [] + }, + "dependents": { + "allow": [] + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json new file mode 100644 index 0000000000000..13b1efafec271 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json @@ -0,0 +1,9 @@ +{ + "name": "@vercel/no-allowlist", + "scripts": { + "dev": "echo building" + }, + "dependencies": { + "utils": "*" + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json new file mode 100644 index 0000000000000..d0e68f7c5b777 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json @@ -0,0 +1,18 @@ +{ + "boundaries": { + "tags": [ + "types", + "unsafe" + ], + "dependencies": { + "deny": [ + "types" + ] + }, + "dependents": { + "deny": [ + "unsafe" + ] + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json new file mode 100644 index 0000000000000..9e26dfeeb6e64 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json @@ -0,0 +1 @@ +{} \ No newline at end of file From 61bf4b714d0e706d180d9f2901660c92227a1f68 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 5 Feb 2025 11:37:55 -0500 Subject: [PATCH 09/16] Fix CI --- crates/turborepo-lib/src/boundaries/mod.rs | 1 + crates/turborepo-lib/src/engine/builder.rs | 68 +++++++++--------- crates/turborepo-lib/src/turbo_json/parser.rs | 70 +++++++++++-------- 3 files changed, 77 insertions(+), 62 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index edb5144d56647..6425b7dcc3077 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -389,6 +389,7 @@ impl Run { /// Either returns a list of errors and number of files checked or a single, /// fatal error + #[allow(clippy::too_many_arguments)] async fn check_package( &self, repo: &Option>, diff --git a/crates/turborepo-lib/src/engine/builder.rs b/crates/turborepo-lib/src/engine/builder.rs index 48900c06d601f..7695f9c7fbd09 100644 --- a/crates/turborepo-lib/src/engine/builder.rs +++ b/crates/turborepo-lib/src/engine/builder.rs @@ -218,7 +218,7 @@ impl<'a> EngineBuilder<'a> { return Ok(Engine::default().seal()); } - let mut turbo_json_loader = self + let turbo_json_loader = self .turbo_json_loader .take() .expect("engine builder cannot be constructed without TurboJsonLoader"); @@ -259,7 +259,7 @@ impl<'a> EngineBuilder<'a> { .task_id() .unwrap_or_else(|| TaskId::new(workspace.as_ref(), task.task())); - if Self::has_task_definition(&mut turbo_json_loader, workspace, task, &task_id)? { + if Self::has_task_definition(turbo_json_loader, workspace, task, &task_id)? { missing_tasks.remove(task.as_inner()); // Even if a task definition was found, we _only_ want to add it as an entry @@ -359,7 +359,7 @@ impl<'a> EngineBuilder<'a> { } let task_definition = self.task_definition( - &mut turbo_json_loader, + turbo_json_loader, &task_id, &task_id.as_non_workspace_task_name(), )?; @@ -853,8 +853,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![ PackageName::from("a"), @@ -910,8 +910,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![PackageName::from("app2")]) .build() @@ -949,8 +949,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("special")))) .with_workspaces(vec![PackageName::from("app1"), PackageName::from("libA")]) .build() @@ -987,8 +987,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(vec![ Spanned::new(TaskName::from("build")), Spanned::new(TaskName::from("test")), @@ -1040,8 +1040,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1083,8 +1083,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![TaskName::from("libA#build"), TaskName::from("build")]) @@ -1118,8 +1118,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1163,8 +1163,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1202,8 +1202,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![ @@ -1249,8 +1249,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("b")]) @@ -1288,8 +1288,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("b")]) @@ -1356,8 +1356,8 @@ mod test { ] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(vec![ Spanned::new(TaskName::from("app1#special")), Spanned::new(TaskName::from("app2#another")), @@ -1398,8 +1398,8 @@ mod test { })] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(Some(Spanned::new(TaskName::from("dev")))) .with_workspaces(vec![PackageName::from("web")]) .build() @@ -1445,8 +1445,8 @@ mod test { ] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader.clone(), false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(vec![Spanned::new(TaskName::from("app1#special"))]) .with_workspaces(vec![PackageName::from("app1")]) .build(); @@ -1458,7 +1458,7 @@ mod test { .unwrap(); assert_json_snapshot!(msg); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(vec![Spanned::new(TaskName::from("app1#another"))]) .with_workspaces(vec![PackageName::from("libA")]) .build(); @@ -1493,8 +1493,8 @@ mod test { )] .into_iter() .collect(); - let loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, loader.clone(), false) + let mut loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) .with_tasks(vec![Spanned::new(TaskName::from("app2#bad-task"))]) .with_workspaces(vec![PackageName::from("app1"), PackageName::from("libA")]) .build(); diff --git a/crates/turborepo-lib/src/turbo_json/parser.rs b/crates/turborepo-lib/src/turbo_json/parser.rs index 87b3e711af50e..f57b60f7e2fde 100644 --- a/crates/turborepo-lib/src/turbo_json/parser.rs +++ b/crates/turborepo-lib/src/turbo_json/parser.rs @@ -105,9 +105,9 @@ impl WithMetadata for RawTurboJson { self.global_env.add_text(text.clone()); self.global_pass_through_env.add_text(text.clone()); self.boundaries.add_text(text.clone()); - self.boundaries - .as_mut() - .map(|boundaries| boundaries.value.add_text(text.clone())); + if let Some(boundaries) = &mut self.boundaries { + boundaries.value.add_text(text.clone()); + } self.tasks.add_text(text.clone()); self.cache_dir.add_text(text.clone()); @@ -121,9 +121,9 @@ impl WithMetadata for RawTurboJson { self.global_env.add_path(path.clone()); self.global_pass_through_env.add_path(path.clone()); self.boundaries.add_path(path.clone()); - self.boundaries - .as_mut() - .map(|boundaries| boundaries.value.add_path(path.clone())); + if let Some(boundaries) = &mut self.boundaries { + boundaries.value.add_path(path.clone()); + } self.tasks.add_path(path.clone()); self.cache_dir.add_path(path.clone()); self.pipeline.add_path(path); @@ -149,48 +149,62 @@ impl WithMetadata for Pipeline { impl WithMetadata for BoundariesConfig { fn add_text(&mut self, text: Arc) { self.tags.add_text(text.clone()); - self.tags - .as_mut() - .map(|tags| tags.value.add_text(text.clone())); + if let Some(tags) = &mut self.tags { + tags.value.add_text(text.clone()); + } + self.dependencies.add_text(text.clone()); - self.dependencies - .as_mut() - .map(|dependencies| dependencies.value.add_text(text.clone())); + if let Some(dependencies) = &mut self.dependencies { + dependencies.value.add_text(text.clone()); + } + self.dependents.add_text(text.clone()); - self.dependents - .as_mut() - .map(|dependents| dependents.value.add_text(text.clone())); + if let Some(dependents) = &mut self.dependents { + dependents.value.add_text(text.clone()); + } } fn add_path(&mut self, path: Arc) { self.tags.add_path(path.clone()); - self.tags - .as_mut() - .map(|tags| tags.value.add_path(path.clone())); + if let Some(tags) = &mut self.tags { + tags.value.add_path(path.clone()); + } + self.dependencies.add_path(path.clone()); - self.dependencies - .as_mut() - .map(|dependencies| dependencies.value.add_path(path.clone())); + if let Some(dependencies) = &mut self.dependencies { + dependencies.value.add_path(path.clone()); + } + self.dependents.add_path(path.clone()); - self.dependents - .as_mut() - .map(|dependents| dependents.value.add_path(path.clone())); + if let Some(dependents) = &mut self.dependents { + dependents.value.add_path(path); + } } } impl WithMetadata for Permissions { fn add_text(&mut self, text: Arc) { self.allow.add_text(text.clone()); - self.allow.as_mut().map(|s| s.value.add_text(text.clone())); + if let Some(allow) = &mut self.allow { + allow.value.add_text(text.clone()); + } + self.deny.add_text(text.clone()); - self.deny.as_mut().map(|s| s.value.add_text(text)); + if let Some(deny) = &mut self.deny { + deny.value.add_text(text.clone()); + } } fn add_path(&mut self, path: Arc) { self.allow.add_path(path.clone()); - self.allow.as_mut().map(|s| s.value.add_path(path.clone())); + if let Some(allow) = &mut self.allow { + allow.value.add_path(path.clone()); + } + self.deny.add_path(path.clone()); - self.deny.as_mut().map(|s| s.value.add_path(path)); + if let Some(deny) = &mut self.deny { + deny.value.add_path(path.clone()); + } } } From 57dcf7b01689320698c64ed4a712c34449716b61 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 6 Feb 2025 14:23:51 -0500 Subject: [PATCH 10/16] Refactoring to make rules general over tags and global --- crates/turborepo-errors/src/lib.rs | 11 +- crates/turborepo-lib/src/boundaries/config.rs | 19 +- .../turborepo-lib/src/boundaries/imports.rs | 193 ++++++++ crates/turborepo-lib/src/boundaries/mod.rs | 425 +++--------------- crates/turborepo-lib/src/boundaries/tags.rs | 200 +++++++++ crates/turborepo-lib/src/query/boundaries.rs | 4 +- crates/turborepo-lib/src/turbo_json/mod.rs | 16 +- crates/turborepo-lib/src/turbo_json/parser.rs | 36 +- 8 files changed, 513 insertions(+), 391 deletions(-) create mode 100644 crates/turborepo-lib/src/boundaries/imports.rs create mode 100644 crates/turborepo-lib/src/boundaries/tags.rs diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index 814799502012d..c0c4806765392 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -87,6 +87,15 @@ impl IntoIterator for Spanned { } } +impl<'a, T> IntoIterator for &'a Spanned { + type Item = &'a T; + type IntoIter = Once<&'a T>; + + fn into_iter(self) -> Self::IntoIter { + iter::once(&self.value) + } +} + impl Deserializable for Spanned { fn deserialize( value: &impl DeserializableValue, @@ -183,7 +192,7 @@ impl Spanned { let path = self.path.as_ref().map_or(default_path, |p| p.as_ref()); match self.range.clone().zip(self.text.as_ref()) { Some((range, text)) => (Some(range.into()), NamedSource::new(path, text.to_string())), - None => (None, NamedSource::new(path, String::new())), + None => (None, NamedSource::new(path, Default::default())), } } diff --git a/crates/turborepo-lib/src/boundaries/config.rs b/crates/turborepo-lib/src/boundaries/config.rs index 88bedf97a20ed..6bf20f0438d04 100644 --- a/crates/turborepo-lib/src/boundaries/config.rs +++ b/crates/turborepo-lib/src/boundaries/config.rs @@ -1,19 +1,24 @@ +use std::collections::HashMap; + use biome_deserialize_macros::Deserializable; use serde::Serialize; use struct_iterable::Iterable; use turborepo_errors::Spanned; #[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct Permissions { - pub allow: Option>>>, - pub deny: Option>>>, +pub struct RootBoundariesConfig { + pub tags: Option>, } +pub type RulesMap = HashMap>; #[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] -#[serde(rename_all = "camelCase")] -pub struct BoundariesConfig { - pub tags: Option>>>, +pub struct Rule { pub dependencies: Option>, pub dependents: Option>, } + +#[derive(Serialize, Default, Debug, Clone, Iterable, Deserializable, PartialEq)] +pub struct Permissions { + pub allow: Option>>>, + pub deny: Option>>>, +} diff --git a/crates/turborepo-lib/src/boundaries/imports.rs b/crates/turborepo-lib/src/boundaries/imports.rs new file mode 100644 index 0000000000000..be3295cae58c9 --- /dev/null +++ b/crates/turborepo-lib/src/boundaries/imports.rs @@ -0,0 +1,193 @@ +use std::collections::{BTreeMap, HashSet}; + +use itertools::Itertools; +use miette::{NamedSource, SourceSpan}; +use oxc_resolver::{ResolveError, Resolver}; +use turbo_trace::ImportType; +use turbopath::{AbsoluteSystemPath, PathRelation, RelativeUnixPath}; +use turborepo_repository::{ + package_graph::{PackageName, PackageNode}, + package_json::PackageJson, +}; + +use crate::{ + boundaries::{BoundariesDiagnostic, Error}, + run::Run, +}; + +impl Run { + pub(crate) fn check_file_import( + &self, + file_path: &AbsoluteSystemPath, + package_path: &AbsoluteSystemPath, + import: &str, + source_span: SourceSpan, + file_content: &str, + ) -> Result, Error> { + let import_path = RelativeUnixPath::new(import)?; + let dir_path = file_path + .parent() + .ok_or_else(|| Error::NoParentDir(file_path.to_owned()))?; + let resolved_import_path = dir_path.join_unix_path(import_path).clean()?; + // We have to check for this case because `relation_to_path` returns `Parent` if + // the paths are equal and there's nothing wrong with importing the + // package you're in. + if resolved_import_path.as_str() == package_path.as_str() { + return Ok(None); + } + // We use `relation_to_path` and not `contains` because `contains` + // panics on invalid paths with too many `..` components + if !matches!( + package_path.relation_to_path(&resolved_import_path), + PathRelation::Parent + ) { + Ok(Some(BoundariesDiagnostic::ImportLeavesPackage { + import: import.to_string(), + span: source_span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + })) + } else { + Ok(None) + } + } + + /// Go through all the possible places a package could be declared to see if + /// it's a valid import. We don't use `oxc_resolver` because there are some + /// cases where you can resolve a package that isn't declared properly. + fn is_dependency( + internal_dependencies: &HashSet<&PackageNode>, + package_json: &PackageJson, + unresolved_external_dependencies: Option<&BTreeMap>, + package_name: &PackageNode, + ) -> bool { + internal_dependencies.contains(&package_name) + || unresolved_external_dependencies.is_some_and(|external_dependencies| { + external_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .dependencies + .as_ref() + .is_some_and(|dependencies| { + dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .dev_dependencies + .as_ref() + .is_some_and(|dev_dependencies| { + dev_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .peer_dependencies + .as_ref() + .is_some_and(|peer_dependencies| { + peer_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + || package_json + .optional_dependencies + .as_ref() + .is_some_and(|optional_dependencies| { + optional_dependencies.contains_key(package_name.as_package_name().as_str()) + }) + } + + fn get_package_name(import: &str) -> String { + if import.starts_with("@") { + import.split('/').take(2).join("/") + } else { + import + .split_once("/") + .map(|(import, _)| import) + .unwrap_or(import) + .to_string() + } + } + + #[allow(clippy::too_many_arguments)] + pub(crate) fn check_package_import( + &self, + import: &str, + import_type: ImportType, + span: SourceSpan, + file_path: &AbsoluteSystemPath, + file_content: &str, + package_json: &PackageJson, + internal_dependencies: &HashSet<&PackageNode>, + unresolved_external_dependencies: Option<&BTreeMap>, + resolver: &Resolver, + ) -> Option { + let package_name = Self::get_package_name(import); + + if package_name.starts_with("@types/") && matches!(import_type, ImportType::Value) { + return Some(BoundariesDiagnostic::NotTypeOnlyImport { + import: import.to_string(), + span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }); + } + let package_name = PackageNode::Workspace(PackageName::Other(package_name)); + let folder = file_path.parent().expect("file_path should have a parent"); + let is_valid_dependency = Self::is_dependency( + internal_dependencies, + package_json, + unresolved_external_dependencies, + &package_name, + ); + + if !is_valid_dependency + && !matches!( + resolver.resolve(folder, import), + Err(ResolveError::Builtin { .. }) + ) + { + // Check the @types package + let types_package_name = PackageNode::Workspace(PackageName::Other(format!( + "@types/{}", + package_name.as_package_name().as_str() + ))); + let is_types_dependency = Self::is_dependency( + internal_dependencies, + package_json, + unresolved_external_dependencies, + &types_package_name, + ); + + if is_types_dependency { + return match import_type { + ImportType::Type => None, + ImportType::Value => Some(BoundariesDiagnostic::NotTypeOnlyImport { + import: import.to_string(), + span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }), + }; + } + + return Some(BoundariesDiagnostic::PackageNotFound { + name: package_name.to_string(), + span, + text: NamedSource::new(file_path.as_str(), file_content.to_string()), + }); + } + + None + } +} + +#[cfg(test)] +mod test { + use test_case::test_case; + + use super::*; + + #[test_case("", ""; "empty")] + #[test_case("ship", "ship"; "basic")] + #[test_case("@types/ship", "@types/ship"; "types")] + #[test_case("@scope/ship", "@scope/ship"; "scoped")] + #[test_case("@scope/foo/bar", "@scope/foo"; "scoped with path")] + #[test_case("foo/bar", "foo"; "regular with path")] + #[test_case("foo/", "foo"; "trailing slash")] + #[test_case("foo/bar/baz", "foo"; "multiple slashes")] + fn test_get_package_name(import: &str, expected: &str) { + assert_eq!(Run::get_package_name(import), expected); + } +} diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 6425b7dcc3077..12d0d2ca35539 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -1,16 +1,16 @@ mod config; +mod imports; +mod tags; use std::{ collections::{BTreeMap, HashMap, HashSet}, sync::{Arc, LazyLock, Mutex}, }; -pub use config::{BoundariesConfig, Permissions}; +pub use config::{Permissions, RootBoundariesConfig, Rule}; use git2::Repository; use globwalk::Settings; -use itertools::Itertools; use miette::{Diagnostic, NamedSource, Report, SourceSpan}; -use oxc_resolver::{ResolveError, Resolver}; use regex::Regex; use swc_common::{ comments::SingleThreadedComments, errors::Handler, input::StringInput, FileName, SourceMap, @@ -20,8 +20,8 @@ use swc_ecma_parser::{lexer::Lexer, Capturing, EsSyntax, Parser, Syntax, TsSynta use swc_ecma_visit::VisitWith; use thiserror::Error; use tracing::log::warn; -use turbo_trace::{ImportFinder, ImportType, Tracer}; -use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathRelation, RelativeUnixPath}; +use turbo_trace::{ImportFinder, Tracer}; +use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; use turborepo_errors::Spanned; use turborepo_repository::{ package_graph::{PackageName, PackageNode}, @@ -29,7 +29,25 @@ use turborepo_repository::{ }; use turborepo_ui::{color, ColorConfig, BOLD_GREEN, BOLD_RED}; -use crate::{run::Run, turbo_json::TurboJson}; +use crate::{boundaries::tags::ProcessedRulesMap, run::Run}; + +#[derive(Clone, Debug, Error, Diagnostic)] +pub enum SecondaryDiagnostic { + #[error("consider adding one of the following tags listed here")] + Allowlist { + #[label] + span: Option, + #[source_code] + text: NamedSource, + }, + #[error("denylist defined here")] + Denylist { + #[label] + span: Option, + #[source_code] + text: NamedSource, + }, +} #[derive(Clone, Debug, Error, Diagnostic)] pub enum BoundariesDiagnostic { @@ -37,7 +55,7 @@ pub enum BoundariesDiagnostic { "Package `{package_name}` found without any tag listed in allowlist for \ `{source_package_name}`" )] - NotAllowedTag { + NoTagInAllowlist { // The package that is declaring the allowlist source_package_name: PackageName, // The package that is either a dependency or dependent of the source package @@ -47,7 +65,9 @@ pub enum BoundariesDiagnostic { #[help] help: Option, #[source_code] - text: Arc, + text: NamedSource, + #[related] + secondary: [SecondaryDiagnostic; 1], }, #[error( "Package `{package_name}` found with tag listed in denylist for `{source_package_name}`: \ @@ -60,7 +80,9 @@ pub enum BoundariesDiagnostic { #[label("tag found here")] span: Option, #[source_code] - text: Arc, + text: NamedSource, + #[related] + secondary: [SecondaryDiagnostic; 1], }, #[error( "importing from a type declaration package, but import is not declared as a type-only \ @@ -166,7 +188,8 @@ impl BoundariesResult { impl Run { pub async fn check_boundaries(&self) -> Result { - let boundaries_configs = self.get_boundaries_configs(); + let package_tags = self.get_package_tags(); + let rules_map = self.get_processed_rules_map(); let packages = self.pkg_dep_graph().packages(); let repo = Repository::discover(self.repo_root()).ok().map(Mutex::new); let mut diagnostics = vec![]; @@ -196,7 +219,8 @@ impl Run { internal_dependencies, unresolved_external_dependencies, &source_map, - &boundaries_configs, + &package_tags, + &rules_map, ) .await?; @@ -213,180 +237,6 @@ impl Run { }) } - fn get_boundaries_configs(&self) -> HashMap> { - let mut boundaries_configs = HashMap::new(); - for (package, _) in self.pkg_dep_graph().packages() { - if let Ok(TurboJson { - boundaries: Some(boundaries_config), - .. - }) = self.turbo_json_loader().uncached_load(package) - { - boundaries_configs.insert(package.clone(), boundaries_config); - } - } - - boundaries_configs - } - - fn is_potential_package_name(import: &str) -> bool { - PACKAGE_NAME_REGEX.is_match(import) - } - - fn validate_relation( - &self, - package_name: &PackageName, - dependency: &PackageName, - allow_list: &Option>, - deny_list: &Option>, - boundaries_config: Option<&Spanned>, - ) -> Result, Error> { - // If there is no allow list, then we vacuously have a tag in the allow list - let mut has_tag_in_allowlist = allow_list.is_none(); - - let dep_tags = boundaries_config - .and_then(|b| b.tags.as_ref()) - .map(|tags| tags.as_inner()) - .into_iter() - .flatten() - .collect::>(); - - // Try our best to find a span for the tags. If the tags exist, we use that - // span, if they don't exist, we use the boundaries config span. - let dep_tags_span = boundaries_config - .and_then(|b| b.tags.as_ref()) - .map(|b| b.to(())) - .or_else(|| boundaries_config.map(|b| b.to(()))) - .unwrap_or_default(); - - for tag in dep_tags { - if let Some(allow_list) = allow_list { - if allow_list.contains(tag.as_inner()) { - has_tag_in_allowlist = true; - } - } - - if let Some(deny_list) = deny_list { - if deny_list.contains(tag.as_inner()) { - let (span, text) = tag.span_and_text("turbo.json"); - return Ok(Some(BoundariesDiagnostic::DeniedTag { - source_package_name: package_name.clone(), - package_name: dependency.clone(), - tag: tag.as_inner().to_string(), - span, - text: Arc::new(text), - })); - } - } - } - - if !has_tag_in_allowlist { - let (span, text) = dep_tags_span.span_and_text("turbo.json"); - let help = span.is_none().then(|| { - format!( - "`{}` doesn't any boundaries config in its `turbo.json` file", - dependency - ) - }); - - return Ok(Some(BoundariesDiagnostic::NotAllowedTag { - source_package_name: package_name.clone(), - package_name: dependency.clone(), - help, - span, - text: Arc::new(text), - })); - } - - Ok(None) - } - - fn check_package_tags( - &self, - pkg: PackageNode, - package_boundaries_config: BoundariesConfig, - boundaries_configs: &HashMap>, - ) -> Result, Error> { - let (dependency_allow_list, dependency_deny_list) = - if let Some(mut dependency_rules) = package_boundaries_config.dependencies { - let allow_rules = dependency_rules.allow.take(); - let deny_rules = dependency_rules.deny.take(); - // If list is `None`, we allow all dependencies. If it's `Some`, we allow only - // the listed dependencies. - let allow_list = allow_rules.map(|allow| { - allow - .into_iter() - .flatten() - .flatten() - .collect::>() - }); - - let deny_list = deny_rules - .map(|deny| deny.into_iter().flatten().flatten().collect::>()); - - (allow_list, deny_list) - } else { - (None, None) - }; - - let (dependent_allow_list, dependent_deny_list) = - if let Some(mut dependent_rules) = package_boundaries_config.dependents { - let allow_rules = dependent_rules.allow.take(); - let deny_rules = dependent_rules.deny.take(); - - // If list is `None`, we allow all dependentes. If it's `Some`, we allow only - // the listed dependentes. - let allow_list = allow_rules.map(|allow| { - allow - .into_iter() - .flatten() - .flatten() - .collect::>() - }); - - let deny_list = deny_rules.map(|deny| { - deny.into_iter() - .flatten() - .flatten() - .collect::>() - }); - - (allow_list, deny_list) - } else { - (None, None) - }; - - let mut diagnostics = Vec::new(); - for dependency in self.pkg_dep_graph().dependencies(&pkg) { - if matches!(dependency, PackageNode::Root) { - continue; - } - - diagnostics.extend(self.validate_relation( - pkg.as_package_name(), - dependency.as_package_name(), - &dependency_allow_list, - &dependency_deny_list, - boundaries_configs.get(dependency.as_package_name()), - )?); - } - - for dependent in self.pkg_dep_graph().ancestors(&pkg) { - if matches!(dependent, PackageNode::Root) { - continue; - } - - diagnostics.extend(self.validate_relation( - pkg.as_package_name(), - dependent.as_package_name(), - &dependent_allow_list, - &dependent_deny_list, - boundaries_configs.get(dependent.as_package_name()), - )?); - } - - Ok(diagnostics) - } - /// Either returns a list of errors and number of files checked or a single, /// fatal error #[allow(clippy::too_many_arguments)] @@ -399,7 +249,8 @@ impl Run { internal_dependencies: HashSet<&PackageNode>, unresolved_external_dependencies: Option<&BTreeMap>, source_map: &SourceMap, - boundaries_configs: &HashMap>, + all_package_tags: &HashMap>>>, + tag_rules: &Option, ) -> Result<(usize, Vec), Error> { let (files_checked, mut diagnostics) = self .check_package_files( @@ -411,18 +262,31 @@ impl Run { source_map, ) .await?; - - if let Some(boundaries_config) = boundaries_configs.get(package_name) { - diagnostics.extend(self.check_package_tags( - PackageNode::Workspace(package_name.clone()), - boundaries_config.as_inner().clone(), - boundaries_configs, - )?); + if let Some(current_package_tags) = all_package_tags.get(package_name) { + if let Some(tag_rules) = tag_rules { + diagnostics.extend(self.check_package_tags( + PackageNode::Workspace(package_name.clone()), + current_package_tags, + all_package_tags, + tag_rules, + )?); + } else { + // NOTE: if we use tags for something other than boundaries, we should remove + // this warning + warn!( + "No boundaries rules found, but package {} has tags", + package_name + ); + } } Ok((files_checked, diagnostics)) } + fn is_potential_package_name(import: &str) -> bool { + PACKAGE_NAME_REGEX.is_match(import) + } + async fn check_package_files( &self, repo: &Option>, @@ -560,179 +424,4 @@ impl Run { Ok((files_checked, diagnostics)) } - - fn check_file_import( - &self, - file_path: &AbsoluteSystemPath, - package_path: &AbsoluteSystemPath, - import: &str, - source_span: SourceSpan, - file_content: &str, - ) -> Result, Error> { - let import_path = RelativeUnixPath::new(import)?; - let dir_path = file_path - .parent() - .ok_or_else(|| Error::NoParentDir(file_path.to_owned()))?; - let resolved_import_path = dir_path.join_unix_path(import_path).clean()?; - // We have to check for this case because `relation_to_path` returns `Parent` if - // the paths are equal and there's nothing wrong with importing the - // package you're in. - if resolved_import_path.as_str() == package_path.as_str() { - return Ok(None); - } - // We use `relation_to_path` and not `contains` because `contains` - // panics on invalid paths with too many `..` components - if !matches!( - package_path.relation_to_path(&resolved_import_path), - PathRelation::Parent - ) { - Ok(Some(BoundariesDiagnostic::ImportLeavesPackage { - import: import.to_string(), - span: source_span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), - })) - } else { - Ok(None) - } - } - - /// Go through all the possible places a package could be declared to see if - /// it's a valid import. We don't use `oxc_resolver` because there are some - /// cases where you can resolve a package that isn't declared properly. - fn is_dependency( - internal_dependencies: &HashSet<&PackageNode>, - package_json: &PackageJson, - unresolved_external_dependencies: Option<&BTreeMap>, - package_name: &PackageNode, - ) -> bool { - internal_dependencies.contains(&package_name) - || unresolved_external_dependencies.is_some_and(|external_dependencies| { - external_dependencies.contains_key(package_name.as_package_name().as_str()) - }) - || package_json - .dependencies - .as_ref() - .is_some_and(|dependencies| { - dependencies.contains_key(package_name.as_package_name().as_str()) - }) - || package_json - .dev_dependencies - .as_ref() - .is_some_and(|dev_dependencies| { - dev_dependencies.contains_key(package_name.as_package_name().as_str()) - }) - || package_json - .peer_dependencies - .as_ref() - .is_some_and(|peer_dependencies| { - peer_dependencies.contains_key(package_name.as_package_name().as_str()) - }) - || package_json - .optional_dependencies - .as_ref() - .is_some_and(|optional_dependencies| { - optional_dependencies.contains_key(package_name.as_package_name().as_str()) - }) - } - - fn get_package_name(import: &str) -> String { - if import.starts_with("@") { - import.split('/').take(2).join("/") - } else { - import - .split_once("/") - .map(|(import, _)| import) - .unwrap_or(import) - .to_string() - } - } - - #[allow(clippy::too_many_arguments)] - fn check_package_import( - &self, - import: &str, - import_type: ImportType, - span: SourceSpan, - file_path: &AbsoluteSystemPath, - file_content: &str, - package_json: &PackageJson, - internal_dependencies: &HashSet<&PackageNode>, - unresolved_external_dependencies: Option<&BTreeMap>, - resolver: &Resolver, - ) -> Option { - let package_name = Self::get_package_name(import); - - if package_name.starts_with("@types/") && matches!(import_type, ImportType::Value) { - return Some(BoundariesDiagnostic::NotTypeOnlyImport { - import: import.to_string(), - span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), - }); - } - let package_name = PackageNode::Workspace(PackageName::Other(package_name)); - let folder = file_path.parent().expect("file_path should have a parent"); - let is_valid_dependency = Self::is_dependency( - internal_dependencies, - package_json, - unresolved_external_dependencies, - &package_name, - ); - - if !is_valid_dependency - && !matches!( - resolver.resolve(folder, import), - Err(ResolveError::Builtin { .. }) - ) - { - // Check the @types package - let types_package_name = PackageNode::Workspace(PackageName::Other(format!( - "@types/{}", - package_name.as_package_name().as_str() - ))); - let is_types_dependency = Self::is_dependency( - internal_dependencies, - package_json, - unresolved_external_dependencies, - &types_package_name, - ); - - if is_types_dependency { - return match import_type { - ImportType::Type => None, - ImportType::Value => Some(BoundariesDiagnostic::NotTypeOnlyImport { - import: import.to_string(), - span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), - }), - }; - } - - return Some(BoundariesDiagnostic::PackageNotFound { - name: package_name.to_string(), - span, - text: NamedSource::new(file_path.as_str(), file_content.to_string()), - }); - } - - None - } -} - -#[cfg(test)] -mod test { - use test_case::test_case; - - use super::*; - - #[test_case("", ""; "empty")] - #[test_case("ship", "ship"; "basic")] - #[test_case("@types/ship", "@types/ship"; "types")] - #[test_case("@scope/ship", "@scope/ship"; "scoped")] - #[test_case("@scope/foo/bar", "@scope/foo"; "scoped with path")] - #[test_case("foo/bar", "foo"; "regular with path")] - #[test_case("foo/", "foo"; "trailing slash")] - #[test_case("foo/bar/baz", "foo"; "multiple slashes")] - fn test_get_package_name(import: &str, expected: &str) { - assert_eq!(Run::get_package_name(import), expected); - } } diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs new file mode 100644 index 0000000000000..83ae81212f6f0 --- /dev/null +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -0,0 +1,200 @@ +use std::{ + collections::{HashMap, HashSet}, +}; + +use turborepo_errors::Spanned; +use turborepo_repository::package_graph::{PackageName, PackageNode}; + +use crate::{ + boundaries::{config::Rule, BoundariesDiagnostic, Error, Permissions, SecondaryDiagnostic}, + run::Run, + turbo_json::TurboJson, +}; + +pub type ProcessedRulesMap = HashMap; + +pub struct ProcessedRule { + dependencies: Option, + dependents: Option, +} + +impl From for ProcessedRule { + fn from(rule: Rule) -> Self { + Self { + dependencies: rule + .dependencies + .map(|dependencies| dependencies.into_inner().into()), + dependents: rule + .dependents + .map(|dependents| dependents.into_inner().into()), + } + } +} + +pub struct ProcessedPermissions { + allow: Option>>, + deny: Option>>, +} + +impl From for ProcessedPermissions { + fn from(permissions: Permissions) -> Self { + Self { + allow: permissions + .allow + .map(|allow| allow.map(|allow| allow.into_iter().flatten().collect())), + deny: permissions + .deny + .map(|deny| deny.map(|deny| deny.into_iter().flatten().collect())), + } + } +} + +impl Run { + pub(crate) fn get_package_tags(&self) -> HashMap>>> { + let mut package_tags = HashMap::new(); + for (package, _) in self.pkg_dep_graph().packages() { + if let Ok(TurboJson { + tags: Some(tags), .. + }) = self.turbo_json_loader().uncached_load(package) + { + package_tags.insert(package.clone(), tags); + } + } + + package_tags + } + + pub(crate) fn get_processed_rules_map(&self) -> Option { + self.root_turbo_json() + .boundaries + .as_ref() + .and_then(|boundaries| boundaries.tags.as_ref()) + .map(|tags| { + tags.as_inner() + .into_iter() + .map(|(k, v)| (k.clone(), v.as_inner().clone().into())) + .collect() + }) + } + + /// Loops through the tags of a package that is related to `package_name` + /// (i.e. either a dependency or a dependent) and checks if the tag is + /// allowed or denied by the rules in `allow_list` and `deny_list`. + fn validate_relation( + &self, + package_name: &PackageName, + relation_package_name: &PackageName, + tags: Option<&Spanned>>>, + allow_list: Option<&Spanned>>, + deny_list: Option<&Spanned>>, + ) -> Result, Error> { + // If there is no allow list, then we vacuously have a tag in the allow list + let mut has_tag_in_allowlist = allow_list.is_none(); + let tags_span = tags.map(|tags| tags.to(())).unwrap_or_default(); + + for tag in tags.into_iter().flatten().flatten() { + if let Some(allow_list) = allow_list { + if allow_list.contains(tag.as_inner()) { + has_tag_in_allowlist = true; + } + } + + if let Some(deny_list) = deny_list { + if deny_list.contains(tag.as_inner()) { + let (span, text) = tag.span_and_text("turbo.json"); + let deny_list_spanned = deny_list.to(()); + let (deny_list_span, deny_list_text) = + deny_list_spanned.span_and_text("turbo.json"); + + return Ok(Some(BoundariesDiagnostic::DeniedTag { + source_package_name: package_name.clone(), + package_name: relation_package_name.clone(), + tag: tag.as_inner().to_string(), + span, + text, + secondary: [SecondaryDiagnostic::Denylist { + span: deny_list_span, + text: deny_list_text, + }], + })); + } + } + } + + if !has_tag_in_allowlist { + let (span, text) = tags_span.span_and_text("turbo.json"); + let help = span.is_none().then(|| { + format!( + "`{}` doesn't any tags defined in its `turbo.json` file", + relation_package_name + ) + }); + + let allow_list_spanned = allow_list + .map(|allow_list| allow_list.to(())) + .unwrap_or_default(); + let (allow_list_span, allow_list_text) = allow_list_spanned.span_and_text("turbo.json"); + + return Ok(Some(BoundariesDiagnostic::NoTagInAllowlist { + source_package_name: package_name.clone(), + package_name: relation_package_name.clone(), + help, + span, + text, + secondary: [SecondaryDiagnostic::Allowlist { + span: allow_list_span, + text: allow_list_text, + }], + })); + } + + Ok(None) + } + + pub(crate) fn check_package_tags( + &self, + pkg: PackageNode, + current_package_tags: &Spanned>>, + all_package_tags: &HashMap>>>, + tags_rules: &ProcessedRulesMap, + ) -> Result, Error> { + let mut diagnostics = Vec::new(); + for tag in current_package_tags.iter() { + if let Some(rule) = tags_rules.get(tag.as_inner()) { + if let Some(dependency_permissions) = &rule.dependencies { + for dependency in self.pkg_dep_graph().dependencies(&pkg) { + if matches!(dependency, PackageNode::Root) { + continue; + } + let dependency_tags = all_package_tags.get(dependency.as_package_name()); + diagnostics.extend(self.validate_relation( + pkg.as_package_name(), + dependency.as_package_name(), + dependency_tags, + dependency_permissions.allow.as_ref(), + dependency_permissions.deny.as_ref(), + )?); + } + } + + if let Some(dependent_permissions) = &rule.dependents { + for dependent in self.pkg_dep_graph().ancestors(&pkg) { + if matches!(dependent, PackageNode::Root) { + continue; + } + let dependent_tags = all_package_tags.get(dependent.as_package_name()); + diagnostics.extend(self.validate_relation( + pkg.as_package_name(), + dependent.as_package_name(), + dependent_tags, + dependent_permissions.allow.as_ref(), + dependent_permissions.deny.as_ref(), + )?) + } + } + } + } + + Ok(diagnostics) + } +} diff --git a/crates/turborepo-lib/src/query/boundaries.rs b/crates/turborepo-lib/src/query/boundaries.rs index 526644b6d5f18..675161a0336e2 100644 --- a/crates/turborepo-lib/src/query/boundaries.rs +++ b/crates/turborepo-lib/src/query/boundaries.rs @@ -38,9 +38,10 @@ impl From for Diagnostic { reason: None, }, - BoundariesDiagnostic::NotAllowedTag { + BoundariesDiagnostic::NoTagInAllowlist { source_package_name: _, help: _, + secondary: _, package_name, span, text, @@ -54,6 +55,7 @@ impl From for Diagnostic { }, BoundariesDiagnostic::DeniedTag { source_package_name: _, + secondary: _, package_name, tag, span, diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index 2bcacdb235c49..5cabbcba262fd 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -31,8 +31,7 @@ pub mod parser; pub use loader::TurboJsonLoader; -use crate::config::UnnecessaryPackageTaskSyntaxError; -use crate::boundaries::BoundariesConfig; +use crate::{boundaries::RootBoundariesConfig, config::UnnecessaryPackageTaskSyntaxError}; #[derive(Serialize, Deserialize, Debug, Default, PartialEq, Clone, Deserializable)] #[serde(rename_all = "camelCase")] @@ -54,7 +53,8 @@ pub struct SpacesJson { pub struct TurboJson { text: Option>, path: Option>, - pub(crate) boundaries: Option>, + pub(crate) tags: Option>>>, + pub(crate) boundaries: Option>, pub(crate) extends: Spanned>, pub(crate) global_deps: Vec, pub(crate) global_env: Vec, @@ -149,7 +149,10 @@ pub struct RawTurboJson { pub cache_dir: Option>, #[serde(skip_serializing_if = "Option::is_none")] - pub boundaries: Option>, + pub tags: Option>>>, + + #[serde(skip_serializing_if = "Option::is_none")] + pub boundaries: Option>, #[deserializable(rename = "//")] #[serde(skip)] @@ -570,6 +573,7 @@ impl TryFrom for TurboJson { Ok(TurboJson { text: raw_turbo.span.text, path: raw_turbo.span.path, + tags: raw_turbo.tags, global_env: { let mut global_env: Vec<_> = global_env.into_iter().collect(); global_env.sort(); @@ -770,7 +774,7 @@ mod tests { use super::{RawTurboJson, Spanned, TurboJson, UIMode}; use crate::{ - boundaries::BoundariesConfig, + boundaries::RootBoundariesConfig, cli::OutputLogsMode, run::task_id::TaskName, task_graph::{TaskDefinition, TaskOutputs}, @@ -795,7 +799,7 @@ mod tests { JsonParserOptions::default().with_allow_comments(), "turbo.json", ); - let raw_task_definition: BoundariesConfig = + let raw_task_definition: RootBoundariesConfig = deserialized_result.into_deserialized().unwrap(); insta::assert_json_snapshot!(name.replace(' ', "_"), raw_task_definition); } diff --git a/crates/turborepo-lib/src/turbo_json/parser.rs b/crates/turborepo-lib/src/turbo_json/parser.rs index f57b60f7e2fde..3f71020964697 100644 --- a/crates/turborepo-lib/src/turbo_json/parser.rs +++ b/crates/turborepo-lib/src/turbo_json/parser.rs @@ -15,7 +15,7 @@ use turborepo_errors::{ParseDiagnostic, WithMetadata}; use turborepo_unescape::UnescapedString; use crate::{ - boundaries::{BoundariesConfig, Permissions}, + boundaries::{Permissions, RootBoundariesConfig, Rule}, run::task_id::TaskName, turbo_json::{Pipeline, RawTaskDefinition, RawTurboJson, Spanned}, }; @@ -101,6 +101,10 @@ impl WithMetadata for RawTurboJson { fn add_text(&mut self, text: Arc) { self.span.add_text(text.clone()); self.extends.add_text(text.clone()); + self.tags.add_text(text.clone()); + if let Some(tags) = &mut self.tags { + tags.value.add_text(text.clone()); + } self.global_dependencies.add_text(text.clone()); self.global_env.add_text(text.clone()); self.global_pass_through_env.add_text(text.clone()); @@ -117,6 +121,10 @@ impl WithMetadata for RawTurboJson { fn add_path(&mut self, path: Arc) { self.span.add_path(path.clone()); self.extends.add_path(path.clone()); + self.tags.add_path(path.clone()); + if let Some(tags) = &mut self.tags { + tags.value.add_path(path.clone()); + } self.global_dependencies.add_path(path.clone()); self.global_env.add_path(path.clone()); self.global_pass_through_env.add_path(path.clone()); @@ -146,13 +154,30 @@ impl WithMetadata for Pipeline { } } -impl WithMetadata for BoundariesConfig { +impl WithMetadata for RootBoundariesConfig { fn add_text(&mut self, text: Arc) { self.tags.add_text(text.clone()); if let Some(tags) = &mut self.tags { - tags.value.add_text(text.clone()); + for rule in tags.as_inner_mut().values_mut() { + rule.add_text(text.clone()); + rule.value.add_text(text.clone()); + } } + } + fn add_path(&mut self, path: Arc) { + self.tags.add_path(path.clone()); + if let Some(tags) = &mut self.tags { + for rule in tags.as_inner_mut().values_mut() { + rule.add_path(path.clone()); + rule.value.add_path(path.clone()); + } + } + } +} + +impl WithMetadata for Rule { + fn add_text(&mut self, text: Arc) { self.dependencies.add_text(text.clone()); if let Some(dependencies) = &mut self.dependencies { dependencies.value.add_text(text.clone()); @@ -165,11 +190,6 @@ impl WithMetadata for BoundariesConfig { } fn add_path(&mut self, path: Arc) { - self.tags.add_path(path.clone()); - if let Some(tags) = &mut self.tags { - tags.value.add_path(path.clone()); - } - self.dependencies.add_path(path.clone()); if let Some(dependencies) = &mut self.dependencies { dependencies.value.add_path(path.clone()); From 0e1b33c1b6da1c9dd42c21e50fc6daa1f831046e Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 7 Feb 2025 12:50:40 -0500 Subject: [PATCH 11/16] Add configuration for nested related errors --- crates/turborepo-lib/src/boundaries/tags.rs | 4 +--- crates/turborepo-lib/src/shim/mod.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs index 83ae81212f6f0..c6e36c38361e1 100644 --- a/crates/turborepo-lib/src/boundaries/tags.rs +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -1,6 +1,4 @@ -use std::{ - collections::{HashMap, HashSet}, -}; +use std::collections::{HashMap, HashSet}; use turborepo_errors::Spanned; use turborepo_repository::package_graph::{PackageName, PackageNode}; diff --git a/crates/turborepo-lib/src/shim/mod.rs b/crates/turborepo-lib/src/shim/mod.rs index 570601ffd5406..6ec56fff02f4c 100644 --- a/crates/turborepo-lib/src/shim/mod.rs +++ b/crates/turborepo-lib/src/shim/mod.rs @@ -288,11 +288,20 @@ pub fn run() -> Result { let _ = miette::set_hook(Box::new(|_| { Box::new( miette::MietteHandlerOpts::new() + .show_related_errors_as_nested() .color(false) .unicode(false) .build(), ) })); + } else { + let _ = miette::set_hook(Box::new(|_| { + Box::new( + miette::MietteHandlerOpts::new() + .show_related_errors_as_nested() + .build(), + ) + })); } let subscriber = TurboSubscriber::new_with_verbosity(args.verbosity, &color_config); From 3688f5a5ac022412d75748a1889bc938ded694b6 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 7 Feb 2025 13:18:54 -0500 Subject: [PATCH 12/16] Some refactoring to reduce arguments and satisfy clippy --- crates/turborepo-lib/src/boundaries/mod.rs | 58 +++++++-------------- crates/turborepo-lib/src/boundaries/tags.rs | 2 +- 2 files changed, 21 insertions(+), 39 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/mod.rs b/crates/turborepo-lib/src/boundaries/mod.rs index 12d0d2ca35539..dca94afb477bc 100644 --- a/crates/turborepo-lib/src/boundaries/mod.rs +++ b/crates/turborepo-lib/src/boundaries/mod.rs @@ -3,7 +3,7 @@ mod imports; mod tags; use std::{ - collections::{BTreeMap, HashMap, HashSet}, + collections::{HashMap, HashSet}, sync::{Arc, LazyLock, Mutex}, }; @@ -21,12 +21,9 @@ use swc_ecma_visit::VisitWith; use thiserror::Error; use tracing::log::warn; use turbo_trace::{ImportFinder, Tracer}; -use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf}; +use turbopath::AbsoluteSystemPathBuf; use turborepo_errors::Spanned; -use turborepo_repository::{ - package_graph::{PackageName, PackageNode}, - package_json::PackageJson, -}; +use turborepo_repository::package_graph::{PackageInfo, PackageName, PackageNode}; use turborepo_ui::{color, ColorConfig, BOLD_GREEN, BOLD_RED}; use crate::{boundaries::tags::ProcessedRulesMap, run::Run}; @@ -202,22 +199,11 @@ impl Run { continue; } - let package_root = self.repo_root().resolve(package_info.package_path()); - let internal_dependencies = self - .pkg_dep_graph() - .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) - .unwrap_or_default(); - let unresolved_external_dependencies = - package_info.unresolved_external_dependencies.as_ref(); - let (files_checked, package_diagnostics) = self .check_package( &repo, - &package_root, package_name, - &package_info.package_json, - internal_dependencies, - unresolved_external_dependencies, + package_info, &source_map, &package_tags, &rules_map, @@ -239,29 +225,19 @@ impl Run { /// Either returns a list of errors and number of files checked or a single, /// fatal error - #[allow(clippy::too_many_arguments)] async fn check_package( &self, repo: &Option>, - package_root: &AbsoluteSystemPath, package_name: &PackageName, - package_json: &PackageJson, - internal_dependencies: HashSet<&PackageNode>, - unresolved_external_dependencies: Option<&BTreeMap>, + package_info: &PackageInfo, source_map: &SourceMap, all_package_tags: &HashMap>>>, tag_rules: &Option, ) -> Result<(usize, Vec), Error> { let (files_checked, mut diagnostics) = self - .check_package_files( - repo, - package_root, - package_json, - internal_dependencies, - unresolved_external_dependencies, - source_map, - ) + .check_package_files(repo, package_name, package_info, source_map) .await?; + if let Some(current_package_tags) = all_package_tags.get(package_name) { if let Some(tag_rules) = tag_rules { diagnostics.extend(self.check_package_tags( @@ -290,14 +266,20 @@ impl Run { async fn check_package_files( &self, repo: &Option>, - package_root: &AbsoluteSystemPath, - package_json: &PackageJson, - internal_dependencies: HashSet<&PackageNode>, - unresolved_external_dependencies: Option<&BTreeMap>, + package_name: &PackageName, + package_info: &PackageInfo, source_map: &SourceMap, ) -> Result<(usize, Vec), Error> { + let package_root = self.repo_root().resolve(package_info.package_path()); + let internal_dependencies = self + .pkg_dep_graph() + .immediate_dependencies(&PackageNode::Workspace(package_name.to_owned())) + .unwrap_or_default(); + let unresolved_external_dependencies = + package_info.unresolved_external_dependencies.as_ref(); + let files = globwalk::globwalk_with_settings( - package_root, + &package_root, &[ "**/*.js".parse().unwrap(), "**/*.jsx".parse().unwrap(), @@ -389,7 +371,7 @@ impl Run { // We have a file import let check_result = if import.starts_with(".") { - self.check_file_import(&file_path, package_root, import, span, &file_content)? + self.check_file_import(&file_path, &package_root, import, span, &file_content)? } else if Self::is_potential_package_name(import) { self.check_package_import( import, @@ -397,7 +379,7 @@ impl Run { span, &file_path, &file_content, - package_json, + &package_info.package_json, &internal_dependencies, unresolved_external_dependencies, &resolver, diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs index c6e36c38361e1..36abd672ffc15 100644 --- a/crates/turborepo-lib/src/boundaries/tags.rs +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -69,7 +69,7 @@ impl Run { .and_then(|boundaries| boundaries.tags.as_ref()) .map(|tags| { tags.as_inner() - .into_iter() + .iter() .map(|(k, v)| (k.clone(), v.as_inner().clone().into())) .collect() }) From b5e5c51e53b1829a3218f8be21fb93278035ec4c Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 7 Feb 2025 13:37:42 -0500 Subject: [PATCH 13/16] Update tests --- crates/turborepo-lib/src/turbo_json/mod.rs | 40 ++++++++-- ...rborepo_lib__turbo_json__tests__empty.snap | 8 +- ...po_lib__turbo_json__tests__empty_tags.snap | 7 ++ ...orepo_lib__turbo_json__tests__one_tag.snap | 7 ++ ...bo_json__tests__tags_and_dependencies.snap | 21 +++--- ..._json__tests__tags_and_dependencies_2.snap | 25 ++++--- ...urbo_json__tests__tags_and_dependents.snap | 23 +++--- ...repo_lib__turbo_json__tests__two_tags.snap | 8 ++ ...ags_get_boundaries_lints_(npm@10.5.0).snap | 16 ++-- .../boundaries/apps/my-app/turbo.json | 16 +--- .../boundaries/packages/ship-types/turbo.json | 8 +- .../packages/unsafe-package/turbo.json | 8 +- .../fixtures/boundaries/turbo.json | 17 ++++- .../boundaries_tags/apps/my-app/package.json | 6 +- .../boundaries_tags/apps/my-app/turbo.json | 16 +--- .../package.json | 2 +- .../allowed-and-denied-tag/turbo.json | 6 ++ .../package.json | 2 +- .../packages/allowed-tag/turbo.json | 5 ++ .../packages/empty-allowlist/turbo.json | 13 ---- .../packages/no-allowlist/turbo.json | 18 ----- .../not-allowed-dependent/package.json | 6 ++ .../packages/not-allowed-dependent/turbo.json | 3 + .../fixtures/boundaries_tags/turbo.json | 27 ++++++- .../integration/tests/bad-turbo-json.t | 75 +++++++++---------- .../integration/tests/dry-json/monorepo.t | 4 +- .../integration/tests/invalid-package-json.t | 13 ++-- .../persistent-dependencies/1-topological.t | 18 ++--- .../persistent-dependencies/10-too-many.t | 12 +-- .../2-same-workspace.t | 18 ++--- .../3-workspace-specific.t | 36 ++++----- .../4-cross-workspace.t | 18 ++--- .../5-root-workspace.t | 18 ++--- .../7-topological-nested.t | 18 ++--- .../8-topological-with-extra.t | 18 ++--- .../9-cross-workspace-nested.t | 18 ++--- .../integration/tests/query/validation.t | 6 +- .../integration/tests/run/missing-tasks.t | 16 +--- .../tests/workspace-configs/persistent.t | 60 +++++++-------- 39 files changed, 334 insertions(+), 322 deletions(-) create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__one_tag.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__two_tags.snap rename turborepo-tests/integration/fixtures/boundaries_tags/packages/{no-allowlist => allowed-and-denied-tag}/package.json (67%) create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/turbo.json rename turborepo-tests/integration/fixtures/boundaries_tags/packages/{empty-allowlist => allowed-tag}/package.json (70%) create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/turbo.json delete mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json delete mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/package.json create mode 100644 turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/turbo.json diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index 5cabbcba262fd..ed7d89bd93d9e 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -782,16 +782,36 @@ mod tests { }; #[test_case("{}", "empty")] - #[test_case(r#"{"tags": ["my-tag"] }"#, "just tags")] + #[test_case(r#"{"tags": {} }"#, "empty tags")] #[test_case( - r#"{"tags": ["my-tag"], "dependencies": { "allow": ["my-package"] } }"#, + r#"{"tags": { "my-tag": { "dependencies": { "allow": ["my-package"] } } } }"#, "tags and dependencies" )] - #[test_case(r#"{"tags": ["my-tag"], "dependencies": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, "tags and dependencies 2")] - #[test_case(r#"{"tags": ["my-tag"], "dependents": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, "tags and dependents")] #[test_case( - r#"{ "dependents": { "allow": ["my-package"], "deny": ["my-other-package"] } }"#, - "dependents" + r#"{ + "tags": { + "my-tag": { + "dependencies": { + "allow": ["my-package"], + "deny": ["my-other-package"] + } + } + } + }"#, + "tags and dependencies 2" + )] + #[test_case( + r#"{ + "tags": { + "my-tag": { + "dependents": { + "allow": ["my-package"], + "deny": ["my-other-package"] + } + } + } + }"#, + "tags and dependents" )] fn test_deserialize_boundaries(json: &str, name: &str) { let deserialized_result = deserialize_from_json_str( @@ -1025,6 +1045,14 @@ mod tests { assert_eq!(actual, expected); } + #[test_case(r#"{ "tags": [] }"#, "empty")] + #[test_case(r#"{ "tags": ["my-tag"] }"#, "one tag")] + #[test_case(r#"{ "tags": ["my-tag", "my-other-tag"] }"#, "two tags")] + fn test_tags(json: &str, name: &str) { + let json = RawTurboJson::parse(json, "").unwrap(); + insta::assert_json_snapshot!(name.replace(' ', "_"), json.tags); + } + #[test_case(r#"{ "ui": "tui" }"#, Some(UIMode::Tui) ; "tui")] #[test_case(r#"{ "ui": "stream" }"#, Some(UIMode::Stream) ; "stream")] #[test_case(r#"{}"#, None ; "missing")] diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap index 36fa44c506a17..1497dfca7fe29 100644 --- a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap @@ -1,9 +1,5 @@ --- source: crates/turborepo-lib/src/turbo_json/mod.rs -expression: raw_task_definition +expression: json.tags --- -{ - "tags": null, - "dependencies": null, - "dependents": null -} +[] diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags.snap new file mode 100644 index 0000000000000..a496ebdb0a319 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags.snap @@ -0,0 +1,7 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": {} +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__one_tag.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__one_tag.snap new file mode 100644 index 0000000000000..a1a184628aafd --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__one_tag.snap @@ -0,0 +1,7 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: json.tags +--- +[ + "my-tag" +] diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap index eb34fd32a32c3..671335b532098 100644 --- a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies.snap @@ -3,14 +3,15 @@ source: crates/turborepo-lib/src/turbo_json/mod.rs expression: raw_task_definition --- { - "tags": [ - "my-tag" - ], - "dependencies": { - "allow": [ - "my-package" - ], - "deny": null - }, - "dependents": null + "tags": { + "my-tag": { + "dependencies": { + "allow": [ + "my-package" + ], + "deny": null + }, + "dependents": null + } + } } diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap index 4b958821b3ee3..28c4ef872b5cc 100644 --- a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependencies_2.snap @@ -3,16 +3,17 @@ source: crates/turborepo-lib/src/turbo_json/mod.rs expression: raw_task_definition --- { - "tags": [ - "my-tag" - ], - "dependencies": { - "allow": [ - "my-package" - ], - "deny": [ - "my-other-package" - ] - }, - "dependents": null + "tags": { + "my-tag": { + "dependencies": { + "allow": [ + "my-package" + ], + "deny": [ + "my-other-package" + ] + }, + "dependents": null + } + } } diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap index 685df5bd029b7..4c0600b860a5c 100644 --- a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__tags_and_dependents.snap @@ -3,16 +3,17 @@ source: crates/turborepo-lib/src/turbo_json/mod.rs expression: raw_task_definition --- { - "tags": [ - "my-tag" - ], - "dependencies": null, - "dependents": { - "allow": [ - "my-package" - ], - "deny": [ - "my-other-package" - ] + "tags": { + "my-tag": { + "dependencies": null, + "dependents": { + "allow": [ + "my-package" + ], + "deny": [ + "my-other-package" + ] + } + } } } diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__two_tags.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__two_tags.snap new file mode 100644 index 0000000000000..f105c209a7739 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__two_tags.snap @@ -0,0 +1,8 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: json.tags +--- +[ + "my-tag", + "my-other-tag" +] diff --git a/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap index 307ee46e25563..1819338c8d9dc 100644 --- a/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap +++ b/crates/turborepo/tests/snapshots/boundaries__boundaries_tags_get_boundaries_lints_(npm@10.5.0).snap @@ -7,16 +7,20 @@ expression: query_output "boundaries": { "items": [ { - "message": "Package `@vercel/no-allowlist` found with tag listed in denylist for `my-app`: `unsafe`", - "import": "@vercel/no-allowlist" + "message": "Package `@vercel/allowed-and-denied-tag` found with tag listed in denylist for `@vercel/my-app`: `unsafe`", + "import": "@vercel/allowed-and-denied-tag" }, { - "message": "Package `@vercel/no-allowlist` found without any tag listed in allowlist for `@vercel/empty-allowlist`", - "import": "@vercel/no-allowlist" + "message": "Package `@vercel/not-allowed-dependent` found without any tag listed in allowlist for `@vercel/allowed-and-denied-tag`", + "import": "@vercel/not-allowed-dependent" }, { - "message": "Package `my-app` found without any tag listed in allowlist for `@vercel/empty-allowlist`", - "import": "my-app" + "message": "Package `@vercel/not-allowed-dependent` found without any tag listed in allowlist for `@vercel/allowed-tag`", + "import": "@vercel/not-allowed-dependent" + }, + { + "message": "Package `@vercel/not-allowed-dependent` found without any tag listed in allowlist for `@vercel/my-app`", + "import": "@vercel/not-allowed-dependent" } ] } diff --git a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json index 58080a56d40f9..4748eae5438bf 100644 --- a/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries/apps/my-app/turbo.json @@ -1,15 +1,5 @@ { - "boundaries": { - "tags": [ - "web" - ], - "dependencies": { - "allow": [ - "types" - ], - "deny": [ - "unsafe" - ] - } - } + "tags": [ + "web" + ] } \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json index fe98316624e4f..04851af8d9678 100644 --- a/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries/packages/ship-types/turbo.json @@ -1,7 +1,5 @@ { - "boundaries": { - "tags": [ - "types" - ] - } + "tags": [ + "types" + ] } \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json index d2613b5cd9815..b1eb6db55ce48 100644 --- a/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries/packages/unsafe-package/turbo.json @@ -1,7 +1,5 @@ { - "boundaries": { - "tags": [ - "unsafe" - ] - } + "tags": [ + "unsafe" + ] } \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries/turbo.json b/turborepo-tests/integration/fixtures/boundaries/turbo.json index 9e26dfeeb6e64..a5d3e78857a45 100644 --- a/turborepo-tests/integration/fixtures/boundaries/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries/turbo.json @@ -1 +1,16 @@ -{} \ No newline at end of file +{ + "boundaries": { + "tags": { + "web": { + "dependencies": { + "allow": [ + "types" + ], + "deny": [ + "unsafe" + ] + } + } + } + } +} diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json index 6c83d0411549a..c0989cf724c93 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/package.json @@ -1,11 +1,11 @@ { - "name": "my-app", + "name": "@vercel/my-app", "scripts": { "build": "echo building", "maybefails": "exit 4" }, "dependencies": { - "@vercel/empty-allowlist": "*", - "@vercel/no-allowlist": "*" + "@vercel/allowed-tag": "*", + "@vercel/allowed-and-denied-tag": "*" } } diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json index 58080a56d40f9..4748eae5438bf 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/apps/my-app/turbo.json @@ -1,15 +1,5 @@ { - "boundaries": { - "tags": [ - "web" - ], - "dependencies": { - "allow": [ - "types" - ], - "deny": [ - "unsafe" - ] - } - } + "tags": [ + "web" + ] } \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/package.json similarity index 67% rename from turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json rename to turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/package.json index 13b1efafec271..57c9c9dca9557 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/package.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/package.json @@ -1,5 +1,5 @@ { - "name": "@vercel/no-allowlist", + "name": "@vercel/allowed-and-denied-tag", "scripts": { "dev": "echo building" }, diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/turbo.json new file mode 100644 index 0000000000000..bb0d0284ae360 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-and-denied-tag/turbo.json @@ -0,0 +1,6 @@ +{ + "tags": [ + "types", + "unsafe" + ] +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json similarity index 70% rename from turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json rename to turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json index 18c8c7b191348..dc8c1450a92d4 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/package.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/package.json @@ -1,5 +1,5 @@ { - "name": "@vercel/empty-allowlist", + "name": "@vercel/allowed-tag", "module": "my-module.mjs", "dependencies": { "@vercel/no-allowlist": "*" diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/turbo.json new file mode 100644 index 0000000000000..04851af8d9678 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/allowed-tag/turbo.json @@ -0,0 +1,5 @@ +{ + "tags": [ + "types" + ] +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json deleted file mode 100644 index b09a30385aaa5..0000000000000 --- a/turborepo-tests/integration/fixtures/boundaries_tags/packages/empty-allowlist/turbo.json +++ /dev/null @@ -1,13 +0,0 @@ -{ - "boundaries": { - "tags": [ - "types" - ], - "dependencies": { - "allow": [] - }, - "dependents": { - "allow": [] - } - } -} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json deleted file mode 100644 index d0e68f7c5b777..0000000000000 --- a/turborepo-tests/integration/fixtures/boundaries_tags/packages/no-allowlist/turbo.json +++ /dev/null @@ -1,18 +0,0 @@ -{ - "boundaries": { - "tags": [ - "types", - "unsafe" - ], - "dependencies": { - "deny": [ - "types" - ] - }, - "dependents": { - "deny": [ - "unsafe" - ] - } - } -} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/package.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/package.json new file mode 100644 index 0000000000000..30e85bfb5cd79 --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/package.json @@ -0,0 +1,6 @@ +{ + "name": "@vercel/not-allowed-dependent", + "dependencies": { + "@vercel/my-app": "*" + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/turbo.json new file mode 100644 index 0000000000000..eb25b1905080a --- /dev/null +++ b/turborepo-tests/integration/fixtures/boundaries_tags/packages/not-allowed-dependent/turbo.json @@ -0,0 +1,3 @@ +{ + "tags": [] +} \ No newline at end of file diff --git a/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json index 9e26dfeeb6e64..becf2c42b2707 100644 --- a/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json +++ b/turborepo-tests/integration/fixtures/boundaries_tags/turbo.json @@ -1 +1,26 @@ -{} \ No newline at end of file +{ + "boundaries": { + "tags": { + "web": { + "dependencies": { + "allow": [ + "types" + ], + "deny": [ + "unsafe" + ] + }, + "dependents": { + "allow": [] + } + }, + "types": { + "dependents": { + "allow": [ + "web" + ] + } + } + } + } +} \ No newline at end of file diff --git a/turborepo-tests/integration/tests/bad-turbo-json.t b/turborepo-tests/integration/tests/bad-turbo-json.t index f18f13be76708..038e7b17c2843 100644 --- a/turborepo-tests/integration/tests/bad-turbo-json.t +++ b/turborepo-tests/integration/tests/bad-turbo-json.t @@ -9,19 +9,20 @@ Run build with package task in non-root turbo.json [1] $ sed 's/\[\([^]]*\)\]/\(\1)/g' < error.txt x Invalid turbo.json configuration - - Error: unnecessary_package_task_syntax (https://turbo.build/messages/unnecessary-package-task-syntax) - - x "my-app#build". Use "build" instead. - ,-\(apps(\/|\\)my-app(\/|\\)turbo.json:8:21\) (re) - 7 | // this comment verifies that turbo can read .json files with comments - 8 | ,-> "my-app#build": { - 9 | | "outputs": ("banana.txt", "apple.json"), - 10 | | "inputs": ("$TURBO_DEFAULT$", ".env.local") - 11 | |-> } - : `---- unnecessary package syntax found here - 12 | } - `---- + `-> unnecessary_package_task_syntax (https://turbo.build/messages/ + unnecessary-package-task-syntax) + + x "my-app#build". Use "build" instead. + ,-(apps/my-app/turbo.json:8:21) + 7 | // this comment verifies that turbo can read .json files + with comments + 8 | ,-> "my-app#build": { + 9 | | "outputs": ("banana.txt", "apple.json"), + 10 | | "inputs": ("$TURBO_DEFAULT$", ".env.local") + 11 | |-> } + : `---- unnecessary package syntax found here + 12 | } + `---- @@ -92,32 +93,26 @@ Run build with syntax errors in turbo.json turbo_json_parse_error x Failed to parse turbo.json. - - Error: - x Expected a property but instead found ','. - ,-[turbo.json:2:48] - 1 | { - 2 | "$schema": "https://turbo.build/schema.json",, - : ^ - 3 | "globalDependencies": ["foo.txt"], - `---- - - Error: - x expected `,` but instead found `42` - ,-[turbo.json:12:46] - 11 | "my-app#build": { - 12 | "outputs": ["banana.txt", "apple.json"]42, - : ^^ - 13 | "inputs": [".env.local" - `---- - - Error: - x expected `,` but instead found `}` - ,-[turbo.json:14:5] - 13 | "inputs": [".env.local" - 14 | }, - : ^ - 15 | - `---- + |-> x Expected a property but instead found ','. + | ,-[turbo.json:2:48] + | 1 | { + | 2 | "$schema": "https://turbo.build/schema.json",, + | : ^ + | 3 | "globalDependencies": ["foo.txt"], + | `---- + |-> x expected `,` but instead found `42` + | ,-[turbo.json:12:46] + | 11 | "my-app#build": { + | 12 | "outputs": ["banana.txt", "apple.json"]42, + | : ^^ + | 13 | "inputs": [".env.local" + | `---- + `-> x expected `,` but instead found `}` + ,-[turbo.json:14:5] + 13 | "inputs": [".env.local" + 14 | }, + : ^ + 15 | + `---- [1] diff --git a/turborepo-tests/integration/tests/dry-json/monorepo.t b/turborepo-tests/integration/tests/dry-json/monorepo.t index ac8abddb619bb..cd1646ea5ae95 100644 --- a/turborepo-tests/integration/tests/dry-json/monorepo.t +++ b/turborepo-tests/integration/tests/dry-json/monorepo.t @@ -180,8 +180,6 @@ Run again with NODE_ENV set and see the value in the summary. --filter=util work Tasks that don't exist throw an error $ ${TURBO} run doesnotexist --dry=json x Missing tasks in project - - Error: - x Could not find task `doesnotexist` in project + `-> x Could not find task `doesnotexist` in project [1] diff --git a/turborepo-tests/integration/tests/invalid-package-json.t b/turborepo-tests/integration/tests/invalid-package-json.t index 8338c00c4b3ee..de7fcef5bc1cd 100644 --- a/turborepo-tests/integration/tests/invalid-package-json.t +++ b/turborepo-tests/integration/tests/invalid-package-json.t @@ -69,13 +69,12 @@ Build should fail due to trailing comma (sed replaces square brackets with paren package_json_parse_error x Unable to parse package.json. - - Error: - x Expected a property but instead found '}'. - ,-\(.*package.json:1:21\) (re) - 1 | { "name": "foobar", } - : ^ - `---- + `-> x Expected a property but instead found '}'. + ,-\[.* (re) + .*package.json:1:21\] (re) + 1 | { "name": "foobar", } + : ^ + `---- diff --git a/turborepo-tests/integration/tests/persistent-dependencies/1-topological.t b/turborepo-tests/integration/tests/persistent-dependencies/1-topological.t index 7b960c5a48f2b..5bd7157508685 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/1-topological.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/1-topological.t @@ -14,15 +14,13 @@ // └── pkg-a#dev $ ${TURBO} run dev x Invalid task configuration - - Error: - x "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it - ,-[turbo.json:5:21] - 4 | "dev": { - 5 | "dependsOn": ["^dev"], - : ^^^|^^ - : `-- persistent task - 6 | "persistent": true - `---- + `-> x "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it + ,-[turbo.json:5:21] + 4 | "dev": { + 5 | "dependsOn": ["^dev"], + : ^^^|^^ + : `-- persistent task + 6 | "persistent": true + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/10-too-many.t b/turborepo-tests/integration/tests/persistent-dependencies/10-too-many.t index ba51006c87abf..329713344fd58 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/10-too-many.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/10-too-many.t @@ -3,19 +3,15 @@ $ ${TURBO} run build --concurrency=1 x Invalid task configuration - - Error: - x You have 2 persistent tasks but `turbo` is configured for concurrency of - | 1. Set --concurrency to at least 3 + `-> x You have 2 persistent tasks but `turbo` is configured for + | concurrency of 1. Set --concurrency to at least 3 [1] $ ${TURBO} run build --concurrency=2 x Invalid task configuration - - Error: - x You have 2 persistent tasks but `turbo` is configured for concurrency of - | 2. Set --concurrency to at least 3 + `-> x You have 2 persistent tasks but `turbo` is configured for + | concurrency of 2. Set --concurrency to at least 3 [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/2-same-workspace.t b/turborepo-tests/integration/tests/persistent-dependencies/2-same-workspace.t index 16e642c8a956a..ab4c462ec8d8a 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/2-same-workspace.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/2-same-workspace.t @@ -14,15 +14,13 @@ // $ ${TURBO} run build x Invalid task configuration - - Error: - x "app-a#dev" is a persistent task, "app-a#build" cannot depend on it - ,-[turbo.json:5:21] - 4 | "build": { - 5 | "dependsOn": ["dev"] - : ^^|^^ - : `-- persistent task - 6 | }, - `---- + `-> x "app-a#dev" is a persistent task, "app-a#build" cannot depend on it + ,-[turbo.json:5:21] + 4 | "build": { + 5 | "dependsOn": ["dev"] + : ^^|^^ + : `-- persistent task + 6 | }, + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/3-workspace-specific.t b/turborepo-tests/integration/tests/persistent-dependencies/3-workspace-specific.t index 94290e828a6a8..b98222754e0ac 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/3-workspace-specific.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/3-workspace-specific.t @@ -18,25 +18,21 @@ # The regex match is liberal, because the build task from either workspace can throw the error $ ${TURBO} run build x Invalid task configuration - - Error: - x "pkg-a#dev" is a persistent task, "app-a#build" cannot depend on it - ,-[turbo.json:5:21] - 4 | "build": { - 5 | "dependsOn": ["pkg-a#dev"] - : ^^^^^|^^^^^ - : `-- persistent task - 6 | }, - `---- - - Error: - x "pkg-a#dev" is a persistent task, "pkg-a#build" cannot depend on it - ,-[turbo.json:5:21] - 4 | "build": { - 5 | "dependsOn": ["pkg-a#dev"] - : ^^^^^|^^^^^ - : `-- persistent task - 6 | }, - `---- + |-> x "pkg-a#dev" is a persistent task, "app-a#build" cannot depend on it + | ,-[turbo.json:5:21] + | 4 | "build": { + | 5 | "dependsOn": ["pkg-a#dev"] + | : ^^^^^|^^^^^ + | : `-- persistent task + | 6 | }, + | `---- + `-> x "pkg-a#dev" is a persistent task, "pkg-a#build" cannot depend on it + ,-[turbo.json:5:21] + 4 | "build": { + 5 | "dependsOn": ["pkg-a#dev"] + : ^^^^^|^^^^^ + : `-- persistent task + 6 | }, + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/4-cross-workspace.t b/turborepo-tests/integration/tests/persistent-dependencies/4-cross-workspace.t index 8042a84ccb6de..578c2100acb05 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/4-cross-workspace.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/4-cross-workspace.t @@ -8,15 +8,13 @@ # └── pkg-a#dev $ ${TURBO} run dev x Invalid task configuration - - Error: - x "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it - ,-[turbo.json:5:21] - 4 | "app-a#dev": { - 5 | "dependsOn": ["pkg-a#dev"], - : ^^^^^|^^^^^ - : `-- persistent task - 6 | "persistent": true - `---- + `-> x "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it + ,-[turbo.json:5:21] + 4 | "app-a#dev": { + 5 | "dependsOn": ["pkg-a#dev"], + : ^^^^^|^^^^^ + : `-- persistent task + 6 | "persistent": true + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/5-root-workspace.t b/turborepo-tests/integration/tests/persistent-dependencies/5-root-workspace.t index 7ddef2ea8fa88..80987bae54211 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/5-root-workspace.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/5-root-workspace.t @@ -14,15 +14,13 @@ # $ ${TURBO} run build x Invalid task configuration - - Error: - x "//#dev" is a persistent task, "app-a#build" cannot depend on it - ,-[turbo.json:5:21] - 4 | "build": { - 5 | "dependsOn": ["//#dev"], - : ^^^^|^^^ - : `-- persistent task - 6 | "persistent": true - `---- + `-> x "//#dev" is a persistent task, "app-a#build" cannot depend on it + ,-[turbo.json:5:21] + 4 | "build": { + 5 | "dependsOn": ["//#dev"], + : ^^^^|^^^ + : `-- persistent task + 6 | "persistent": true + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/7-topological-nested.t b/turborepo-tests/integration/tests/persistent-dependencies/7-topological-nested.t index 26dee8791b931..9ae94a5f315a0 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/7-topological-nested.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/7-topological-nested.t @@ -21,15 +21,13 @@ # this case. $ ${TURBO} run dev x Invalid task configuration - - Error: - x "pkg-b#dev" is a persistent task, "pkg-a#dev" cannot depend on it - ,-[turbo.json:5:21] - 4 | "dev": { - 5 | "dependsOn": ["^dev"], - : ^^^|^^ - : `-- persistent task - 6 | "persistent": true - `---- + `-> x "pkg-b#dev" is a persistent task, "pkg-a#dev" cannot depend on it + ,-[turbo.json:5:21] + 4 | "dev": { + 5 | "dependsOn": ["^dev"], + : ^^^|^^ + : `-- persistent task + 6 | "persistent": true + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/8-topological-with-extra.t b/turborepo-tests/integration/tests/persistent-dependencies/8-topological-with-extra.t index abbe1fb2ea8b5..96a32d2817487 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/8-topological-with-extra.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/8-topological-with-extra.t @@ -20,15 +20,13 @@ // └── workspace-z#dev // this one is persistent $ ${TURBO} run build x Invalid task configuration - - Error: - x "pkg-z#dev" is a persistent task, "pkg-b#build" cannot depend on it - ,-[turbo.json:8:21] - 7 | "pkg-b#build": { - 8 | "dependsOn": ["pkg-z#dev"] - : ^^^^^|^^^^^ - : `-- persistent task - 9 | }, - `---- + `-> x "pkg-z#dev" is a persistent task, "pkg-b#build" cannot depend on it + ,-[turbo.json:8:21] + 7 | "pkg-b#build": { + 8 | "dependsOn": ["pkg-z#dev"] + : ^^^^^|^^^^^ + : `-- persistent task + 9 | }, + `---- [1] diff --git a/turborepo-tests/integration/tests/persistent-dependencies/9-cross-workspace-nested.t b/turborepo-tests/integration/tests/persistent-dependencies/9-cross-workspace-nested.t index b163ca8bfd59e..e90b9fa4b0a8c 100644 --- a/turborepo-tests/integration/tests/persistent-dependencies/9-cross-workspace-nested.t +++ b/turborepo-tests/integration/tests/persistent-dependencies/9-cross-workspace-nested.t @@ -13,15 +13,13 @@ // $ ${TURBO} run build x Invalid task configuration - - Error: - x "app-z#dev" is a persistent task, "app-c#build" cannot depend on it - ,-[turbo.json:13:21] - 12 | "app-c#build": { - 13 | "dependsOn": ["app-z#dev"] - : ^^^^^|^^^^^ - : `-- persistent task - 14 | }, - `---- + `-> x "app-z#dev" is a persistent task, "app-c#build" cannot depend on it + ,-[turbo.json:13:21] + 12 | "app-c#build": { + 13 | "dependsOn": ["app-z#dev"] + : ^^^^^|^^^^^ + : `-- persistent task + 14 | }, + `---- [1] diff --git a/turborepo-tests/integration/tests/query/validation.t b/turborepo-tests/integration/tests/query/validation.t index 09bb86518ede5..7a48a7720730e 100644 --- a/turborepo-tests/integration/tests/query/validation.t +++ b/turborepo-tests/integration/tests/query/validation.t @@ -4,10 +4,8 @@ Setup Validate that we get an error when we try to run multiple persistent tasks with concurrency 1 $ ${TURBO} run build --concurrency=1 x Invalid task configuration - - Error: - x You have 2 persistent tasks but `turbo` is configured for concurrency of - | 1. Set --concurrency to at least 3 + `-> x You have 2 persistent tasks but `turbo` is configured for + | concurrency of 1. Set --concurrency to at least 3 [1] diff --git a/turborepo-tests/integration/tests/run/missing-tasks.t b/turborepo-tests/integration/tests/run/missing-tasks.t index 370c2dca67e94..ae7c3c035a2ca 100644 --- a/turborepo-tests/integration/tests/run/missing-tasks.t +++ b/turborepo-tests/integration/tests/run/missing-tasks.t @@ -4,30 +4,22 @@ Setup # Running non-existent tasks errors $ ${TURBO} run doesnotexist x Missing tasks in project - - Error: - x Could not find task `doesnotexist` in project + `-> x Could not find task `doesnotexist` in project [1] # Multiple non-existent tasks also error $ ${TURBO} run doesnotexist alsono x Missing tasks in project - - Error: - x Could not find task `alsono` in project - - Error: - x Could not find task `doesnotexist` in project + |-> x Could not find task `alsono` in project + `-> x Could not find task `doesnotexist` in project [1] # One good and one bad task does not error $ ${TURBO} run build doesnotexist x Missing tasks in project - - Error: - x Could not find task `doesnotexist` in project + `-> x Could not find task `doesnotexist` in project [1] diff --git a/turborepo-tests/integration/tests/workspace-configs/persistent.t b/turborepo-tests/integration/tests/workspace-configs/persistent.t index c0a8b8f36dd15..7b717ab722d4a 100644 --- a/turborepo-tests/integration/tests/workspace-configs/persistent.t +++ b/turborepo-tests/integration/tests/workspace-configs/persistent.t @@ -11,17 +11,15 @@ This test covers: # persistent-task-1 is persistent:true in the root workspace, and does NOT get overriden in the workspace $ ${TURBO} run persistent-task-1-parent --filter=persistent x Invalid task configuration - - Error: - x "persistent#persistent-task-1" is a persistent task, - | "persistent#persistent-task-1-parent" cannot depend on it - ,-[turbo.json:89:9] - 88 | "dependsOn": [ - 89 | "persistent-task-1" - : ^^^^^^^^^|^^^^^^^^^ - : `-- persistent task - 90 | ] - `---- + `-> x "persistent#persistent-task-1" is a persistent task, + | "persistent#persistent-task-1-parent" cannot depend on it + ,-[turbo.json:89:9] + 88 | "dependsOn": [ + 89 | "persistent-task-1" + : ^^^^^^^^^|^^^^^^^^^ + : `-- persistent task + 90 | ] + `---- [1] @@ -53,17 +51,15 @@ This test covers: # persistent-task-3 is defined in workspace, but does NOT have the persistent flag $ ${TURBO} run persistent-task-3-parent --filter=persistent x Invalid task configuration - - Error: - x "persistent#persistent-task-3" is a persistent task, - | "persistent#persistent-task-3-parent" cannot depend on it - ,-[turbo.json:99:9] - 98 | "dependsOn": [ - 99 | "persistent-task-3" - : ^^^^^^^^^|^^^^^^^^^ - : `-- persistent task - 100 | ] - `---- + `-> x "persistent#persistent-task-3" is a persistent task, + | "persistent#persistent-task-3-parent" cannot depend on it + ,-[turbo.json:99:9] + 98 | "dependsOn": [ + 99 | "persistent-task-3" + : ^^^^^^^^^|^^^^^^^^^ + : `-- persistent task + 100 | ] + `---- [1] @@ -71,16 +67,14 @@ This test covers: # persistent-task-4 has no config in the root workspace, and is set to true in the workspace $ ${TURBO} run persistent-task-4-parent --filter=persistent x Invalid task configuration - - Error: - x "persistent#persistent-task-4" is a persistent task, - | "persistent#persistent-task-4-parent" cannot depend on it - ,-[turbo.json:104:9] - 103 | "dependsOn": [ - 104 | "persistent-task-4" - : ^^^^^^^^^|^^^^^^^^^ - : `-- persistent task - 105 | ] - `---- + `-> x "persistent#persistent-task-4" is a persistent task, + | "persistent#persistent-task-4-parent" cannot depend on it + ,-[turbo.json:104:9] + 103 | "dependsOn": [ + 104 | "persistent-task-4" + : ^^^^^^^^^|^^^^^^^^^ + : `-- persistent task + 105 | ] + `---- [1] From fc47970a7f5efe170852dac1b03e3ae51fc9f089 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 7 Feb 2025 15:42:44 -0500 Subject: [PATCH 14/16] Test names clashing --- crates/turborepo-lib/src/turbo_json/mod.rs | 6 +++--- .../snapshots/turborepo_lib__turbo_json__tests__empty.snap | 6 ++++-- ...turborepo_lib__turbo_json__tests__empty_boundaries.snap | 7 +++++++ ...repo_lib__turbo_json__tests__empty_tags_in_package.snap | 5 +++++ turborepo-tests/integration/tests/bad-turbo-json.t | 2 +- 5 files changed, 20 insertions(+), 6 deletions(-) create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_boundaries.snap create mode 100644 crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags_in_package.snap diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index ed7d89bd93d9e..16c0fe8a28f3c 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -781,7 +781,7 @@ mod tests { turbo_json::RawTaskDefinition, }; - #[test_case("{}", "empty")] + #[test_case("{}", "empty boundaries")] #[test_case(r#"{"tags": {} }"#, "empty tags")] #[test_case( r#"{"tags": { "my-tag": { "dependencies": { "allow": ["my-package"] } } } }"#, @@ -828,7 +828,7 @@ mod tests { "{}", RawTaskDefinition::default(), TaskDefinition::default() - ; "empty")] + ; "empty task definition")] #[test_case( r#"{ "persistent": false }"#, RawTaskDefinition { @@ -1045,7 +1045,7 @@ mod tests { assert_eq!(actual, expected); } - #[test_case(r#"{ "tags": [] }"#, "empty")] + #[test_case(r#"{ "tags": [] }"#, "empty tags in package")] #[test_case(r#"{ "tags": ["my-tag"] }"#, "one tag")] #[test_case(r#"{ "tags": ["my-tag", "my-other-tag"] }"#, "two tags")] fn test_tags(json: &str, name: &str) { diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap index 1497dfca7fe29..e4b2050155490 100644 --- a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty.snap @@ -1,5 +1,7 @@ --- source: crates/turborepo-lib/src/turbo_json/mod.rs -expression: json.tags +expression: raw_task_definition --- -[] +{ + "tags": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_boundaries.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_boundaries.snap new file mode 100644 index 0000000000000..e4b2050155490 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_boundaries.snap @@ -0,0 +1,7 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: raw_task_definition +--- +{ + "tags": null +} diff --git a/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags_in_package.snap b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags_in_package.snap new file mode 100644 index 0000000000000..1497dfca7fe29 --- /dev/null +++ b/crates/turborepo-lib/src/turbo_json/snapshots/turborepo_lib__turbo_json__tests__empty_tags_in_package.snap @@ -0,0 +1,5 @@ +--- +source: crates/turborepo-lib/src/turbo_json/mod.rs +expression: json.tags +--- +[] diff --git a/turborepo-tests/integration/tests/bad-turbo-json.t b/turborepo-tests/integration/tests/bad-turbo-json.t index 038e7b17c2843..221455abacedb 100644 --- a/turborepo-tests/integration/tests/bad-turbo-json.t +++ b/turborepo-tests/integration/tests/bad-turbo-json.t @@ -13,7 +13,7 @@ Run build with package task in non-root turbo.json unnecessary-package-task-syntax) x "my-app#build". Use "build" instead. - ,-(apps/my-app/turbo.json:8:21) + ,-\(apps(\/|\\)my-app(\/|\\)turbo.json:8:21\) (re) 7 | // this comment verifies that turbo can read .json files with comments 8 | ,-> "my-app#build": { From 68a00bdf9711696844048c3656fbf252ea093eb4 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 10 Feb 2025 11:34:24 -0500 Subject: [PATCH 15/16] Revert turbo_json_loader changes --- crates/turborepo-lib/src/boundaries/tags.rs | 5 +- crates/turborepo-lib/src/engine/builder.rs | 72 ++++++++++----------- crates/turborepo-lib/src/run/builder.rs | 6 +- crates/turborepo-lib/src/run/mod.rs | 6 +- 4 files changed, 45 insertions(+), 44 deletions(-) diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs index 36abd672ffc15..5e73d70e757e2 100644 --- a/crates/turborepo-lib/src/boundaries/tags.rs +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -50,12 +50,13 @@ impl From for ProcessedPermissions { impl Run { pub(crate) fn get_package_tags(&self) -> HashMap>>> { let mut package_tags = HashMap::new(); + let mut turbo_json_loader = self.turbo_json_loader(); for (package, _) in self.pkg_dep_graph().packages() { if let Ok(TurboJson { tags: Some(tags), .. - }) = self.turbo_json_loader().uncached_load(package) + }) = turbo_json_loader.load(package) { - package_tags.insert(package.clone(), tags); + package_tags.insert(package.clone(), tags.clone()); } } diff --git a/crates/turborepo-lib/src/engine/builder.rs b/crates/turborepo-lib/src/engine/builder.rs index 7695f9c7fbd09..16013d325cdc6 100644 --- a/crates/turborepo-lib/src/engine/builder.rs +++ b/crates/turborepo-lib/src/engine/builder.rs @@ -118,7 +118,7 @@ pub enum Error { pub struct EngineBuilder<'a> { repo_root: &'a AbsoluteSystemPath, package_graph: &'a PackageGraph, - turbo_json_loader: Option<&'a mut TurboJsonLoader>, + turbo_json_loader: Option, is_single: bool, workspaces: Vec, tasks: Vec>>, @@ -132,7 +132,7 @@ impl<'a> EngineBuilder<'a> { pub fn new( repo_root: &'a AbsoluteSystemPath, package_graph: &'a PackageGraph, - turbo_json_loader: &'a mut TurboJsonLoader, + turbo_json_loader: TurboJsonLoader, is_single: bool, ) -> Self { Self { @@ -218,7 +218,7 @@ impl<'a> EngineBuilder<'a> { return Ok(Engine::default().seal()); } - let turbo_json_loader = self + let mut turbo_json_loader = self .turbo_json_loader .take() .expect("engine builder cannot be constructed without TurboJsonLoader"); @@ -259,7 +259,7 @@ impl<'a> EngineBuilder<'a> { .task_id() .unwrap_or_else(|| TaskId::new(workspace.as_ref(), task.task())); - if Self::has_task_definition(turbo_json_loader, workspace, task, &task_id)? { + if Self::has_task_definition(&mut turbo_json_loader, workspace, task, &task_id)? { missing_tasks.remove(task.as_inner()); // Even if a task definition was found, we _only_ want to add it as an entry @@ -359,7 +359,7 @@ impl<'a> EngineBuilder<'a> { } let task_definition = self.task_definition( - turbo_json_loader, + &mut turbo_json_loader, &task_id, &task_id.as_non_workspace_task_name(), )?; @@ -853,8 +853,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![ PackageName::from("a"), @@ -910,8 +910,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![PackageName::from("app2")]) .build() @@ -949,8 +949,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("special")))) .with_workspaces(vec![PackageName::from("app1"), PackageName::from("libA")]) .build() @@ -987,8 +987,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(vec![ Spanned::new(TaskName::from("build")), Spanned::new(TaskName::from("test")), @@ -1040,8 +1040,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1083,8 +1083,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![TaskName::from("libA#build"), TaskName::from("build")]) @@ -1118,8 +1118,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1163,8 +1163,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("app1")]) .with_root_tasks(vec![ @@ -1202,8 +1202,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("test")))) .with_workspaces(vec![ @@ -1249,8 +1249,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("b")]) @@ -1288,8 +1288,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks_only(true) .with_tasks(Some(Spanned::new(TaskName::from("build")))) .with_workspaces(vec![PackageName::from("b")]) @@ -1356,8 +1356,8 @@ mod test { ] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(vec![ Spanned::new(TaskName::from("app1#special")), Spanned::new(TaskName::from("app2#another")), @@ -1398,8 +1398,8 @@ mod test { })] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(Some(Spanned::new(TaskName::from("dev")))) .with_workspaces(vec![PackageName::from("web")]) .build() @@ -1445,8 +1445,8 @@ mod test { ] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader.clone(), false) .with_tasks(vec![Spanned::new(TaskName::from("app1#special"))]) .with_workspaces(vec![PackageName::from("app1")]) .build(); @@ -1458,7 +1458,7 @@ mod test { .unwrap(); assert_json_snapshot!(msg); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let engine = EngineBuilder::new(&repo_root, &package_graph, loader, false) .with_tasks(vec![Spanned::new(TaskName::from("app1#another"))]) .with_workspaces(vec![PackageName::from("libA")]) .build(); @@ -1493,8 +1493,8 @@ mod test { )] .into_iter() .collect(); - let mut loader = TurboJsonLoader::noop(turbo_jsons); - let engine = EngineBuilder::new(&repo_root, &package_graph, &mut loader, false) + let loader = TurboJsonLoader::noop(turbo_jsons); + let engine = EngineBuilder::new(&repo_root, &package_graph, loader.clone(), false) .with_tasks(vec![Spanned::new(TaskName::from("app2#bad-task"))]) .with_workspaces(vec![PackageName::from("app1"), PackageName::from("libA")]) .build(); diff --git a/crates/turborepo-lib/src/run/builder.rs b/crates/turborepo-lib/src/run/builder.rs index 3802a534c964b..f1773daa87785 100644 --- a/crates/turborepo-lib/src/run/builder.rs +++ b/crates/turborepo-lib/src/run/builder.rs @@ -436,7 +436,7 @@ impl RunBuilder { &pkg_dep_graph, &root_turbo_json, filtered_pkgs.keys(), - &mut turbo_json_loader, + turbo_json_loader.clone(), )?; if self.opts.run_opts.parallel { @@ -445,7 +445,7 @@ impl RunBuilder { &pkg_dep_graph, &root_turbo_json, filtered_pkgs.keys(), - &mut turbo_json_loader, + turbo_json_loader.clone(), )?; } @@ -497,7 +497,7 @@ impl RunBuilder { pkg_dep_graph: &PackageGraph, root_turbo_json: &TurboJson, filtered_pkgs: impl Iterator, - turbo_json_loader: &mut TurboJsonLoader, + turbo_json_loader: TurboJsonLoader, ) -> Result { let tasks = self.opts.run_opts.tasks.iter().map(|task| { // TODO: Pull span info from command diff --git a/crates/turborepo-lib/src/run/mod.rs b/crates/turborepo-lib/src/run/mod.rs index 020acb4906b04..964cefb892760 100644 --- a/crates/turborepo-lib/src/run/mod.rs +++ b/crates/turborepo-lib/src/run/mod.rs @@ -66,7 +66,7 @@ pub struct Run { env_at_execution_start: EnvironmentVariableMap, filtered_pkgs: HashSet, pkg_dep_graph: Arc, - pub(crate) turbo_json_loader: TurboJsonLoader, + turbo_json_loader: TurboJsonLoader, root_turbo_json: TurboJson, scm: SCM, run_cache: Arc, @@ -123,8 +123,8 @@ impl Run { } } - pub fn turbo_json_loader(&self) -> &TurboJsonLoader { - &self.turbo_json_loader + pub fn turbo_json_loader(&self) -> TurboJsonLoader { + self.turbo_json_loader.clone() } pub fn opts(&self) -> &Opts { From 9baee66ff7978602f9eb3c0c48049e073d5cd7d4 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 10 Feb 2025 12:24:40 -0500 Subject: [PATCH 16/16] Add warning for boundaries rules in non-root turbo.json --- crates/turborepo-lib/src/boundaries/tags.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/turborepo-lib/src/boundaries/tags.rs b/crates/turborepo-lib/src/boundaries/tags.rs index 5e73d70e757e2..c016f8a1150e6 100644 --- a/crates/turborepo-lib/src/boundaries/tags.rs +++ b/crates/turborepo-lib/src/boundaries/tags.rs @@ -1,5 +1,6 @@ use std::collections::{HashMap, HashSet}; +use tracing::warn; use turborepo_errors::Spanned; use turborepo_repository::package_graph::{PackageName, PackageNode}; @@ -53,9 +54,17 @@ impl Run { let mut turbo_json_loader = self.turbo_json_loader(); for (package, _) in self.pkg_dep_graph().packages() { if let Ok(TurboJson { - tags: Some(tags), .. + tags: Some(tags), + boundaries, + .. }) = turbo_json_loader.load(package) { + if boundaries.is_some() && !matches!(package, PackageName::Root) { + warn!( + "Boundaries rules can only be defined in the root turbo.json. Any rules \ + defined in a package's turbo.json will be ignored." + ) + } package_tags.insert(package.clone(), tags.clone()); } }