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.

Sign up for GitHub

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

Jump to bottom

feat: show all persistent task validation errors #5807

Merged
merged 2 commits into from
Aug 25, 2023
Merged

feat: show all persistent task validation errors #5807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e09e850
return all persistent task errors
Aug 25, 2023
994671a
sort validation errors in rust
Aug 25, 2023
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
24 changes: 12 additions & 12 deletions cli/internal/core/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,12 +454,12 @@ func (e *Engine) AddDep(fromTaskID string, toTaskID string) error {
// ValidatePersistentDependencies checks if any task dependsOn persistent tasks and throws
// an error if that task is actually implemented
func (e *Engine) ValidatePersistentDependencies(graph *graph.CompleteGraph, concurrency int) error {
var validationError error
var validationErrors []string
persistentCount := 0

// Adding in a lock because otherwise walking the graph can introduce a data race
// (reproducible with `go test -race`)
var sema = util.NewSemaphore(1)
var mu sync.Mutex
Copy link
Member Author

Choose a reason for hiding this comment

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

This wasn't strictly necessary, but we really just want a mutex guarding the validation errors and not a semaphore.


errs := e.TaskGraph.Walk(func(v dag.Vertex) error {
vertexName := dag.VertexName(v) // vertexName is a taskID
Expand All @@ -469,12 +469,6 @@ func (e *Engine) ValidatePersistentDependencies(graph *graph.CompleteGraph, conc
return nil
}

// Aquire a lock, because otherwise walking this group can cause a race condition
// writing to the same validationError var defined outside the Walk(). This shows
// up when running tests with the `-race` flag.
sema.Acquire()
defer sema.Release()

currentTaskDefinition, currentTaskExists := e.completeGraph.TaskDefinitions[vertexName]
if currentTaskExists && currentTaskDefinition.Persistent {
persistentCount++
Expand Down Expand Up @@ -511,11 +505,16 @@ func (e *Engine) ValidatePersistentDependencies(graph *graph.CompleteGraph, conc

// If both conditions are true set a value and break out of checking the dependencies
if depTaskDefinition.Persistent && hasScript {
validationError = fmt.Errorf(
// Aquire a lock, because otherwise walking this group can cause a race condition
// writing to the same validationErrors var defined outside the Walk(). This shows
// up when running tests with the `-race` flag.
mu.Lock()
defer mu.Unlock()
Comment on lines +511 to +512
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the locking/unlocking to only where we write to some shared state

validationErrors = append(validationErrors, fmt.Sprintf(
"\"%s\" is a persistent task, \"%s\" cannot depend on it",
util.GetTaskId(packageName, taskName),
util.GetTaskId(currentPackageName, currentTaskName),
)
))

break
}
Expand All @@ -528,8 +527,9 @@ func (e *Engine) ValidatePersistentDependencies(graph *graph.CompleteGraph, conc
return fmt.Errorf("Validation failed: %v", err)
}

if validationError != nil {
return validationError
if len(validationErrors) > 0 {
sort.Strings(validationErrors)
return fmt.Errorf("%s", strings.Join(validationErrors, "\n"))
} else if persistentCount >= concurrency {
return fmt.Errorf("You have %v persistent tasks but `turbo` is configured for concurrency of %v. Set --concurrency to at least %v", persistentCount, concurrency, persistentCount+1)
}
Expand Down
6 changes: 5 additions & 1 deletion crates/turborepo-lib/src/run/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,11 @@ impl Run {
.map_err(|errors| {
anyhow!(
"error preparing engine: Invalid persistent task configuration:\n{}",
errors.into_iter().join("\n")
errors
.into_iter()
.map(|e| e.to_string())
.sorted()
Copy link
Member Author

Choose a reason for hiding this comment

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

Now the Rust version is deterministic

.join("\n")
)
})?;

Expand Down
6 changes: 4 additions & 2 deletions turborepo-tests/integration/tests/persistent_dependencies/3-workspace-specific.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
# The regex match is liberal, because the build task from either workspace can throw the error
$ ${TURBO} run build
ERROR run failed: error preparing engine: Invalid persistent task configuration:
"pkg-a#dev" is a persistent task, .*-a#build" cannot depend on it (re)
"pkg-a#dev" is a persistent task, "app-a#build" cannot depend on it
"pkg-a#dev" is a persistent task, "pkg-a#build" cannot depend on it
Turbo error: error preparing engine: Invalid persistent task configuration:
"pkg-a#dev" is a persistent task, .*-a#build" cannot depend on it (re)
"pkg-a#dev" is a persistent task, "app-a#build" cannot depend on it
"pkg-a#dev" is a persistent task, "pkg-a#build" cannot depend on it
[1]
Loading