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

fix: Skip over repo root package for package inferrence #3503

Merged
merged 2 commits into from
Jan 26, 2023
Merged

fix: Skip over repo root package for package inferrence #3503

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
860253f
add root package causing infer failures
chris-olszewski Jan 26, 2023
e0ca84b
fix root discovered as inferred package
chris-olszewski Jan 26, 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
3 changes: 2 additions & 1 deletion cli/internal/scope/scope.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

if inferredPathIsBelow && pkgPath != repoRoot {
// set both. The user might have set a parent directory filter,
// in which case we *should* fail to find any packages, but we should
// do so in a consistent manner
Expand Down
18 changes: 11 additions & 7 deletions cli/internal/scope/scope_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ func TestResolvePackages(t *testing.T) {
graph.Connect(dag.BasicEdge("app2", "libC"))
graph.Connect(dag.BasicEdge("app2-a", "libC"))
workspaceInfos := internalGraph.WorkspaceInfos{
"//": {
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

"app0": {
Dir: turbopath.AnchoredUnixPath("app/app0").ToSystemPath(),
Name: "app0",
Expand Down Expand Up @@ -142,37 +146,37 @@ func TestResolvePackages(t *testing.T) {
{
name: "Only turbo.json changed",
changed: []string{"turbo.json"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
includeDependencies: true,
},
{
name: "Only root package.json changed",
changed: []string{"package.json"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
includeDependencies: true,
},
{
name: "Only package-lock.json changed",
changed: []string{"package-lock.json"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
includeDependencies: true,
lockfile: "package-lock.json",
},
{
name: "Only yarn.lock changed",
changed: []string{"yarn.lock"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
includeDependencies: true,
lockfile: "yarn.lock",
},
{
name: "Only pnpm-lock.yaml changed",
changed: []string{"pnpm-lock.yaml"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
includeDependencies: true,
lockfile: "pnpm-lock.yaml",
Expand Down Expand Up @@ -234,7 +238,7 @@ func TestResolvePackages(t *testing.T) {
{
name: "global dependency changed, even though it was ignored, forcing a build of everything",
changed: []string{"libs/libB/src/index.ts"},
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
since: "dummy",
ignore: "libs/libB/**/*.ts",
globalDeps: []string{"libs/**/*.ts"},
Expand All @@ -257,7 +261,7 @@ func TestResolvePackages(t *testing.T) {
// no changes, no base to compare against, defaults to everything
name: "no changes or scope specified, build everything",
since: "",
expected: []string{"app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"},
expectAllPackages: true,
},
{
Expand Down