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

chore: move lockfile ffi boundary #4629

Merged
merged 18 commits into from Apr 26, 2023
Merged

chore: move lockfile ffi boundary #4629

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8c99b57
populate workspace graph serially
chris-olszewski Apr 14, 2023
8d93b1e
move external deps population to method
chris-olszewski Apr 17, 2023
ab9f0b3
move lockfile parsing and traversal to final step
chris-olszewski Apr 17, 2023
f9a460c
calculate closures all at once
chris-olszewski Apr 17, 2023
4f9c395
calculate all lockfiles at once in rust
chris-olszewski Apr 18, 2023
31ef289
revert serial graph construction
chris-olszewski Apr 18, 2023
1c6d663
minor pr feedback changes
chris-olszewski Apr 21, 2023
92f0b58
switch to protobuf enum for package manager
chris-olszewski Apr 21, 2023
2efd867
remove clone in favor of borrow
chris-olszewski Apr 21, 2023
57be317
remove type hint
chris-olszewski Apr 21, 2023
0ff9dbc
move ffi conversion logic to standard traits
chris-olszewski Apr 21, 2023
1931953
Merge branch 'main' into olszewski/move_lockfile_ffi_boundary
chris-olszewski Apr 24, 2023
fbdeffd
Merge branch 'main' into olszewski/move_lockfile_ffi_boundary
chris-olszewski Apr 24, 2023
8c6c647
fix berry test
chris-olszewski Apr 24, 2023
b0f20e7
add tests for verifying external hash is empty string when no lockfil…
chris-olszewski Apr 24, 2023
d1c52da
add tests for ensuring empty sets are different than no lockfile
chris-olszewski Apr 24, 2023
fada352
fix go lint
chris-olszewski Apr 24, 2023
9049e5c
add go unit test on the other side of ffi
chris-olszewski Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 7 additions & 19 deletions cli/internal/context/context.go
View file
Open in desktop
Expand Up @@ -257,17 +257,9 @@ func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warn
// 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 {
if err := pkg.SetExternalDeps(depSet); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thing I investigated during review: why is this "every time" if you have a lockfile and not simply collapsing to "" when empty?

The answer: ExternalDepsHash being set to "" is basically a None sigil which is correctly distinct from Hash(Some({})).

return err
}
pkg.ExternalDepsHash = hashOfExternalDeps
} else {
pkg.TransitiveDeps = []lockfile.Package{}
pkg.ExternalDepsHash = ""
Expand Down Expand Up @@ -338,22 +330,18 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root
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 {

if err := pkg.SetExternalDeps(externalDeps); err != nil {
return err
}
pkg.ExternalDepsHash = hashOfExternalDeps

sort.Strings(pkg.InternalDeps)

return nil
}

Expand Down
20 changes: 20 additions & 0 deletions cli/internal/fs/package_json.go
View file
Open in desktop
Expand Up @@ -3,8 +3,10 @@ package fs
import (
"bytes"
"encoding/json"
"sort"
"sync"

mapset "github.com/deckarep/golang-set"
"github.com/vercel/turbo/cli/internal/lockfile"
"github.com/vercel/turbo/cli/internal/turbopath"
)
Expand Down Expand Up @@ -120,6 +122,24 @@ func MarshalPackageJSON(pkgJSON *PackageJSON) ([]byte, error) {
return b.Bytes(), nil
}

// SetExternalDeps sets TransitiveDeps and populates ExternalDepsHash
func (p *PackageJSON) SetExternalDeps(externalDeps mapset.Set) error {
p.Mu.Lock()
defer p.Mu.Unlock()
p.TransitiveDeps = make([]lockfile.Package, 0, externalDeps.Cardinality())
for _, dependency := range externalDeps.ToSlice() {
dependency := dependency.(lockfile.Package)
nathanhammond marked this conversation as resolved.
Show resolved Hide resolved
p.TransitiveDeps = append(p.TransitiveDeps, dependency)
}
sort.Sort(lockfile.ByKey(p.TransitiveDeps))
Copy link
Contributor

Choose a reason for hiding this comment

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

Teach me! Why was string sorting here not enough? (I read it but I don't have enough context to understand why we do it this way.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Quick history of the TransitiveDeps field:

  • Initially this was a string of the format {packageName}@{version}, worked when we just had yarn1 support, but for pnpm these two pieces of information weren't enough. So we moved to using lockfile keys instead.
  • I moved from the {lockfileKey}@{version} to the Package struct (a mistake in hindsight), just because the string wasn't useful unless you correctly split it into two parts and I was afraid of someone (myself) doing strings.Split("/@babel/types@1.2.3@1.2.3", "@") to try to get the version and end up with garbage
  • Most recently there was an issue where pnpm package aliases where we weren't correctly merging lockfile keys. We basically had an issue of the same package being listed as Package{Key: "/foo/1.2.3", Version: "1.2.3"} and another as Package{Key: "/foo/1.2.3", Version: "npm:foo@1.2.3"} (See fix: better support for pnpm aliases #4555 for a more complete description).

hashOfExternalDeps, err := HashObject(p.TransitiveDeps)
if err != nil {
return err
}
p.ExternalDepsHash = hashOfExternalDeps
return nil
}

func isEmpty(value interface{}) bool {
if value == nil {
return true
Expand Down