这是indexloc提供的服务,不要输入任何密码
Skip to content

Add self-ref check when build task graph #1141

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

Merged
merged 6 commits into from
May 10, 2022

Conversation

Kingwl
Copy link
Contributor

@Kingwl Kingwl commented Apr 28, 2022

Fixed #1117

@vercel
Copy link

vercel bot commented Apr 28, 2022

Someone is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@Kingwl Kingwl force-pushed the fix/avoid-task-self-ref branch 2 times, most recently from e39ca16 to 81ac2dd Compare April 28, 2022 08:55
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

This is the right check, but there are two graphs: the package-dependency graph, and the package-task graph. You're adding self-referential checks to the package-task graph, and we have cycle detection on the package-dependency graph.

How would you feel about making a utility function that does cycle + self-referential checks and applying it to both? I've linked to the other one in a comment, and I think the utility function would be the combination of the two checks (almost exactly dag.Validate, except the root check)

@@ -376,6 +376,12 @@ func buildTaskGraph(topoGraph *dag.AcyclicGraph, pipeline fs.Pipeline, rs *runSp
}); err != nil {
return nil, err
}

for _, e := range engine.TaskGraph.Edges() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add the cycle validation here as well? You can find an example here

@Kingwl Kingwl force-pushed the fix/avoid-task-self-ref branch from 418cbe5 to c6e66ce Compare April 29, 2022 08:35
@Kingwl Kingwl force-pushed the fix/avoid-task-self-ref branch from c6e66ce to deac761 Compare April 29, 2022 08:36
gsoltis
gsoltis previously approved these changes May 2, 2022
Copy link
Contributor

@gsoltis gsoltis left a comment

Choose a reason for hiding this comment

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

Awesome! Can you fix merge conflicts/lint and we'll get this merged?

@gsoltis
Copy link
Contributor

gsoltis commented May 5, 2022

Looks like ValidateGraph still needs a comment

@gsoltis
Copy link
Contributor

gsoltis commented May 9, 2022

@Kingwl it looks like there is one minor lint issue to be taken care of and then we can merge this. I'm also happy to take over the PR if need be.

@vercel
Copy link

vercel bot commented May 10, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
turbo-site ✅ Ready (Inspect) Visit Preview May 10, 2022 at 6:19PM (UTC)

@kodiakhq kodiakhq bot merged commit ac22e47 into vercel:main May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turborepo hangs when a package depends on itself
2 participants