Skip to content

Commit

Permalink
chore: move lockfile ffi boundary (#4629)
Browse files Browse the repository at this point in the history
### Description

In order to avoid Go holding onto memory that's been allocated by Rust
we parse the lockfile each time we need to make a lockfile call. This
worked fine for the npm implementation as we just needed parse the JSON
and it was usable, but Berry requires a lot more work/allocations that
makes this strategy unfeasible.

A quick sketch of the changes in this PR:
- Moves the traversal of the lockfile to be the final step of building
the package graph
 - We now calculate all of the closures of all workspaces at once
- Rust FFI now takes the package manager string to select the lockfile
implementation. This is a quick prefactor to prepare for hooking up the
berry lockfile

Reviewer notes:
- Each commit can be reviewed on it's own, the first commit ended up
getting reverted down the stack so it can be skipped.

### Testing Instructions

Existing unit tests/integration tests. Manual verification of correct
pruning behavior for the npm and pnpm monorepos created by
`create-turbo@latest`
  • Loading branch information
chris-olszewski committed Apr 26, 2023
1 parent 34a0740 commit 1c8d3aa
Show file tree
Hide file tree
Showing 16 changed files with 811 additions and 251 deletions.
109 changes: 47 additions & 62 deletions cli/internal/context/context.go
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/vercel/turbo/cli/internal/workspace"

"github.com/Masterminds/semver"
mapset "github.com/deckarep/golang-set"
"github.com/pyr-sh/dag"
"golang.org/x/sync/errgroup"
)
Expand Down Expand Up @@ -179,12 +178,6 @@ func BuildPackageGraph(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *f
}
c.PackageManager = packageManager

if lockfile, err := c.PackageManager.ReadLockfile(repoRoot, rootPackageJSON); err != nil {
warnings.append(err)
} else {
c.Lockfile = lockfile
}

if err := c.resolveWorkspaceRootDeps(rootPackageJSON, &warnings); err != nil {
// TODO(Gaspar) was this the intended return error?
return nil, fmt.Errorf("could not resolve workspaces: %w", err)
Expand Down Expand Up @@ -232,6 +225,10 @@ func BuildPackageGraph(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *f
}
c.WorkspaceInfos.PackageJSONs[util.RootPkgName] = rootPackageJSON

if err := c.populateExternalDeps(repoRoot, rootPackageJSON, &warnings); err != nil {
return nil, err
}

return c, warnings.errorOrNil()
}

Expand All @@ -247,33 +244,6 @@ func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warn
for dep, version := range pkg.Dependencies {
pkg.UnresolvedExternalDeps[dep] = version
}
if c.Lockfile != nil {
depSet, err := lockfile.TransitiveClosure(
pkg.Dir.ToUnixPath(),
pkg.UnresolvedExternalDeps,
c.Lockfile,
)
if err != nil {
warnings.append(err)
// Return early to skip using results of incomplete dep graph resolution
return nil
}
pkg.TransitiveDeps = make([]lockfile.Package, 0, depSet.Cardinality())
for _, v := range depSet.ToSlice() {
dep := v.(lockfile.Package)
pkg.TransitiveDeps = append(pkg.TransitiveDeps, dep)
}
sort.Sort(lockfile.ByKey(pkg.TransitiveDeps))
hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps)
if err != nil {
return err
}
pkg.ExternalDepsHash = hashOfExternalDeps
} else {
pkg.TransitiveDeps = []lockfile.Package{}
pkg.ExternalDepsHash = ""
}

return nil
}

Expand Down Expand Up @@ -326,37 +296,18 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root
}
}

externalDeps, err := lockfile.TransitiveClosure(
pkg.Dir.ToUnixPath(),
pkg.UnresolvedExternalDeps,
c.Lockfile,
)
if err != nil {
warnings.append(err)
// reset external deps to original state
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.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.Sort(lockfile.ByKey(pkg.TransitiveDeps))
hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps)
if err != nil {
return err
}
pkg.ExternalDepsHash = hashOfExternalDeps

return nil
}

Expand Down Expand Up @@ -387,6 +338,39 @@ func (c *Context) parsePackageJSON(repoRoot turbopath.AbsoluteSystemPath, pkgJSO
return nil
}

func (c *Context) externalWorkspaceDeps() map[turbopath.AnchoredUnixPath]map[string]string {
workspaces := make(map[turbopath.AnchoredUnixPath]map[string]string, len(c.WorkspaceInfos.PackageJSONs))
for _, pkg := range c.WorkspaceInfos.PackageJSONs {
workspaces[pkg.Dir.ToUnixPath()] = pkg.UnresolvedExternalDeps
}
return workspaces
}

func (c *Context) populateExternalDeps(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *fs.PackageJSON, warnings *Warnings) error {
if lockFile, err := c.PackageManager.ReadLockfile(repoRoot, rootPackageJSON); err != nil {
warnings.append(err)
rootPackageJSON.TransitiveDeps = nil
rootPackageJSON.ExternalDepsHash = ""
} else {
c.Lockfile = lockFile
if closures, err := lockfile.AllTransitiveClosures(c.externalWorkspaceDeps(), c.Lockfile); err != nil {
warnings.append(err)
} else {
for _, pkg := range c.WorkspaceInfos.PackageJSONs {
if closure, ok := closures[pkg.Dir.ToUnixPath()]; ok {
if err := pkg.SetExternalDeps(closure); err != nil {
return err
}
} else {
return fmt.Errorf("Unable to calculate closure for workspace %s", pkg.Dir.ToString())
}
}
}
}

return nil
}

// InternalDependencies finds all dependencies required by the slice of starting
// packages, as well as the starting packages themselves.
func (c *Context) InternalDependencies(start []string) ([]string, error) {
Expand Down Expand Up @@ -424,13 +408,14 @@ func (c *Context) ChangedPackages(previousLockfile lockfile.Lockfile) ([]string,
return nil, fmt.Errorf("Cannot detect changed packages without previous and current lockfile")
}

closures, err := lockfile.AllTransitiveClosures(c.externalWorkspaceDeps(), previousLockfile)
if err != nil {
return nil, err
}

didPackageChange := func(pkgName string, pkg *fs.PackageJSON) bool {
previousDeps, err := lockfile.TransitiveClosure(
pkg.Dir.ToUnixPath(),
pkg.UnresolvedExternalDeps,
previousLockfile,
)
if err != nil || previousDeps.Cardinality() != len(pkg.TransitiveDeps) {
previousDeps, ok := closures[pkg.Dir.ToUnixPath()]
if !ok || previousDeps.Cardinality() != len(pkg.TransitiveDeps) {
return true
}

Expand Down
37 changes: 37 additions & 0 deletions cli/internal/context/context_test.go
@@ -1,14 +1,20 @@
package context

import (
"errors"
"os"
"path/filepath"
"regexp"
"sync"
"testing"

testifyAssert "github.com/stretchr/testify/assert"
"github.com/vercel/turbo/cli/internal/fs"
"github.com/vercel/turbo/cli/internal/lockfile"
"github.com/vercel/turbo/cli/internal/packagemanager"
"github.com/vercel/turbo/cli/internal/turbopath"
"github.com/vercel/turbo/cli/internal/workspace"
"gotest.tools/v3/assert"
)

func Test_isWorkspaceReference(t *testing.T) {
Expand Down Expand Up @@ -144,6 +150,37 @@ func TestBuildPackageGraph_DuplicateNames(t *testing.T) {
testifyAssert.Regexp(t, regexp.MustCompile("^Failed to add workspace \"same-name\".+$"), actualErr)
}

func Test_populateExternalDeps_NoTransitiveDepsWithoutLockfile(t *testing.T) {
path := getTestDir(t, "dupe-workspace-names")
pkgJSON := &fs.PackageJSON{
Name: "dupe-workspace-names",
PackageManager: "pnpm@7.15.0",
}

pm, err := packagemanager.GetPackageManager(path, pkgJSON)
assert.NilError(t, err)
pm.UnmarshalLockfile = func(rootPackageJSON *fs.PackageJSON, contents []byte) (lockfile.Lockfile, error) {
return nil, errors.New("bad lockfile")
}
context := Context{
WorkspaceInfos: workspace.Catalog{
PackageJSONs: map[string]*fs.PackageJSON{
"a": {},
},
},
WorkspaceNames: []string{},
PackageManager: pm,
mutex: sync.Mutex{},
}
var warnings Warnings
err = context.populateExternalDeps(path, pkgJSON, &warnings)
assert.NilError(t, err)

assert.DeepEqual(t, pkgJSON.ExternalDepsHash, "")
assert.DeepEqual(t, context.WorkspaceInfos.PackageJSONs["a"].ExternalDepsHash, "")
assert.Assert(t, warnings.errorOrNil() != nil)
}

// This is duplicated from fs.turbo_json_test.go.
// I wasn't able to pull it into a helper file/package because
// it requires the `fs` package and it would cause cyclical dependencies
Expand Down
2 changes: 1 addition & 1 deletion cli/internal/ffi/bindings.h
Expand Up @@ -16,6 +16,6 @@ struct Buffer changed_files(struct Buffer buffer);

struct Buffer previous_content(struct Buffer buffer);

struct Buffer npm_transitive_closure(struct Buffer buf);
struct Buffer transitive_closure(struct Buffer buf);

struct Buffer npm_subgraph(struct Buffer buf);
45 changes: 30 additions & 15 deletions cli/internal/ffi/ffi.go
Expand Up @@ -18,6 +18,7 @@ import "C"

import (
"errors"
"fmt"
"reflect"
"unsafe"

Expand Down Expand Up @@ -167,23 +168,28 @@ func PreviousContent(gitRoot, fromCommit, filePath string) ([]byte, error) {
return []byte(content), nil
}

// NpmTransitiveDeps returns the transitive external deps of a given package based on the deps and specifiers given
func NpmTransitiveDeps(content []byte, pkgDir string, unresolvedDeps map[string]string) ([]*ffi_proto.LockfilePackage, error) {
return transitiveDeps(npmTransitiveDeps, content, pkgDir, unresolvedDeps)
}

func npmTransitiveDeps(buf C.Buffer) C.Buffer {
return C.npm_transitive_closure(buf)
}

func transitiveDeps(cFunc func(C.Buffer) C.Buffer, content []byte, pkgDir string, unresolvedDeps map[string]string) ([]*ffi_proto.LockfilePackage, error) {
// TransitiveDeps returns the transitive external deps for all provided workspaces
func TransitiveDeps(content []byte, packageManager string, workspaces map[string]map[string]string) (map[string]*ffi_proto.LockfilePackageList, error) {
flatWorkspaces := make(map[string]*ffi_proto.PackageDependencyList)
for workspace, deps := range workspaces {
packageDependencyList := make([]*ffi_proto.PackageDependency, len(deps))
i := 0
for name, version := range deps {
packageDependencyList[i] = &ffi_proto.PackageDependency{
Name: name,
Range: version,
}
i++
}
flatWorkspaces[workspace] = &ffi_proto.PackageDependencyList{List: packageDependencyList}
}
req := ffi_proto.TransitiveDepsRequest{
Contents: content,
WorkspaceDir: pkgDir,
UnresolvedDeps: unresolvedDeps,
PackageManager: toPackageManager(packageManager),
Workspaces: flatWorkspaces,
}
reqBuf := Marshal(&req)
resBuf := cFunc(reqBuf)
resBuf := C.transitive_closure(reqBuf)
reqBuf.Free()

resp := ffi_proto.TransitiveDepsResponse{}
Expand All @@ -195,8 +201,17 @@ func transitiveDeps(cFunc func(C.Buffer) C.Buffer, content []byte, pkgDir string
return nil, errors.New(err)
}

list := resp.GetPackages()
return list.GetList(), nil
dependencies := resp.GetDependencies()
return dependencies.GetDependencies(), nil
}

func toPackageManager(packageManager string) ffi_proto.PackageManager {
switch packageManager {
case "npm":
return ffi_proto.PackageManager_NPM
default:
panic(fmt.Sprintf("Invalid package manager string: %s", packageManager))
}
}

// NpmSubgraph returns the contents of a npm lockfile subgraph
Expand Down

0 comments on commit 1c8d3aa

Please sign in to comment.