Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(query): remove engine validation for query #9271

Merged
merged 2 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion crates/turborepo-lib/src/commands/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ pub async fn run(
execution_args: Box::default(),
});

let run_builder = RunBuilder::new(base)?.add_all_tasks();
let run_builder = RunBuilder::new(base)?
.add_all_tasks()
.do_not_validate_engine();
let run = run_builder.build(&handler, telemetry).await?;

if let Some(query) = query {
Expand Down
9 changes: 8 additions & 1 deletion crates/turborepo-lib/src/engine/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ pub struct EngineBuilder<'a> {
root_enabled_tasks: HashSet<TaskName<'static>>,
tasks_only: bool,
add_all_tasks: bool,
should_validate_engine: bool,
}

impl<'a> EngineBuilder<'a> {
Expand All @@ -120,6 +121,7 @@ impl<'a> EngineBuilder<'a> {
root_enabled_tasks: HashSet::new(),
tasks_only: false,
add_all_tasks: false,
should_validate_engine: true,
}
}

Expand Down Expand Up @@ -157,6 +159,11 @@ impl<'a> EngineBuilder<'a> {
self
}

pub fn do_not_validate_engine(mut self) -> Self {
self.should_validate_engine = false;
self
}

// Returns the set of allowed tasks that can be run if --only is used
// The set is exactly the product of the packages in filter and tasks specified
// by CLI
Expand Down Expand Up @@ -502,7 +509,7 @@ impl<'a> EngineBuilder<'a> {
}
}

if task_definitions.is_empty() {
if task_definitions.is_empty() && self.should_validate_engine {
let (span, text) = task_id.span_and_text("turbo.json");
return Err(Error::MissingPackageTask {
span,
Expand Down
14 changes: 13 additions & 1 deletion crates/turborepo-lib/src/run/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ pub struct RunBuilder {
should_print_prelude_override: Option<bool>,
allow_missing_package_manager: bool,
allow_no_turbo_json: bool,
// In query, we don't want to validate the engine. Defaults to `true`
should_validate_engine: bool,
// If true, we will add all tasks to the graph, even if they are not specified
add_all_tasks: bool,
}
Expand Down Expand Up @@ -111,6 +113,7 @@ impl RunBuilder {
allow_missing_package_manager,
root_turbo_json_path,
allow_no_turbo_json,
should_validate_engine: true,
add_all_tasks: false,
})
}
Expand All @@ -130,6 +133,11 @@ impl RunBuilder {
self
}

pub fn do_not_validate_engine(mut self) -> Self {
self.should_validate_engine = false;
self
}

fn connect_process_manager(&self, signal_subscriber: SignalSubscriber) {
let manager = self.processes.clone();
tokio::spawn(async move {
Expand Down Expand Up @@ -496,6 +504,10 @@ impl RunBuilder {
builder = builder.add_all_tasks();
}

if !self.should_validate_engine {
builder = builder.do_not_validate_engine();
}

let mut engine = builder.build()?;

// If we have an initial task, we prune out the engine to only
Expand All @@ -504,7 +516,7 @@ impl RunBuilder {
engine = engine.create_engine_for_subgraph(entrypoint_packages);
}

if !self.opts.run_opts.parallel {
if !self.opts.run_opts.parallel && self.should_validate_engine {
engine
.validate(
pkg_dep_graph,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "app-a",
"scripts": {
"build": "echo 'build app-a'",
"test": "echo 'test app-a'"
},
"dependencies": {
"lib-a": "*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "app-b",
"scripts": {
"build": "echo 'build app-b'",
"test": "echo 'test app-b'"
},
"dependencies": {
"lib-b": "*",
"lib-c": "*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "lib-a",
"scripts": {
"build": "echo 'build lib-a'",
"test": "echo 'test lib-a'"
},
"dependencies": {
"lib-b": "*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "lib-b",
"scripts": {
"build": "echo 'build lib-b'",
"test": "echo 'test lib-b'"
},
"dependencies": {
"lib-d": "*"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"name": "lib-c",
"scripts": {
"build": "echo 'build lib-c'"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "lib-d",
"scripts": {
"build": "echo 'build lib-d'",
"test": "echo 'test lib-d'"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"name": "unknown-dependency",
"workspaces": [
"app-a",
"app-b",
"lib-a",
"lib-b",
"lib-c",
"lib-d"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{
"tasks": {
"build0": {
"dependsOn": [
"^build0",
"prepare"
]
},
"test": {
"dependsOn": [
"^build0",
"prepare"
]
},
"prepare": {},
"side-quest": {
"dependsOn": [
"prepare"
]
},
"build1": {
"dependsOn": [
"^build1"
]
},
"build2": {
"dependsOn": [
"app-a#custom"
]
}
}
}
148 changes: 148 additions & 0 deletions turborepo-tests/integration/tests/query/validation.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
Setup
$ . ${TESTDIR}/../../../helpers/setup_integration_test.sh persistent_dependencies/10-too-many

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

[1]

However on query, we ignore this validation
$ ${TURBO} query "query { packages { items { tasks { items { fullName } } } } }" | jq
WARNING query command is experimental and may change in the future
{
"data": {
"packages": {
"items": [
{
"tasks": {
"items": []
}
},
{
"tasks": {
"items": [
{
"fullName": "one#build"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "two#build"
}
]
}
}
]
}
}
}

Setup
$ . ${TESTDIR}/../../../helpers/setup_integration_test.sh task_dependencies/invalid-dependency
warning: re-init: ignored --initial-branch=main

Validate that we get an error when trying to depend on a task that doesn't exist
$ ${TURBO} run build2
x Could not find "app-a#custom" in root turbo.json or "custom" in package
,-[turbo.json:27:1]
27 | "dependsOn": [
28 | "app-a#custom"
: ^^^^^^^^^^^^^^
29 | ]
`----

[1]

However, we don't get an error when we query
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you since you're the one driving query, but returning information that we know is incorrect feels off to me. Do we need to worry about panics down the line in query around assumptions that every task has a valid package and has valid task dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's necessary so people can use query to help refactor their repos into valid configurations for turbo. But yeah, we will have to watch out for panics. I have a larger plan to switch us to a compiler-y error model where we accumulate diagnostics and try to recover from errors as much as possible.

$ ${TURBO} query "query { packages { items { tasks { items { fullName } } } } }" | jq
WARNING query command is experimental and may change in the future
{
"data": {
"packages": {
"items": [
{
"tasks": {
"items": []
}
},
{
"tasks": {
"items": [
{
"fullName": "app-a#build"
},
{
"fullName": "app-a#test"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "app-b#build"
},
{
"fullName": "app-b#test"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "lib-a#build"
},
{
"fullName": "lib-a#test"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "lib-b#build"
},
{
"fullName": "lib-b#test"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "lib-c#build"
}
]
}
},
{
"tasks": {
"items": [
{
"fullName": "lib-d#build"
},
{
"fullName": "lib-d#test"
}
]
}
}
]
}
}
}
Loading