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

Extract ResolvePackages #830

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 11 commits into from
Mar 8, 2022
Merged

Extract ResolvePackages #830

merged 11 commits into from
Mar 8, 2022

Conversation

gsoltis
Copy link
Contributor

@gsoltis gsoltis commented Mar 7, 2022

  • Move package resolution logic to scope package
  • Remove unused functionality from SCM interface to make it easier to mock in tests
  • Start adding tests for scope resolution

Review Note:
scope_test.go provides examples of parsed flags and how they translate to packages considered in-scope. I've done my best to infer what packages should be built in each scenario, but please verify. Also very happy to accept additions to that test.

Possibly controversial change:
To maintain backwards compatibility, specifying --since and --scope to turbo run will imply --include-dependencies. This means that using scope will generally cause all dependencies of the specified scopes to be built, although the cache will still be used so this may potentially be cheap.

Previously, this behavior was triggered when --scope and --since constraints produced packages to be built. In cases where they were both specified but one or both did not produce packages to be built, dependencies were not included.

With this change, the behavior will be consistent based on commandline flags and default to building dependencies. With an upcoming switch over to cobra for commandline parsing, we should be able to flag off building dependencies if the user requests it.

@vercel
Copy link

vercel bot commented Mar 7, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/mLNofRE3WeCn3xouzGxU89xEPjxV
✅ Preview: https://turbo-site-git-gsoltis-extractresolvepackages.vercel.sh

@gsoltis
Copy link
Contributor Author

gsoltis commented Mar 7, 2022

Two of the tests I've added currently fail, but I think they should pass.

{
// a non-dependent lib changed
changed: []string{"libs/libC/src/index.ts"},
expected: []string{},
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 this is correct, given that we've supplied a scope which does not depend on the changed package. However our current scope resolution logic includes all changed packages in this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

{
changed: []string{"libs/libB/src/index.ts"},
// expect everything, global changed, no scope
expected: []string{"app1", "app2", "libA", "libB", "libC"},
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 this is correct because there was a global change. A global change should imply a build of everything in scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed

jaredpalmer
jaredpalmer previously approved these changes Mar 7, 2022
includeDependents = true

}
tui.Output(fmt.Sprintf(ui.Dim("• Packages changed since %s in scope: %s"), opts.Since, strings.Join(filteredPkgs.UnsafeListOfStrings(), ", ")))
Copy link
Contributor

Choose a reason for hiding this comment

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

in the --plan PR we can abstract this into returning some state variable and printing JSON output.

@@ -919,6 +809,9 @@ func parseRunArgs(args []string, output cli.Ui) (*RunOptions, error) {
}
}
}
if len(runOptions.scope) != 0 && runOptions.since != "" && !includDepsSet {
Copy link
Contributor

@jaredpalmer jaredpalmer Mar 7, 2022

Choose a reason for hiding this comment

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

why not just --scope or --since?

@jaredpalmer jaredpalmer self-requested a review March 7, 2022 22:55
@@ -829,6 +712,11 @@ func parseRunArgs(args []string, output cli.Ui) (*RunOptions, error) {

unresolvedCacheFolder := filepath.FromSlash("./node_modules/.cache/turbo")

// --scope and --since implies --include-dependencies for backwards compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be --scope or --since?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--since-only was previously handled here: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L250
It does not pull in dependencies.

--scope-only was previously handled here: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L253
It also does not pull in dependencies.

It was only when they were both specified, and there were actual changed packages, that dependencies were included: https://github.com/vercel/turborepo/blob/main/cli/internal/run/run.go#L238
This amounts to building all changed packages and their dependencies, regardless of specified scope. I don't think this behavior is correct, but I'm not sure if we want to stop building dependencies in that case since users might be relying on it? Although they are somewhat arbitrary...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, what I'm saying is that I think in addition to these changes, passing either --scope or --since should automatically --include-dependencies and --no-deps flags (breaking behavior).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, gotcha. Yeah, we can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned here, we're going to keep the behavior in this PR, without expanding it to "--scope or --since triggers --include-dependencies. This closely mirrors existing behavior. We can then look at alternate ways of specifying packages.

@kodiakhq kodiakhq bot merged commit 2cfa935 into main Mar 8, 2022
@kodiakhq kodiakhq bot deleted the gsoltis/extract_resolve_packages branch March 8, 2022 22:25
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.

2 participants