From 1ee2e7ac4e8b53933d21b94d8f0554fc5335eacd Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 4 Jun 2024 15:59:54 -0400 Subject: [PATCH 01/11] Added package.json errors using biome span info --- crates/turborepo-errors/src/lib.rs | 1 - crates/turborepo-lib/src/engine/mod.rs | 7 ++- crates/turborepo-lib/src/run/error.rs | 2 + .../src/run/summary/task_factory.rs | 1 + .../turborepo-lib/src/task_graph/visitor.rs | 15 +++++- crates/turborepo-lib/src/turbo_json/mod.rs | 4 +- .../turborepo-repository/src/package_json.rs | 48 ++++++++++++++----- .../src/package_manager/mod.rs | 38 ++++++++++----- 8 files changed, 86 insertions(+), 30 deletions(-) diff --git a/crates/turborepo-errors/src/lib.rs b/crates/turborepo-errors/src/lib.rs index 701d0ba704f5d..43314adbefebd 100644 --- a/crates/turborepo-errors/src/lib.rs +++ b/crates/turborepo-errors/src/lib.rs @@ -191,7 +191,6 @@ impl Deref for Spanned { &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/engine/mod.rs b/crates/turborepo-lib/src/engine/mod.rs index 3e6b7ac712043..501051bb4ae2c 100644 --- a/crates/turborepo-lib/src/engine/mod.rs +++ b/crates/turborepo-lib/src/engine/mod.rs @@ -586,8 +586,11 @@ mod test { let scripts = if had_build { BTreeMap::from_iter( [ - ("build".to_string(), "echo built!".to_string()), - ("dev".to_string(), "echo running dev!".to_string()), + ("build".to_string(), Spanned::new("echo built!".to_string())), + ( + "dev".to_string(), + Spanned::new("echo running dev!".to_string()), + ), ] .into_iter(), ) diff --git a/crates/turborepo-lib/src/run/error.rs b/crates/turborepo-lib/src/run/error.rs index aca34ea786df5..706a3ba5a6e79 100644 --- a/crates/turborepo-lib/src/run/error.rs +++ b/crates/turborepo-lib/src/run/error.rs @@ -29,6 +29,7 @@ pub enum Error { #[diagnostic(transparent)] PackageJson(#[from] turborepo_repository::package_json::Error), #[error(transparent)] + #[diagnostic(transparent)] PackageManager(#[from] turborepo_repository::package_manager::Error), #[error(transparent)] #[diagnostic(transparent)] @@ -49,6 +50,7 @@ pub enum Error { #[error(transparent)] TaskHash(#[from] task_hash::Error), #[error(transparent)] + #[diagnostic(transparent)] Visitor(#[from] task_graph::VisitorError), #[error("error registering signal handler: {0}")] SignalHandler(std::io::Error), diff --git a/crates/turborepo-lib/src/run/summary/task_factory.rs b/crates/turborepo-lib/src/run/summary/task_factory.rs index e2bbaeb726a2d..4f3effccffd5a 100644 --- a/crates/turborepo-lib/src/run/summary/task_factory.rs +++ b/crates/turborepo-lib/src/run/summary/task_factory.rs @@ -114,6 +114,7 @@ impl<'a> TaskSummaryFactory<'a> { .package_json .scripts .get(task_id.task()) + .map(|script| script.as_inner()) .cloned() .unwrap_or_else(|| "".to_string()); diff --git a/crates/turborepo-lib/src/task_graph/visitor.rs b/crates/turborepo-lib/src/task_graph/visitor.rs index 773c342492c71..3852edf766f63 100644 --- a/crates/turborepo-lib/src/task_graph/visitor.rs +++ b/crates/turborepo-lib/src/task_graph/visitor.rs @@ -10,6 +10,7 @@ use console::{Style, StyledObject}; use either::Either; use futures::{stream::FuturesUnordered, StreamExt}; use itertools::Itertools; +use miette::{Diagnostic, NamedSource, SourceSpan}; use regex::Regex; use tokio::sync::{mpsc, oneshot}; use tracing::{debug, error, Instrument, Span}; @@ -67,7 +68,7 @@ pub struct Visitor<'a> { is_watch: bool, } -#[derive(Debug, thiserror::Error)] +#[derive(Debug, thiserror::Error, Diagnostic)] pub enum Error { #[error("cannot find package {package_name} for task {task_id}")] MissingPackage { @@ -77,7 +78,14 @@ pub enum Error { #[error( "root task {task_name} ({command}) looks like it invokes turbo and might cause a loop" )] - RecursiveTurbo { task_name: String, command: String }, + RecursiveTurbo { + task_name: String, + command: String, + #[label("task found here")] + span: Option, + #[source_code] + text: NamedSource, + }, #[error("Could not find definition for task")] MissingDefinition, #[error("error while executing engine: {0}")] @@ -186,9 +194,12 @@ impl<'a> Visitor<'a> { match command { Some(cmd) if info.package() == ROOT_PKG_NAME && turbo_regex().is_match(&cmd) => { package_task_event.track_error(TrackedErrors::RecursiveError); + let (span, text) = cmd.span_and_text("package.json"); return Err(Error::RecursiveTurbo { task_name: info.to_string(), command: cmd.to_string(), + span, + text, }); } _ => (), diff --git a/crates/turborepo-lib/src/turbo_json/mod.rs b/crates/turborepo-lib/src/turbo_json/mod.rs index eb4c483298b3c..db4045ba741e7 100644 --- a/crates/turborepo-lib/src/turbo_json/mod.rs +++ b/crates/turborepo-lib/src/turbo_json/mod.rs @@ -794,7 +794,7 @@ mod tests { #[test_case( None, PackageJson { - scripts: [("build".to_string(), "echo build".to_string())].into_iter().collect(), + scripts: [("build".to_string(), Spanned::new("echo build".to_string()))].into_iter().collect(), ..PackageJson::default() }, TurboJson { @@ -818,7 +818,7 @@ mod tests { } }"#), PackageJson { - scripts: [("test".to_string(), "echo test".to_string())].into_iter().collect(), + scripts: [("test".to_string(), Spanned::new("echo test".to_string()))].into_iter().collect(), ..PackageJson::default() }, TurboJson { diff --git a/crates/turborepo-repository/src/package_json.rs b/crates/turborepo-repository/src/package_json.rs index 61de1d3fedde0..af2406c10971d 100644 --- a/crates/turborepo-repository/src/package_json.rs +++ b/crates/turborepo-repository/src/package_json.rs @@ -1,4 +1,7 @@ -use std::collections::{BTreeMap, HashMap}; +use std::{ + collections::{BTreeMap, HashMap}, + sync::Arc, +}; use anyhow::Result; use biome_deserialize::{json::deserialize_from_json_str, Text}; @@ -8,7 +11,7 @@ use biome_json_parser::JsonParserOptions; use miette::Diagnostic; use serde::Serialize; use turbopath::{AbsoluteSystemPath, RelativeUnixPathBuf}; -use turborepo_errors::ParseDiagnostic; +use turborepo_errors::{ParseDiagnostic, Spanned, WithMetadata}; use turborepo_unescape::UnescapedString; #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)] @@ -19,7 +22,7 @@ pub struct PackageJson { #[serde(skip_serializing_if = "Option::is_none")] pub version: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub package_manager: Option, + pub package_manager: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub dependencies: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -29,7 +32,7 @@ pub struct PackageJson { #[serde(skip_serializing_if = "Option::is_none")] pub peer_dependencies: Option>, #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub scripts: BTreeMap, + pub scripts: BTreeMap>, #[serde(skip_serializing_if = "Option::is_none")] pub resolutions: Option>, #[serde(skip_serializing_if = "Option::is_none")] @@ -53,12 +56,12 @@ pub struct PnpmConfig { pub struct RawPackageJson { pub name: Option, pub version: Option, - pub package_manager: Option, + pub package_manager: Option>, pub dependencies: Option>, pub dev_dependencies: Option>, pub optional_dependencies: Option>, pub peer_dependencies: Option>, - pub scripts: BTreeMap, + pub scripts: BTreeMap>, pub resolutions: Option>, pub pnpm: Option, // Unstructured fields kept for round trip capabilities @@ -85,12 +88,32 @@ pub enum Error { Parse(#[related] Vec), } +impl WithMetadata for RawPackageJson { + fn add_text(&mut self, text: Arc) { + self.package_manager + .as_mut() + .map(|p| p.add_text(text.clone())); + self.scripts + .iter_mut() + .for_each(|(_, v)| v.add_text(text.clone())); + } + + fn add_path(&mut self, path: Arc) { + self.package_manager + .as_mut() + .map(|p| p.add_path(path.clone())); + self.scripts + .iter_mut() + .for_each(|(_, v)| v.add_path(path.clone())); + } +} + impl From for PackageJson { fn from(raw: RawPackageJson) -> Self { Self { name: raw.name.map(|s| s.into()), version: raw.version.map(|s| s.into()), - package_manager: raw.package_manager.map(|s| s.into()), + package_manager: raw.package_manager.map(|s| s.map(|s| s.into())), dependencies: raw .dependencies .map(|m| m.into_iter().map(|(k, v)| (k, v.into())).collect()), @@ -106,7 +129,7 @@ impl From for PackageJson { scripts: raw .scripts .into_iter() - .map(|(k, v)| (k, v.into())) + .map(|(k, v)| (k, v.map(|v| v.into()))) .collect(), resolutions: raw .resolutions @@ -158,9 +181,12 @@ impl PackageJson { } // We expect a result if there are no errors - Ok(result - .expect("no parse errors produced but no result") - .into()) + let mut package_json = result.expect("no parse errors produced but no result"); + + package_json.add_path(path.into()); + package_json.add_text(contents.into()); + + Ok(package_json.into()) } // Utility method for easy construction of package.json during testing diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index 2f04c99de57ec..f40f414fc7f2a 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -15,11 +15,13 @@ use bun::BunDetector; use globwalk::{fix_glob_pattern, ValidatedGlob}; use itertools::{Either, Itertools}; use lazy_regex::{lazy_regex, Lazy}; +use miette::{Diagnostic, NamedSource, SourceSpan}; use npm::NpmDetector; use regex::Regex; use serde::{Deserialize, Serialize}; use thiserror::Error; use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf, PathError, RelativeUnixPath}; +use turborepo_errors::Spanned; use turborepo_lockfiles::Lockfile; use wax::{Any, Glob, Program}; use which::which; @@ -262,7 +264,7 @@ impl From for Error { } } -#[derive(Debug, Error)] +#[derive(Debug, Error, Diagnostic)] pub enum Error { #[error("io error: {0}")] Io(#[from] std::io::Error, #[backtrace] backtrace::Backtrace), @@ -293,9 +295,16 @@ pub enum Error { #[error(transparent)] Path(#[from] turbopath::PathError), #[error( - "We could not parse the packageManager field in package.json, expected: {0}, received: {1}" + "could not parse the packageManager field in package.json,\nexpected to match regular \ + expression {pattern}" )] - InvalidPackageManager(String, String), + InvalidPackageManager { + pattern: String, + #[label("invalid `packageManager` field")] + span: Option, + #[source_code] + text: NamedSource, + }, #[error(transparent)] WalkError(#[from] globwalk::WalkError), #[error("invalid workspace glob {0}: {1}")] @@ -309,8 +318,6 @@ pub enum Error { WorkspaceDiscovery(#[from] discovery::Error), #[error("missing packageManager field in package.json")] MissingPackageManager, - #[error("{0} set in packageManager is not a supported package manager")] - UnsupportedPackageManager(String), } impl From for Error { @@ -447,7 +454,9 @@ impl PackageManager { "bun" => Ok(PackageManager::Bun), "yarn" => Ok(YarnDetector::detect_berry_or_yarn(&version)?), "pnpm" => Ok(PnpmDetector::detect_pnpm6_or_pnpm(&version)?), - _ => Err(Error::UnsupportedPackageManager(manager.to_owned())), + _ => unreachable!( + "found invalid package manager even though regex should have caught it" + ), } } @@ -482,16 +491,20 @@ impl PackageManager { Self::get_package_manager(package_json).or_else(|_| Self::detect_package_manager(repo_root)) } - pub(crate) fn parse_package_manager_string(manager: &str) -> Result<(&str, &str), Error> { + pub(crate) fn parse_package_manager_string( + manager: &Spanned, + ) -> Result<(&str, &str), Error> { if let Some(captures) = PACKAGE_MANAGER_PATTERN.captures(manager) { let manager = captures.name("manager").unwrap().as_str(); let version = captures.name("version").unwrap().as_str(); Ok((manager, version)) } else { - Err(Error::InvalidPackageManager( - PACKAGE_MANAGER_PATTERN.to_string(), - manager.to_string(), - )) + let (span, text) = manager.span_and_text("package.json"); + Err(Error::InvalidPackageManager { + pattern: PACKAGE_MANAGER_PATTERN.to_string(), + span, + text, + }) } } @@ -800,7 +813,8 @@ mod tests { ]; for case in tests { - let result = PackageManager::parse_package_manager_string(&case.package_manager); + let result = + PackageManager::parse_package_manager_string(&Spanned::new(case.package_manager)); let Ok((received_manager, received_version)) = result else { assert!(case.expected_error, "{}: received error", case.name); continue; From 83fa9a38eddd407489742d136f547f7f7a2ad935 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Tue, 4 Jun 2024 16:25:08 -0400 Subject: [PATCH 02/11] Converting more errors --- .../src/package_graph/dep_splitter.rs | 2 +- .../src/package_manager/mod.rs | 22 ++++++++++++++++--- .../src/package_manager/pnpm.rs | 4 ++-- .../src/package_manager/yarn.rs | 2 +- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/crates/turborepo-repository/src/package_graph/dep_splitter.rs b/crates/turborepo-repository/src/package_graph/dep_splitter.rs index 02cca9fcd04d2..bd8ff52311a0c 100644 --- a/crates/turborepo-repository/src/package_graph/dep_splitter.rs +++ b/crates/turborepo-repository/src/package_graph/dep_splitter.rs @@ -175,7 +175,7 @@ impl<'a> DependencyVersion<'a> { _ => { // If we got this far, then we need to check the workspace package version to // see it satisfies the dependencies range to determin whether - // or not its an internal or external dependency. + // or not it's an internal or external dependency. let constraint = node_semver::Range::parse(self.version); let version = node_semver::Version::parse(package_version); diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index f40f414fc7f2a..bfb5e0b983cc5 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -285,8 +285,16 @@ pub enum Error { #[error("We detected multiple package managers in your repository: {}. Please remove one \ of them.", managers.join(", "))] MultiplePackageManagers { managers: Vec }, - #[error(transparent)] - Semver(#[from] node_semver::SemverError), + //#[error(transparent)] + //Semver(#[from] node_semver::SemverError), + #[error("invalid semantic version `{version}`")] + InvalidVersion { + version: String, + #[label("version found here")] + span: Option, + #[source_code] + text: NamedSource, + }, #[error("{0}: {1}")] // this will be something like "cannot find binary: " Which(which::Error, String), @@ -448,7 +456,15 @@ impl PackageManager { }; let (manager, version) = Self::parse_package_manager_string(package_manager)?; - let version = version.parse()?; + let version = version.parse().map_err(|_| { + let (span, text) = package_manager.span_and_text("package.json"); + Error::InvalidVersion { + version: version.to_string(), + span, + text, + } + })?; + match manager { "npm" => Ok(PackageManager::Npm), "bun" => Ok(PackageManager::Bun), diff --git a/crates/turborepo-repository/src/package_manager/pnpm.rs b/crates/turborepo-repository/src/package_manager/pnpm.rs index 6a51288d230a5..e89bea7163fff 100644 --- a/crates/turborepo-repository/src/package_manager/pnpm.rs +++ b/crates/turborepo-repository/src/package_manager/pnpm.rs @@ -24,8 +24,8 @@ impl<'a> PnpmDetector<'a> { } pub fn detect_pnpm6_or_pnpm(version: &Version) -> Result { - let pnpm6_constraint: Range = "<7.0.0".parse()?; - let pnpm9_constraint: Range = ">=9.0.0-alpha.0".parse()?; + let pnpm6_constraint: Range = "<7.0.0".parse().expect("valid version"); + let pnpm9_constraint: Range = ">=9.0.0-alpha.0".parse().expect("valid version"); if pnpm6_constraint.satisfies(version) { Ok(PackageManager::Pnpm6) } else if pnpm9_constraint.satisfies(version) { diff --git a/crates/turborepo-repository/src/package_manager/yarn.rs b/crates/turborepo-repository/src/package_manager/yarn.rs index cf4b27e75b12e..a4188d6d4d14d 100644 --- a/crates/turborepo-repository/src/package_manager/yarn.rs +++ b/crates/turborepo-repository/src/package_manager/yarn.rs @@ -36,7 +36,7 @@ impl<'a> YarnDetector<'a> { } pub fn detect_berry_or_yarn(version: &Version) -> Result { - let berry_constraint: Range = ">=2.0.0-0".parse()?; + let berry_constraint: Range = ">=2.0.0-0".parse().expect("valid version"); if berry_constraint.satisfies(version) { Ok(PackageManager::Berry) } else { From fb983ac90c40f63c1f01c87e03c621045df7bae0 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 21 Jun 2024 11:41:09 -0400 Subject: [PATCH 03/11] Fixed up tests --- .../src/package_manager/mod.rs | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index bfb5e0b983cc5..b6010d999e140 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -660,7 +660,7 @@ mod tests { struct TestCase { name: String, - package_manager: String, + package_manager: Spanned, expected_manager: String, expected_version: String, expected_error: bool, @@ -758,70 +758,70 @@ mod tests { let tests = vec![ TestCase { name: "errors with a tag version".to_owned(), - package_manager: "npm@latest".to_owned(), + package_manager: Spanned::new("npm@latest".to_owned()), expected_manager: "".to_owned(), expected_version: "".to_owned(), expected_error: true, }, TestCase { name: "errors with no version".to_owned(), - package_manager: "npm".to_owned(), + package_manager: Spanned::new("npm".to_owned()), expected_manager: "".to_owned(), expected_version: "".to_owned(), expected_error: true, }, TestCase { name: "requires fully-qualified semver versions (one digit)".to_owned(), - package_manager: "npm@1".to_owned(), + package_manager: Spanned::new("npm@1".to_owned()), expected_manager: "".to_owned(), expected_version: "".to_owned(), expected_error: true, }, TestCase { name: "requires fully-qualified semver versions (two digits)".to_owned(), - package_manager: "npm@1.2".to_owned(), + package_manager: Spanned::new("npm@1.2".to_owned()), expected_manager: "".to_owned(), expected_version: "".to_owned(), expected_error: true, }, TestCase { name: "supports custom labels".to_owned(), - package_manager: "npm@1.2.3-alpha.1".to_owned(), + package_manager: Spanned::new("npm@1.2.3-alpha.1".to_owned()), expected_manager: "npm".to_owned(), expected_version: "1.2.3-alpha.1".to_owned(), expected_error: false, }, TestCase { name: "only supports specified package managers".to_owned(), - package_manager: "pip@1.2.3".to_owned(), + package_manager: Spanned::new("pip@1.2.3".to_owned()), expected_manager: "".to_owned(), expected_version: "".to_owned(), expected_error: true, }, TestCase { name: "supports npm".to_owned(), - package_manager: "npm@0.0.1".to_owned(), + package_manager: Spanned::new("npm@0.0.1".to_owned()), expected_manager: "npm".to_owned(), expected_version: "0.0.1".to_owned(), expected_error: false, }, TestCase { name: "supports pnpm".to_owned(), - package_manager: "pnpm@0.0.1".to_owned(), + package_manager: Spanned::new("pnpm@0.0.1".to_owned()), expected_manager: "pnpm".to_owned(), expected_version: "0.0.1".to_owned(), expected_error: false, }, TestCase { name: "supports yarn".to_owned(), - package_manager: "yarn@111.0.1".to_owned(), + package_manager: Spanned::new("yarn@111.0.1".to_owned()), expected_manager: "yarn".to_owned(), expected_version: "111.0.1".to_owned(), expected_error: false, }, TestCase { name: "supports bun".to_owned(), - package_manager: "bun@1.0.1".to_owned(), + package_manager: Spanned::new("bun@1.0.1".to_owned()), expected_manager: "bun".to_owned(), expected_version: "1.0.1".to_owned(), expected_error: false, @@ -844,29 +844,29 @@ mod tests { #[test] fn test_read_package_manager() -> Result<(), Error> { let mut package_json = PackageJson { - package_manager: Some("npm@8.19.4".to_string()), + package_manager: Some(Spanned::new("npm@8.19.4".to_string())), ..Default::default() }; let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Npm); - package_json.package_manager = Some("yarn@2.0.0".to_string()); + package_json.package_manager = Some(Spanned::new("yarn@2.0.0".to_string())); let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Berry); - package_json.package_manager = Some("yarn@1.9.0".to_string()); + package_json.package_manager = Some(Spanned::new("yarn@1.9.0".to_string())); let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Yarn); - package_json.package_manager = Some("pnpm@6.0.0".to_string()); + package_json.package_manager = Some(Spanned::new("pnpm@6.0.0".to_string())); let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Pnpm6); - package_json.package_manager = Some("pnpm@7.2.0".to_string()); + package_json.package_manager = Some(Spanned::new("pnpm@7.2.0".to_string())); let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Pnpm); - package_json.package_manager = Some("bun@1.0.1".to_string()); + package_json.package_manager = Some(Spanned::new("bun@1.0.1".to_string())); let package_manager = PackageManager::read_package_manager(&package_json)?; assert_eq!(package_manager, PackageManager::Bun); From 6742d9ae390aeb74970754a10c36497f400c15bd Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 21 Jun 2024 12:44:41 -0400 Subject: [PATCH 04/11] Fix clippy --- crates/turborepo-repository/src/package_json.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-repository/src/package_json.rs b/crates/turborepo-repository/src/package_json.rs index af2406c10971d..2618e7b1c1caa 100644 --- a/crates/turborepo-repository/src/package_json.rs +++ b/crates/turborepo-repository/src/package_json.rs @@ -90,18 +90,18 @@ pub enum Error { impl WithMetadata for RawPackageJson { fn add_text(&mut self, text: Arc) { - self.package_manager - .as_mut() - .map(|p| p.add_text(text.clone())); + if let Some(ref mut package_manager) = self.package_manager { + package_manager.add_text(text.clone()); + } self.scripts .iter_mut() .for_each(|(_, v)| v.add_text(text.clone())); } fn add_path(&mut self, path: Arc) { - self.package_manager - .as_mut() - .map(|p| p.add_path(path.clone())); + if let Some(ref mut package_manager) = self.package_manager { + package_manager.add_path(path.clone()); + } self.scripts .iter_mut() .for_each(|(_, v)| v.add_path(path.clone())); From 72b519fcc3923038109820a7e68b7c2e3bd63ec5 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 11 Jul 2024 13:20:36 -0400 Subject: [PATCH 05/11] Remove commented out error variant --- crates/turborepo-repository/src/package_manager/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index b6010d999e140..1c3bb4ec9a1a5 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -285,9 +285,8 @@ pub enum Error { #[error("We detected multiple package managers in your repository: {}. Please remove one \ of them.", managers.join(", "))] MultiplePackageManagers { managers: Vec }, - //#[error(transparent)] - //Semver(#[from] node_semver::SemverError), #[error("invalid semantic version `{version}`")] + #[diagnostic(code(invalid_semantic_version))] InvalidVersion { version: String, #[label("version found here")] @@ -306,6 +305,7 @@ pub enum Error { "could not parse the packageManager field in package.json,\nexpected to match regular \ expression {pattern}" )] + #[diagnostic(code(invalid_package_manager_field))] InvalidPackageManager { pattern: String, #[label("invalid `packageManager` field")] From 4c5705cf9c7c3bd0e7f906a3e37b4e2b153f8934 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 12 Jul 2024 16:37:19 -0400 Subject: [PATCH 06/11] Added test for recursive turbo --- turborepo-tests/integration/tests/recursive-turbo.t | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 turborepo-tests/integration/tests/recursive-turbo.t diff --git a/turborepo-tests/integration/tests/recursive-turbo.t b/turborepo-tests/integration/tests/recursive-turbo.t new file mode 100644 index 0000000000000..12b8d77463356 --- /dev/null +++ b/turborepo-tests/integration/tests/recursive-turbo.t @@ -0,0 +1,13 @@ +Setup + $ . ${TESTDIR}/../../helpers/setup_integration_test.sh + +We write into a file because prysk doesn't play well +with the square brackets miette uses for source file paths + $ ${TURBO} something > tmp.log 2>&1 + [1] + $ grep --quiet -E "root task //#something \(turbo run build\) looks like it invokes turbo and" tmp.log + $ grep --quiet -E "might cause a loop" tmp.log + $ grep --quiet -E "task found here" tmp.log + $ grep --quiet -E "\"something\": \"turbo run build\"" tmp.log + + From e603e62c38fe42eb75657edb34b44835659301b7 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 12 Jul 2024 17:55:44 -0400 Subject: [PATCH 07/11] Added tests --- crates/turborepo-lib/src/cli/error.rs | 1 + .../src/package_graph/builder.rs | 8 ++- .../src/package_manager/mod.rs | 11 +++-- .../fixtures/basic_monorepo/package.json | 1 + .../integration/tests/invalid-package-json.t | 49 +++++++++++++++++-- .../integration/tests/recursive-turbo.t | 25 +++++++--- 6 files changed, 74 insertions(+), 21 deletions(-) diff --git a/crates/turborepo-lib/src/cli/error.rs b/crates/turborepo-lib/src/cli/error.rs index 4295580d08075..712d850136de6 100644 --- a/crates/turborepo-lib/src/cli/error.rs +++ b/crates/turborepo-lib/src/cli/error.rs @@ -30,6 +30,7 @@ pub enum Error { #[error(transparent)] ChromeTracing(#[from] crate::tracing::Error), #[error(transparent)] + #[diagnostic(transparent)] BuildPackageGraph(#[from] package_graph::builder::Error), #[error(transparent)] Rewrite(#[from] RewriteError), diff --git a/crates/turborepo-repository/src/package_graph/builder.rs b/crates/turborepo-repository/src/package_graph/builder.rs index c7b068009b0b7..d80a6da553caf 100644 --- a/crates/turborepo-repository/src/package_graph/builder.rs +++ b/crates/turborepo-repository/src/package_graph/builder.rs @@ -36,11 +36,9 @@ pub struct PackageGraphBuilder<'a, T> { #[derive(Debug, Diagnostic, thiserror::Error)] pub enum Error { - #[error("could not resolve workspaces: {0}")] - PackageManager( - #[from] crate::package_manager::Error, - #[backtrace] Backtrace, - ), + #[error("could not resolve workspaces")] + #[diagnostic(transparent)] + PackageManager(#[from] crate::package_manager::Error), #[error( "Failed to add workspace \"{name}\" from \"{path}\", it already exists at \ \"{existing_path}\"" diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index 1c3bb4ec9a1a5..1ed100fb5ff9d 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -17,6 +17,7 @@ use itertools::{Either, Itertools}; use lazy_regex::{lazy_regex, Lazy}; use miette::{Diagnostic, NamedSource, SourceSpan}; use npm::NpmDetector; +use node_semver::SemverError; use regex::Regex; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -285,10 +286,10 @@ pub enum Error { #[error("We detected multiple package managers in your repository: {}. Please remove one \ of them.", managers.join(", "))] MultiplePackageManagers { managers: Vec }, - #[error("invalid semantic version `{version}`")] + #[error("invalid semantic version: {explanation}")] #[diagnostic(code(invalid_semantic_version))] InvalidVersion { - version: String, + explanation: String, #[label("version found here")] span: Option, #[source_code] @@ -302,7 +303,7 @@ pub enum Error { #[error(transparent)] Path(#[from] turbopath::PathError), #[error( - "could not parse the packageManager field in package.json,\nexpected to match regular \ + "could not parse the packageManager field in package.json, expected to match regular \ expression {pattern}" )] #[diagnostic(code(invalid_package_manager_field))] @@ -456,10 +457,10 @@ impl PackageManager { }; let (manager, version) = Self::parse_package_manager_string(package_manager)?; - let version = version.parse().map_err(|_| { + let version = version.parse().map_err(|err: SemverError| { let (span, text) = package_manager.span_and_text("package.json"); Error::InvalidVersion { - version: version.to_string(), + explanation: err.to_string(), span, text, } diff --git a/turborepo-tests/integration/fixtures/basic_monorepo/package.json b/turborepo-tests/integration/fixtures/basic_monorepo/package.json index b3601d4b13a2c..e86a3bf3c329b 100644 --- a/turborepo-tests/integration/fixtures/basic_monorepo/package.json +++ b/turborepo-tests/integration/fixtures/basic_monorepo/package.json @@ -3,6 +3,7 @@ "scripts": { "something": "turbo run build" }, + "packageManager": "bower", "workspaces": [ "apps/**", "packages/**" diff --git a/turborepo-tests/integration/tests/invalid-package-json.t b/turborepo-tests/integration/tests/invalid-package-json.t index de27f0df1066f..235df41d8738c 100644 --- a/turborepo-tests/integration/tests/invalid-package-json.t +++ b/turborepo-tests/integration/tests/invalid-package-json.t @@ -2,24 +2,67 @@ Setup $ . ${TESTDIR}/../../helpers/setup_integration_test.sh Clear name field $ jq '.name = ""' apps/my-app/package.json > package.json.new + $ mv apps/my-app/package.json apps/my-app/package.json.old $ mv package.json.new apps/my-app/package.json Build should fail due to missing name field $ ${TURBO} build 1> ERR [1] $ grep -F --quiet 'x package.json must have a name field:' ERR +Restore name field + $ mv apps/my-app/package.json.old apps/my-app/package.json + +Clear add invalid packageManager field + $ jq '.packageManager = "bower@8.19.4"' package.json > package.json.new + $ mv package.json.new package.json + +Build should fail due to invalid packageManager field (sed removes the square brackets) + $ ${TURBO} build 2>&1 | sed 's/\[\([^]]*\)\]/\\1/g' + invalid_package_manager_field + + x could not resolve workspaces + `-> could not parse the packageManager field in package.json, expected to + match regular expression (?Pbun|npm|pnpm|yarn)@(?P\d+ + \.\d+\.\d+(-.+)?) + ,-\1 + 5 | }, + 6 | "packageManager": "bower@8.19.4", + : ^^^^^^^|^^^^^^ + : `-- invalid `packageManager` field + 7 | "workspaces": [ + `---- + + +Add invalid packageManager field that passes the regex. + $ jq '.packageManager = "npm@0.3.211111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111"' package.json > package.json.new + $ mv package.json.new package.json + + $ ${TURBO} build 2>&1 | sed 's/\[\([^]]*\)\]/\(\1)/g' + invalid_semantic_version + + x could not resolve workspaces + `-> invalid semantic version: Failed to parse an integer component of a + semver string: number too large to fit in target type + ,-\(.*/package.json:5:1\) (re) + 5 | }, + 6 | "packageManager": "npm@0.3.211111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111", + : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + : `-- version found here + 7 | "workspaces": [ + `---- + Add a trailing comma $ echo "{ \"name\": \"foobar\", }" > package.json.new $ mv package.json.new apps/my-app/package.json Build should fail due to trailing comma (sed replaces square brackets with parentheses) $ ${TURBO} build 2>&1 | sed 's/\[\([^]]*\)\]/\(\1)/g' package_json_parse_error - + x unable to parse package.json - + Error: x Expected a property but instead found '}'. ,-\(.*package.json:1:1\) (re) 1 | { "name": "foobar", } : ^ `---- - + diff --git a/turborepo-tests/integration/tests/recursive-turbo.t b/turborepo-tests/integration/tests/recursive-turbo.t index 12b8d77463356..b5688e486983d 100644 --- a/turborepo-tests/integration/tests/recursive-turbo.t +++ b/turborepo-tests/integration/tests/recursive-turbo.t @@ -1,13 +1,22 @@ Setup $ . ${TESTDIR}/../../helpers/setup_integration_test.sh -We write into a file because prysk doesn't play well -with the square brackets miette uses for source file paths - $ ${TURBO} something > tmp.log 2>&1 - [1] - $ grep --quiet -E "root task //#something \(turbo run build\) looks like it invokes turbo and" tmp.log - $ grep --quiet -E "might cause a loop" tmp.log - $ grep --quiet -E "task found here" tmp.log - $ grep --quiet -E "\"something\": \"turbo run build\"" tmp.log +sed replaces the square brackets with parentheses so prysk can parse the file path + $ ${TURBO} something 2>&1 | sed 's/\[\([^]]*\)\]/\(\1)/g' + \xe2\x80\xa2 Packages in scope: //, another, my-app, util (esc) + \xe2\x80\xa2 Running something in 4 packages (esc) + \xe2\x80\xa2 Remote caching disabled (esc) + x root task //#something (turbo run build) looks like it invokes turbo and + | might cause a loop + ,-\(.*/package.json:3:1\) (re) + 3 | "scripts": { + 4 | "something": "turbo run build" + : ^^^^^^^^|^^^^^^^^ + : `-- task found here + 5 | }, + `---- + + + From d20bcdd5fae598dd1da7c2ecb63bdf71862d975c Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Fri, 12 Jul 2024 18:11:49 -0400 Subject: [PATCH 08/11] Fixed clippy --- .../turborepo-repository/src/package_graph/builder.rs | 5 +---- crates/turborepo-repository/src/package_manager/mod.rs | 2 +- .../turborepo-repository/src/package_manager/yarn.rs | 10 +++++++++- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/crates/turborepo-repository/src/package_graph/builder.rs b/crates/turborepo-repository/src/package_graph/builder.rs index d80a6da553caf..6adce3b9f9088 100644 --- a/crates/turborepo-repository/src/package_graph/builder.rs +++ b/crates/turborepo-repository/src/package_graph/builder.rs @@ -1,7 +1,4 @@ -use std::{ - backtrace::Backtrace, - collections::{BTreeMap, HashMap, HashSet}, -}; +use std::collections::{BTreeMap, HashMap, HashSet}; use miette::Diagnostic; use petgraph::graph::{Graph, NodeIndex}; diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index 1ed100fb5ff9d..08e8cb3208c75 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -16,8 +16,8 @@ use globwalk::{fix_glob_pattern, ValidatedGlob}; use itertools::{Either, Itertools}; use lazy_regex::{lazy_regex, Lazy}; use miette::{Diagnostic, NamedSource, SourceSpan}; -use npm::NpmDetector; use node_semver::SemverError; +use npm::NpmDetector; use regex::Regex; use serde::{Deserialize, Serialize}; use thiserror::Error; diff --git a/crates/turborepo-repository/src/package_manager/yarn.rs b/crates/turborepo-repository/src/package_manager/yarn.rs index a4188d6d4d14d..f81d31d145dc0 100644 --- a/crates/turborepo-repository/src/package_manager/yarn.rs +++ b/crates/turborepo-repository/src/package_manager/yarn.rs @@ -1,5 +1,6 @@ use std::process::Command; +use miette::NamedSource; use node_semver::{Range, Version}; use turbopath::{AbsoluteSystemPath, RelativeUnixPath}; use which::which; @@ -32,7 +33,14 @@ impl<'a> YarnDetector<'a> { .current_dir(self.repo_root) .output()?; let yarn_version_output = String::from_utf8(output.stdout)?; - Ok(yarn_version_output.trim().parse()?) + yarn_version_output + .trim() + .parse() + .map_err(|err| Error::InvalidVersion { + explanation: format!("{} {}", yarn_version_output, err), + span: None, + text: NamedSource::new("yarn --version", yarn_version_output), + }) } pub fn detect_berry_or_yarn(version: &Version) -> Result { From b834a4588f3b51a85660e8d8c01f1c725285bb2f Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Mon, 15 Jul 2024 11:22:12 -0400 Subject: [PATCH 09/11] fixed tests for windows --- turborepo-tests/integration/tests/invalid-package-json.t | 2 +- turborepo-tests/integration/tests/recursive-turbo.t | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/turborepo-tests/integration/tests/invalid-package-json.t b/turborepo-tests/integration/tests/invalid-package-json.t index 235df41d8738c..a487892d21c01 100644 --- a/turborepo-tests/integration/tests/invalid-package-json.t +++ b/turborepo-tests/integration/tests/invalid-package-json.t @@ -43,7 +43,7 @@ Add invalid packageManager field that passes the regex. x could not resolve workspaces `-> invalid semantic version: Failed to parse an integer component of a semver string: number too large to fit in target type - ,-\(.*/package.json:5:1\) (re) + ,-\(.*package.json:5:1\) (re) 5 | }, 6 | "packageManager": "npm@0.3.211111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111", : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/turborepo-tests/integration/tests/recursive-turbo.t b/turborepo-tests/integration/tests/recursive-turbo.t index b5688e486983d..910276dffda71 100644 --- a/turborepo-tests/integration/tests/recursive-turbo.t +++ b/turborepo-tests/integration/tests/recursive-turbo.t @@ -8,7 +8,7 @@ sed replaces the square brackets with parentheses so prysk can parse the file pa \xe2\x80\xa2 Remote caching disabled (esc) x root task //#something (turbo run build) looks like it invokes turbo and | might cause a loop - ,-\(.*/package.json:3:1\) (re) + ,-\(.*package.json:3:1\) (re) 3 | "scripts": { 4 | "something": "turbo run build" : ^^^^^^^^|^^^^^^^^ From 84767f913bf0ebfe51af85c6d8b3e840f65e6d32 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Wed, 17 Jul 2024 14:11:22 -0400 Subject: [PATCH 10/11] Fix test --- .../integration/tests/invalid-package-json.t | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/turborepo-tests/integration/tests/invalid-package-json.t b/turborepo-tests/integration/tests/invalid-package-json.t index a487892d21c01..cc2a586a0d4c3 100644 --- a/turborepo-tests/integration/tests/invalid-package-json.t +++ b/turborepo-tests/integration/tests/invalid-package-json.t @@ -51,18 +51,25 @@ Add invalid packageManager field that passes the regex. 7 | "workspaces": [ `---- +Restore packageManager field + $ jq '.packageManager = "npm@8.19.4"' package.json > package.json.new + $ mv package.json.new package.json + Add a trailing comma $ echo "{ \"name\": \"foobar\", }" > package.json.new $ mv package.json.new apps/my-app/package.json Build should fail due to trailing comma (sed replaces square brackets with parentheses) $ ${TURBO} build 2>&1 | sed 's/\[\([^]]*\)\]/\(\1)/g' package_json_parse_error - + x unable to parse package.json - + Error: x Expected a property but instead found '}'. ,-\(.*package.json:1:1\) (re) 1 | { "name": "foobar", } : ^ `---- + + + From 653b34e0cc287fa4c6b4880e19a9b124a7a2d2b8 Mon Sep 17 00:00:00 2001 From: nicholaslyang Date: Thu, 18 Jul 2024 13:14:21 -0400 Subject: [PATCH 11/11] Fix test --- crates/turborepo-repository/src/package_manager/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/turborepo-repository/src/package_manager/mod.rs b/crates/turborepo-repository/src/package_manager/mod.rs index 08e8cb3208c75..c0a54382af8d0 100644 --- a/crates/turborepo-repository/src/package_manager/mod.rs +++ b/crates/turborepo-repository/src/package_manager/mod.rs @@ -830,8 +830,7 @@ mod tests { ]; for case in tests { - let result = - PackageManager::parse_package_manager_string(&Spanned::new(case.package_manager)); + let result = PackageManager::parse_package_manager_string(&case.package_manager); let Ok((received_manager, received_version)) = result else { assert!(case.expected_error, "{}: received error", case.name); continue;