Skip to content

Commit

Permalink
feat: Use lockfiles when determining what packages changed across com…
Browse files Browse the repository at this point in the history
…mits (#3250)

This PR attempts to address #2162

This PR was written to be conservative by nature and it will mark
packages as changed on almost any lockfile change that isn't just a
version bump.

Notable changes:
- Extended SCM interface as we need to be able to reconstruct old
lockfile
- Refactored calculation of the transitive closure of the package graph
so a full `Context` object isn't required
- Merged `TransitiveDeps` and `ExternalDeps` as they contained almost
identical information

I haven't been able to fully test this, but from a quick spot check of
pnpm it seems to behave as expected.
Needed testing:

- [x] Npm
- [x] Yarn
- [x] Yarn2+
- [x] Pnpm
  • Loading branch information
chris-olszewski authored and mehulkar committed Feb 3, 2023
1 parent 6f91b4b commit 226f6dc
Show file tree
Hide file tree
Showing 25 changed files with 523 additions and 80 deletions.
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 @@ -374,40 +368,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
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)

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 @@ -443,3 +447,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 @@ -6,6 +6,7 @@ import (
"path/filepath"
"sync"

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

Expand All @@ -32,8 +33,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

0 comments on commit 226f6dc

Please sign in to comment.