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

feat: Use lockfiles when determining what packages changed across commits #3250

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 17 additions & 0 deletions cli/integration_tests/lockfile_aware_caching/berry.t
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ Only b should have a cache miss
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "bump lockfile" --quiet
Only root and b should be rebuilt since only the deps for b had a version bump
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages"
[
"//",
"b"
]

Bump of root workspace invalidates all packages
$ patch yarn.lock turbo-bump.patch
Expand All @@ -79,3 +87,12 @@ Bump of root workspace invalidates all packages
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "global lockfile change" --quiet
Everything should be rebuilt as a dependency of the root package got bumped
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort"
[
"//",
"a",
"b"
]
17 changes: 17 additions & 0 deletions cli/integration_tests/lockfile_aware_caching/npm.t
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ Only b should have a cache miss
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "bump lockfile" --quiet
Only root and b should be rebuilt since only the deps for b had a version bump
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages"
[
"//",
"b"
]

Bump of root workspace invalidates all packages
$ patch package-lock.json turbo-bump.patch
Expand Down Expand Up @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "global lockfile change" --quiet
Everything should be rebuilt as a dependency of the root package got bumped
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort"
[
"//",
"a",
"b"
]
17 changes: 17 additions & 0 deletions cli/integration_tests/lockfile_aware_caching/pnpm.t
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ Only b should have a cache miss
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "bump pnpm-lock" --quiet
Only root and b should be rebuilt since only the deps for b had a version bump
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages"
[
"//",
"b"
]

Bump of root workspace invalidates all packages
$ patch pnpm-lock.yaml turbo-bump.patch
Expand Down Expand Up @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "global lockfile change" --quiet
Everything should be rebuilt as a dependency of the root package got bumped
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort"
[
"//",
"a",
"b"
]
2 changes: 2 additions & 0 deletions cli/integration_tests/lockfile_aware_caching/setup.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ git init ${TARGET_DIR} --quiet
GIT_ARGS="--git-dir=${TARGET_DIR}/.git --work-tree=${TARGET_DIR}"
git ${GIT_ARGS} config user.email "turbo-test@example.com"
git ${GIT_ARGS} config user.name "Turbo Test"
echo ".turbo" >> ${TARGET_DIR}/.gitignore
echo "node_modules" >> ${TARGET_DIR}/.gitignore
git ${GIT_ARGS} add .
git ${GIT_ARGS} commit -m "Initial" --quiet

17 changes: 17 additions & 0 deletions cli/integration_tests/lockfile_aware_caching/yarn.t
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,14 @@ Only b should have a cache miss
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "bump lockfile" --quiet
Only root and b should be rebuilt since only the deps for b had a version bump
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages"
[
"//",
"b"
]

Bump of root workspace invalidates all packages
$ patch yarn.lock turbo-bump.patch
Expand Down Expand Up @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages
Cached: 0 cached, 1 total
Time:\s*[\.0-9]+m?s (re)

Add lockfile changes to a commit
$ git add . && git commit -m "global lockfile change" --quiet
Everything should be rebuilt as a dependency of the root package got bumped
$ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort"
[
"//",
"a",
"b"
]
135 changes: 99 additions & 36 deletions cli/internal/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,7 @@ func BuildPackageGraph(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *f
}

func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warnings *Warnings) error {
seen := mapset.NewSet()
var lockfileEg errgroup.Group
pkg := rootPackageJSON
depSet := mapset.NewSet()
pkg.UnresolvedExternalDeps = make(map[string]string)
for dep, version := range pkg.DevDependencies {
pkg.UnresolvedExternalDeps[dep] = version
Expand All @@ -245,25 +242,25 @@ func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warn
pkg.UnresolvedExternalDeps[dep] = version
}
if c.Lockfile != nil {
pkg.TransitiveDeps = []string{}
c.resolveDepGraph(&lockfileEg, pkg, pkg.UnresolvedExternalDeps, depSet, seen, pkg)
if err := lockfileEg.Wait(); err != nil {
depSet, err := TransitiveClosure(pkg, c.Lockfile)
if err != nil {
warnings.append(err)
// Return early to skip using results of incomplete dep graph resolution
return nil
}
pkg.ExternalDeps = make([]string, 0, depSet.Cardinality())
pkg.TransitiveDeps = make([]lockfile.Package, 0, depSet.Cardinality())
for _, v := range depSet.ToSlice() {
pkg.ExternalDeps = append(pkg.ExternalDeps, fmt.Sprintf("%v", v))
dep := v.(lockfile.Package)
pkg.TransitiveDeps = append(pkg.TransitiveDeps, dep)
}
sort.Strings(pkg.ExternalDeps)
hashOfExternalDeps, err := fs.HashObject(pkg.ExternalDeps)
sort.Sort(lockfile.ByKey(pkg.TransitiveDeps))
hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps)
if err != nil {
return err
}
pkg.ExternalDepsHash = hashOfExternalDeps
} else {
pkg.ExternalDeps = []string{}
pkg.TransitiveDeps = []lockfile.Package{}
pkg.ExternalDepsHash = ""
}

Expand All @@ -280,7 +277,6 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root
depMap := make(map[string]string)
internalDepsSet := make(dag.Set)
externalUnresolvedDepsSet := make(dag.Set)
externalDepSet := mapset.NewSet()
pkg.UnresolvedExternalDeps = make(map[string]string)

for dep, version := range pkg.DevDependencies {
Expand Down Expand Up @@ -320,31 +316,29 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root
}
}

pkg.TransitiveDeps = []string{}
seen := mapset.NewSet()
lockfileEg := &errgroup.Group{}
c.resolveDepGraph(lockfileEg, pkg, pkg.UnresolvedExternalDeps, externalDepSet, seen, pkg)
if err := lockfileEg.Wait(); err != nil {
externalDeps, err := TransitiveClosure(pkg, c.Lockfile)
if err != nil {
warnings.append(err)
// reset external deps to original state
externalDepSet = mapset.NewSet()
externalDeps = mapset.NewSet()
}

// when there are no internal dependencies, we need to still add these leafs to the graph
if internalDepsSet.Len() == 0 {
c.WorkspaceGraph.Connect(dag.BasicEdge(pkg.Name, core.ROOT_NODE_NAME))
}
pkg.ExternalDeps = make([]string, 0, externalDepSet.Cardinality())
for _, v := range externalDepSet.ToSlice() {
pkg.ExternalDeps = append(pkg.ExternalDeps, fmt.Sprintf("%v", v))
pkg.TransitiveDeps = make([]lockfile.Package, 0, externalDeps.Cardinality())
for _, dependency := range externalDeps.ToSlice() {
dependency := dependency.(lockfile.Package)
pkg.TransitiveDeps = append(pkg.TransitiveDeps, dependency)
}
pkg.InternalDeps = make([]string, 0, internalDepsSet.Len())
for _, v := range internalDepsSet.List() {
pkg.InternalDeps = append(pkg.InternalDeps, fmt.Sprintf("%v", v))
}
sort.Strings(pkg.InternalDeps)
sort.Strings(pkg.ExternalDeps)
hashOfExternalDeps, err := fs.HashObject(pkg.ExternalDeps)
sort.Sort(lockfile.ByKey(pkg.TransitiveDeps))
hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps)
if err != nil {
return err
}
Expand Down Expand Up @@ -379,40 +373,50 @@ func (c *Context) parsePackageJSON(repoRoot turbopath.AbsoluteSystemPath, pkgJSO
return nil
}

func (c *Context) resolveDepGraph(wg *errgroup.Group, workspace *fs.PackageJSON, unresolvedDirectDeps map[string]string, resolvedDepsSet mapset.Set, seen mapset.Set, pkg *fs.PackageJSON) {
if c.Lockfile == (lockfile.Lockfile)(nil) {
return
// TransitiveClosure the set of all lockfile keys that pkg depends on
func TransitiveClosure(pkg *fs.PackageJSON, lockFile lockfile.Lockfile) (mapset.Set, error) {
if lockfile.IsNil(lockFile) {
return nil, fmt.Errorf("No lockfile available to do analysis on")
}

resolvedPkgs := mapset.NewSet()
lockfileEg := &errgroup.Group{}

transitiveClosureHelper(lockfileEg, pkg, lockFile, pkg.UnresolvedExternalDeps, resolvedPkgs)

if err := lockfileEg.Wait(); err != nil {
return nil, err
}

return resolvedPkgs, nil
}

func transitiveClosureHelper(wg *errgroup.Group, pkg *fs.PackageJSON, lockfile lockfile.Lockfile, unresolvedDirectDeps map[string]string, resolvedDeps mapset.Set) {
for directDepName, unresolvedVersion := range unresolvedDirectDeps {
directDepName := directDepName
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of these lines?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't copy the values here then it'll resolve as the same value in every iteration of the loop: https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable

I'm so excited for Rust

unresolvedVersion := unresolvedVersion
wg.Go(func() error {

lockfilePkg, err := c.Lockfile.ResolvePackage(workspace.Dir.ToUnixPath(), directDepName, unresolvedVersion)
lockfilePkg, err := lockfile.ResolvePackage(pkg.Dir.ToUnixPath(), directDepName, unresolvedVersion)

if err != nil {
return err
}

if !lockfilePkg.Found || seen.Contains(lockfilePkg.Key) {
if !lockfilePkg.Found || resolvedDeps.Contains(lockfilePkg) {
return nil
}

seen.Add(lockfilePkg.Key)

pkg.Mu.Lock()
pkg.TransitiveDeps = append(pkg.TransitiveDeps, lockfilePkg.Key)
pkg.Mu.Unlock()
resolvedDepsSet.Add(fmt.Sprintf("%s@%s", lockfilePkg.Key, lockfilePkg.Version))
resolvedDeps.Add(lockfilePkg)
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we can get rid of the lock here? Is it because we're only mutating resolvedDeps now? Are there any race conditions due to the wg.Go?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we needed the lock because each goroutine that was spun up via the working group could try to append TransitiveDeps and go slices aren't thread safe. mapset.Set is threadsafe so we don't need to lock anything up


allDeps, ok := c.Lockfile.AllDependencies(lockfilePkg.Key)
allDeps, ok := lockfile.AllDependencies(lockfilePkg.Key)

if !ok {
panic(fmt.Sprintf("Unable to find entry for %s", lockfilePkg.Key))
}

if len(allDeps) > 0 {
c.resolveDepGraph(wg, workspace, allDeps, resolvedDepsSet, seen, pkg)
transitiveClosureHelper(wg, pkg, lockfile, allDeps, resolvedDeps)
}

return nil
Expand Down Expand Up @@ -448,3 +452,62 @@ func (c *Context) InternalDependencies(start []string) ([]string, error) {

return targets, nil
}

// ChangedPackages returns a list of changed packages based on the contents of a previous lockfile
// This assumes that none of the package.json in the workspace change, it is
// the responsibility of the caller to verify this.
func (c *Context) ChangedPackages(previousLockfile lockfile.Lockfile) ([]string, error) {
if lockfile.IsNil(previousLockfile) || lockfile.IsNil(c.Lockfile) {
return nil, fmt.Errorf("Cannot detect changed packages without previous and current lockfile")
}

didPackageChange := func(pkgName string, pkg *fs.PackageJSON) bool {
previousDeps, err := TransitiveClosure(pkg, previousLockfile)
if err != nil || previousDeps.Cardinality() != len(pkg.TransitiveDeps) {
return true
}

prevExternalDeps := make([]lockfile.Package, 0, previousDeps.Cardinality())
for _, d := range previousDeps.ToSlice() {
prevExternalDeps = append(prevExternalDeps, d.(lockfile.Package))
}
sort.Sort(lockfile.ByKey(prevExternalDeps))

for i := range prevExternalDeps {
if prevExternalDeps[i] != pkg.TransitiveDeps[i] {
return true
}
}
return false
}

changedPkgs := make([]string, 0, len(c.WorkspaceInfos))

// check if prev and current have "global" changes e.g. lockfile bump
globalChange := c.Lockfile.GlobalChange(previousLockfile)

for pkgName, pkg := range c.WorkspaceInfos {
if globalChange {
break
}
if didPackageChange(pkgName, pkg) {
if pkgName == util.RootPkgName {
globalChange = true
} else {
changedPkgs = append(changedPkgs, pkgName)
}
}
}

if globalChange {
changedPkgs = make([]string, 0, len(c.WorkspaceInfos))
for pkgName := range c.WorkspaceInfos {
changedPkgs = append(changedPkgs, pkgName)
}
sort.Strings(changedPkgs)
return changedPkgs, nil
}

sort.Strings(changedPkgs)
return changedPkgs, nil
}
4 changes: 2 additions & 2 deletions cli/internal/fs/package_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"sync"

"github.com/vercel/turbo/cli/internal/lockfile"
"github.com/vercel/turbo/cli/internal/turbopath"
)

Expand All @@ -31,8 +32,7 @@ type PackageJSON struct {
Dir turbopath.AnchoredSystemPath `json:"-"`
InternalDeps []string `json:"-"`
UnresolvedExternalDeps map[string]string `json:"-"`
ExternalDeps []string `json:"-"`
TransitiveDeps []string `json:"-"`
TransitiveDeps []lockfile.Package `json:"-"`
LegacyTurboConfig *TurboJSON `json:"turbo"`
Mu sync.Mutex `json:"-"`
ExternalDepsHash string `json:"-"`
Expand Down
12 changes: 12 additions & 0 deletions cli/internal/lockfile/berry_lockfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"encoding/json"
"fmt"
"io"
"reflect"
"regexp"
"sort"
"strconv"
Expand Down Expand Up @@ -400,6 +401,17 @@ func DecodeBerryLockfile(contents []byte) (*BerryLockfile, error) {
return &lockfile, nil
}

// GlobalChange checks if there are any differences between lockfiles that would completely invalidate
// the cache.
func (l *BerryLockfile) GlobalChange(other Lockfile) bool {
otherBerry, ok := other.(*BerryLockfile)
return !ok ||
l.cacheKey != otherBerry.cacheKey ||
l.version != otherBerry.version ||
// This is probably overly cautious, but getting it correct will be hard
!reflect.DeepEqual(l.patches, otherBerry.patches)
}

// Fields shared between _Locator and _Descriptor
type _Ident struct {
// Scope of package without leading @
Expand Down
Loading