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: Skip over repo root package for package inferrence #3503

Conversation

chris-olszewski
Copy link
Member

Once again map iteration is the culprit. The failure of infer_pkg.t is caused by package inference iterating over the root package before the correct candidate package. This results in pkgPath == repoRoot and since fullInferencePath is necessarily contained in the repo root, the root package will always be selected as the exact package. The fix is just skipping over the root package.

First commit adds a root package to the tests. The package inference cases will fail in a non-deterministic fashion since this bug depends on the repo root package being iterated over before the package that should be inferred. The next commit is a fix.

@vercel
Copy link

vercel bot commented Jan 26, 2023

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

Name Status Preview Comments Updated
examples-basic-web 🔄 Building (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-designsystem-docs 🔄 Building (Inspect) Jan 26, 2023 at 11:19PM (UTC)
turbo-site ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 26, 2023 at 11:19PM (UTC)
7 Ignored Deployments
Name Status Preview Comments Updated
examples-cra-web ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-kitchensink-blog ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-native-web ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-nonmonorepo ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-svelte-web ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-tailwind-web ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)
examples-vite-web ⬜️ Ignored (Inspect) Jan 26, 2023 at 11:19PM (UTC)

@vercel
Copy link

vercel bot commented Jan 26, 2023

@chris-olszewski is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

@chris-olszewski chris-olszewski enabled auto-merge (squash) January 26, 2023 23:25
Copy link
Contributor

@mehulkar mehulkar left a comment

Choose a reason for hiding this comment

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

Looks good to me

"//": {
Dir: turbopath.AnchoredSystemPath("").ToSystemPath(),
Name: "monorepo",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need a test helper to construct all the internal bits that make up a valid monorepo representation... it's quite a pain to dig through the code to do this right. There's a beginning of one here: https://github.com/vercel/turbo/blob/3bbbc471d3f8b9f6c141d620ae6f18dd206fd8cd/cli/internal/core/engine_persistent_deps_test.go#L448-L449

@@ -165,7 +165,8 @@ func calculateInference(repoRoot turbopath.AbsoluteSystemPath, rawPkgInferenceDi
if err != nil {
return nil, err
}
if inferredPathIsBelow {
// We skip over the root package as the inferred path will always be below it
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this feature, but is this true? If you run turbo run from the root of the repo, will inference be skipped altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

turbo run from repo root will cause rawPkgInferenceDir to be "" which triggers a return at the beginning of this function

@chris-olszewski chris-olszewski merged commit 77afb8a into vercel:main Jan 26, 2023
@mehulkar mehulkar mentioned this pull request Jan 27, 2023
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.

3 participants