Skip to content

Commit

Permalink
feat(turborepo): Convert even more errors (#7513)
Browse files Browse the repository at this point in the history
### Description

Converting more errors to be pretty. Involves some plumbing and
bookkeeping.

### Testing Instructions

Current tests around task validation should suffice.


Closes TURBO-2433

---------

Co-authored-by: Mehul Kar <mehul.kar@vercel.com>
  • Loading branch information
NicholasLYang and mehulkar committed Feb 28, 2024
1 parent f790c14 commit 77c4713
Show file tree
Hide file tree
Showing 17 changed files with 222 additions and 62 deletions.
1 change: 1 addition & 0 deletions crates/turborepo-lib/src/cli/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub enum Error {
#[error(transparent)]
Generate(#[from] generate::Error),
#[error(transparent)]
#[diagnostic(transparent)]
Prune(#[from] prune::Error),
#[error(transparent)]
PackageJson(#[from] turborepo_repository::package_json::Error),
Expand Down
4 changes: 3 additions & 1 deletion crates/turborepo-lib/src/commands/prune.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use std::os::unix::fs::PermissionsExt;
use std::sync::OnceLock;

use lazy_static::lazy_static;
use miette::Diagnostic;
use tracing::trace;
use turbopath::{
AbsoluteSystemPathBuf, AnchoredSystemPath, AnchoredSystemPathBuf, RelativeUnixPath,
Expand All @@ -19,7 +20,7 @@ use crate::turbo_json::RawTurboJson;

pub const DEFAULT_OUTPUT_DIR: &str = "out";

#[derive(Debug, thiserror::Error)]
#[derive(Debug, thiserror::Error, Diagnostic)]
pub enum Error {
#[error("io error while pruning: {0}")]
Io(#[from] std::io::Error),
Expand All @@ -30,6 +31,7 @@ pub enum Error {
#[error("path error while pruning: {0}")]
Path(#[from] turbopath::PathError),
#[error(transparent)]
#[diagnostic(transparent)]
TurboJsonParser(#[from] crate::turbo_json::parser::Error),
#[error(transparent)]
PackageJson(#[from] turborepo_repository::package_json::Error),
Expand Down
69 changes: 53 additions & 16 deletions crates/turborepo-lib/src/engine/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,24 @@ pub enum Error {
#[source_code]
text: NamedSource,
},
#[error("Could not find workspace \"{package}\" from task \"{task_id}\" in project")]
MissingWorkspaceFromTask { package: String, task_id: String },
#[error("Could not find \"{task_id}\" in root turbo.json or \"{task_name}\" in workspace")]
MissingWorkspaceTask { task_id: String, task_name: String },
#[error("Could not find package \"{package}\" from task \"{task_id}\" in project")]
MissingPackageFromTask {
#[label]
span: Option<SourceSpan>,
#[source_code]
text: NamedSource,
package: String,
task_id: String,
},
#[error("Could not find \"{task_id}\" in root turbo.json or \"{task_name}\" in package")]
MissingPackageTask {
#[label]
span: Option<SourceSpan>,
#[source_code]
text: NamedSource,
task_id: String,
task_name: String,
},
#[error(transparent)]
#[diagnostic(transparent)]
Config(#[from] crate::config::Error),
Expand All @@ -64,8 +78,15 @@ pub enum Error {
},
#[error(transparent)]
Graph(#[from] graph::Error),
#[error("Invalid task name {task_name}: {reason}")]
InvalidTaskName { task_name: String, reason: String },
#[error("invalid task name: {reason}")]
InvalidTaskName {
#[label]
span: Option<SourceSpan>,
#[source_code]
text: NamedSource,
task_name: String,
reason: String,
},
}

pub struct EngineBuilder<'a> {
Expand Down Expand Up @@ -160,7 +181,8 @@ impl<'a> EngineBuilder<'a> {
// workspace is acceptable)
if !matches!(workspace, PackageName::Root) || self.root_enabled_tasks.contains(task)
{
traversal_queue.push_back(task.to(task_id));
let task_id = task.to(task_id);
traversal_queue.push_back(task_id);
}
}
}
Expand All @@ -187,6 +209,11 @@ impl<'a> EngineBuilder<'a> {
let mut engine = Engine::default();

while let Some(task_id) = traversal_queue.pop_front() {
{
let (task_id, span) = task_id.clone().split();
engine.add_task_location(task_id.into_owned(), span);
}

if task_id.package() == ROOT_PKG_NAME
&& !self
.root_enabled_tasks
Expand All @@ -200,7 +227,7 @@ impl<'a> EngineBuilder<'a> {
});
}

validate_task_name(task_id.task())?;
validate_task_name(task_id.to(task_id.task()))?;

if task_id.package() != ROOT_PKG_NAME
&& self
Expand All @@ -210,9 +237,12 @@ impl<'a> EngineBuilder<'a> {
{
// If we have a pkg it should be in PackageGraph.
// If we're hitting this error something has gone wrong earlier when building
// PackageGraph or the workspace really doesn't exist and
// PackageGraph or the package really doesn't exist and
// turbo.json is misconfigured.
return Err(Error::MissingWorkspaceFromTask {
let (span, text) = task_id.span_and_text("turbo.json");
return Err(Error::MissingPackageFromTask {
span,
text,
package: task_id.package().to_string(),
task_id: task_id.to_string(),
});
Expand Down Expand Up @@ -275,7 +305,8 @@ impl<'a> EngineBuilder<'a> {
engine
.task_graph
.add_edge(to_task_index, from_task_index, ());
traversal_queue.push_back(span.to(from_task_id));
let from_task_id = span.to(from_task_id);
traversal_queue.push_back(from_task_id);
}
});

Expand All @@ -289,11 +320,11 @@ impl<'a> EngineBuilder<'a> {
engine
.task_graph
.add_edge(to_task_index, from_task_index, ());
traversal_queue.push_back(span.to(from_task_id));
let from_task_id = span.to(from_task_id);
traversal_queue.push_back(from_task_id);
}

engine.add_definition(task_id.as_inner().clone().into_owned(), task_definition);

if !has_deps && !has_topo_deps {
engine.connect_to_root(&to_task_id);
}
Expand Down Expand Up @@ -395,7 +426,10 @@ impl<'a> EngineBuilder<'a> {
}

if task_definitions.is_empty() {
return Err(Error::MissingWorkspaceTask {
let (span, text) = task_id.span_and_text("turbo.json");
return Err(Error::MissingPackageTask {
span,
text,
task_id: task_id.to_string(),
task_name: task_name.to_string(),
});
Expand Down Expand Up @@ -447,12 +481,15 @@ impl Error {
// we can expand the patterns here.
const INVALID_TOKENS: &[&str] = &["$colon$"];

fn validate_task_name(task: &str) -> Result<(), Error> {
fn validate_task_name(task: Spanned<&str>) -> Result<(), Error> {
INVALID_TOKENS
.iter()
.find(|token| task.contains(**token))
.map(|found_token| {
let (span, text) = task.span_and_text("turbo.json");
Err(Error::InvalidTaskName {
span,
text,
task_name: task.to_string(),
reason: format!("task contains invalid string '{found_token}'"),
})
Expand Down Expand Up @@ -1106,7 +1143,7 @@ mod test {
#[test_case("build:prod", None)]
#[test_case("build$colon$prod", Some("task contains invalid string '$colon$'"))]
fn test_validate_task_name(task_name: &str, reason: Option<&str>) {
let result = validate_task_name(task_name)
let result = validate_task_name(Spanned::new(task_name))
.map_err(|e| {
if let Error::InvalidTaskName { reason, .. } = e {
reason
Expand Down
32 changes: 31 additions & 1 deletion crates/turborepo-lib/src/engine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ use std::{

pub use builder::{EngineBuilder, Error as BuilderError};
pub use execute::{ExecuteError, ExecutionOptions, Message, StopExecution};
use miette::Diagnostic;
use miette::{Diagnostic, NamedSource, SourceSpan};
use petgraph::Graph;
use thiserror::Error;
use turborepo_errors::Spanned;
use turborepo_repository::package_graph::{PackageGraph, PackageName};

use crate::{run::task_id::TaskId, task_graph::TaskDefinition};
Expand Down Expand Up @@ -42,6 +43,7 @@ pub struct Engine<S = Built> {
root_index: petgraph::graph::NodeIndex,
task_lookup: HashMap<TaskId<'static>, petgraph::graph::NodeIndex>,
task_definitions: HashMap<TaskId<'static>, TaskDefinition>,
task_locations: HashMap<TaskId<'static>, Spanned<()>>,
}

impl Engine<Building> {
Expand All @@ -54,6 +56,7 @@ impl Engine<Building> {
root_index,
task_lookup: HashMap::default(),
task_definitions: HashMap::default(),
task_locations: HashMap::default(),
}
}

Expand All @@ -78,13 +81,27 @@ impl Engine<Building> {
self.task_definitions.insert(task_id, definition)
}

pub fn add_task_location(&mut self, task_id: TaskId<'static>, location: Spanned<()>) {
// If we don't have the location stored,
// or if the location stored is empty, we add it to the map.
let has_location = self
.task_locations
.get(&task_id)
.map_or(false, |existing| existing.range.is_some());

if !has_location {
self.task_locations.insert(task_id, location);
}
}

// Seals the task graph from being mutated
pub fn seal(self) -> Engine<Built> {
let Engine {
task_graph,
task_lookup,
root_index,
task_definitions,
task_locations,
..
} = self;
Engine {
Expand All @@ -93,6 +110,7 @@ impl Engine<Building> {
task_lookup,
root_index,
task_definitions,
task_locations,
}
}
}
Expand Down Expand Up @@ -192,7 +210,15 @@ impl Engine<Built> {
if task_definition.persistent
&& package_json.scripts.contains_key(dep_id.task())
{
let (span, text) = self
.task_locations
.get(dep_id)
.map(|spanned| spanned.span_and_text("turbo.json"))
.unwrap_or((None, NamedSource::new("", "")));

return Err(ValidateError::DependencyOnPersistentTask {
span,
text,
persistent_task: dep_id.to_string(),
dependant: task_id.to_string(),
});
Expand Down Expand Up @@ -254,6 +280,10 @@ pub enum ValidateError {
MissingPackageJson { package: String },
#[error("\"{persistent_task}\" is a persistent task, \"{dependant}\" cannot depend on it")]
DependencyOnPersistentTask {
#[label("persistent task")]
span: Option<SourceSpan>,
#[source_code]
text: NamedSource,
persistent_task: String,
dependant: String,
},
Expand Down
8 changes: 5 additions & 3 deletions crates/turborepo-lib/src/run/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ use turborepo_repository::package_graph;

use super::graph_visualizer;
use crate::{
config, daemon, engine, opts,
config, daemon, engine,
engine::ValidateError,
opts,
run::{global_hash, scope},
task_graph, task_hash,
};

#[derive(Debug, Error, Diagnostic)]
pub enum Error {
#[error("error preparing engine: Invalid persistent task configuration:\n{0}")]
EngineValidation(String),
#[error("invalid persistent task configuration")]
EngineValidation(#[related] Vec<ValidateError>),
#[error(transparent)]
Graph(#[from] graph_visualizer::Error),
#[error(transparent)]
Expand Down
11 changes: 1 addition & 10 deletions crates/turborepo-lib/src/run/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use std::{

pub use cache::{ConfigCache, RunCache, TaskCache};
use chrono::{DateTime, Local};
use itertools::Itertools;
use rayon::iter::ParallelBridge;
use tracing::debug;
use turbopath::{AbsoluteSystemPathBuf, AnchoredSystemPath};
Expand Down Expand Up @@ -586,15 +585,7 @@ impl Run {
if !self.opts.run_opts.parallel {
engine
.validate(pkg_dep_graph, self.opts.run_opts.concurrency)
.map_err(|errors| {
Error::EngineValidation(
errors
.into_iter()
.map(|e| e.to_string())
.sorted()
.join("\n"),
)
})?;
.map_err(Error::EngineValidation)?;
}

Ok(engine)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,15 @@
// app-a#dev
// └── pkg-a#dev
$ ${TURBO} run dev
x error preparing engine: Invalid persistent task configuration:
| "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it
x invalid persistent task configuration

Error: x "pkg-a#dev" is a persistent task, "app-a#dev" cannot depend on it
,-[turbo.json:4:1]
4 | "dev": {
5 | "dependsOn": ["^dev"],
: ^^^|^^
: `-- persistent task
6 | "persistent": true
`----

[1]
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,17 @@
$ . ${TESTDIR}/../../../helpers/setup_integration_test.sh persistent_dependencies/10-too-many

$ ${TURBO} run build --concurrency=1
x error preparing engine: Invalid persistent task configuration:
| You have 2 persistent tasks but `turbo` is configured for concurrency of
x invalid persistent task configuration

Error: 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 error preparing engine: Invalid persistent task configuration:
| You have 2 persistent tasks but `turbo` is configured for concurrency of
x invalid persistent task configuration

Error: x You have 2 persistent tasks but `turbo` is configured for concurrency of
| 2. Set --concurrency to at least 3

[1]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,15 @@
// └── app-a#dev
//
$ ${TURBO} run build
x error preparing engine: Invalid persistent task configuration:
| "app-a#dev" is a persistent task, "app-a#build" cannot depend on it
x invalid persistent task configuration

Error: x "app-a#dev" is a persistent task, "app-a#build" cannot depend on it
,-[turbo.json:4:1]
4 | "build": {
5 | "dependsOn": ["dev"]
: ^^|^^
: `-- persistent task
6 | },
`----

[1]

0 comments on commit 77c4713

Please sign in to comment.