chore: move lockfile ffi boundary #4629

chris-olszewski · 2023-04-18T18:24:51Z

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

vercel · 2023-04-18T18:25:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
examples-gatsby-web	🔄 Building (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-nonmonorepo	🔄 Building (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-svelte-web	🔄 Building (Inspect)		Apr 25, 2023 6:09pm

8 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-cra-web	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm
examples-tailwind-web	⬜️ Ignored (Inspect)		Apr 25, 2023 6:09pm
examples-vite-web	⬜️ Ignored (Inspect)		Apr 25, 2023 6:09pm
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2023 6:09pm

github-actions · 2023-04-18T18:54:46Z

🟢 CI successful 🟢

Thanks

nathanhammond

There's no tests added here and existing coverage is super-spotty. It's a pretty-invasive change. That leaves me a little concerned. Is there testing that could be reasonably added to constrain the ffi serialization and object reconstruction from the message?

If you want to go for bonus points, constraining ExternalDepsHash to always be "" when there is not a lockfile, and that it is always populated with something when it is would be good. I think that it's Hash("") sometimes, which is at least distinct?

nathanhammond · 2023-04-20T06:05:56Z

cli/internal/context/context.go

-		sort.Sort(lockfile.ByKey(pkg.TransitiveDeps))
-		hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps)
-		if err != nil {
+		if err := pkg.SetExternalDeps(depSet); err != nil {


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({})).

cli/internal/fs/package_json.go

nathanhammond · 2023-04-20T06:18:39Z

cli/internal/fs/package_json.go

+		dependency := dependency.(lockfile.Package)
+		p.TransitiveDeps = append(p.TransitiveDeps, dependency)
+	}
+	sort.Sort(lockfile.ByKey(p.TransitiveDeps))


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.)

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).

nathanhammond · 2023-04-20T06:20:56Z

cli/internal/context/context.go

@@ -231,6 +224,29 @@ func BuildPackageGraph(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *f
 	}
 	c.WorkspaceInfos.PackageJSONs[util.RootPkgName] = rootPackageJSON

+	if lockFile, err := c.PackageManager.ReadLockfile(repoRoot, rootPackageJSON); err != nil {
+		warnings.append(err)
+		rootPackageJSON.TransitiveDeps = []lockfile.Package{}


Does this work as just nil?

Haven't done a full code trace, but it should. This was just copied from previous behavior.

Edit: Checked and nil works just fine here

cli/internal/context/context.go

nathanhammond · 2023-04-20T06:25:36Z

cli/internal/context/context.go

-	if err != nil {
-		warnings.append(err)
-		// reset external deps to original state
-		externalDeps = mapset.NewSet()


What was this doing?

This was here instead of proper control flow, if the closure calculation errored we would override the return value with an empty set. This would then get translated to pkg.TransitiveDeps = make([]lockfile.Package, 0) and nothing would get added.

cli/internal/context/context.go

nathanhammond · 2023-04-20T06:35:43Z

cli/internal/ffi/ffi.go

-
-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) {


Confirming understanding: this has to do a full walk because if there are floating versions the top-level deps could have not changed, but somewhere way down the tree could have.

Exactly, if someone blows away the lockfile and does a fresh install all of the top level specifiers will remain the same, but what version gets installed might change.

crates/turborepo-ffi/src/lockfile.rs

NicholasLYang

The code makes sense to me, at least to the extent that I understand the lockfile code. The testing situation I'll let you and Nathan determine

cli/internal/context/context.go

NicholasLYang · 2023-04-20T15:11:38Z

crates/turborepo-ffi/src/lockfile.rs

-    ProtobufError(#[from] prost::DecodeError),
+    Protobuf(#[from] prost::DecodeError),
+    #[error("unsupported package manager")]
+    UnsupportedPackageManager(String),


not super relevant to the PR but if we have new error variants, perhaps we could add backtrace to them?

crates/turborepo-ffi/src/lockfile.rs

…e is present

### Description Built on top of #4629 Overview of changes: - ~~Change `subgraph` so we calculate workspace's closure in parallel in a similar manner to Go~~ This had to get reverted, it appears that rayon was causing [compilation issues on linux](https://github.com/vercel/turbo/actions/runs/4791974785/jobs/8522988391#step:4:648) - `npm_subgraph` -> `subgraph` makes this FFI call generic - Addition of an optional `resolutions` map to the `subgraph` and `transitive_closure`, this is only used by Berry. In the ideal world this would be all of the root `package.json`, but serializing all of that via protobuf is overkill since berry is the only lockfile that needs that info and we only use that field. - Addition of `patches` FFI function, this wasn't needed before as NPM doesn't have patch files encoded in the lockfile - Addition of `global_changes` FFI function, previously for NPM we were just doing this in Go since there were just two fields we needed to check and parsing JSON is cheap. - Switches lockfile usage of berry from the Go implementation to the Rust one introduced in #4589 Reviewer notes: - The passing of the optional resolutions map is very icky and one off, but this is the only lockfile the requires additional information to properly parse. Once the package graph is squarely in Rust land this should go back to being more generic. This will finally close out #2791 ### Testing Instructions Added a new integration test that uses resolutions. This integration test invokes the new `patches` FFI call. For a full test of the new resolution feature, clone and follow the instructions in the [repro](https://github.com/erj826/turbo-prune-bug-repro-11-21) The `lockfile_aware_caching` integration tests all hit the new `global_changes` FFI callsite and provide test coverage. --------- Co-authored-by: Alexander Lyon <arlyon@me.com>

chris-olszewski added 6 commits April 18, 2023 10:44

populate workspace graph serially

8c99b57

move external deps population to method

8d93b1e

move lockfile parsing and traversal to final step

ab9f0b3

calculate closures all at once

f9a460c

calculate all lockfiles at once in rust

4f9c395

revert serial graph construction

31ef289

turbo-orchestrator bot added the team: turborepo label Apr 18, 2023

chris-olszewski marked this pull request as ready for review April 18, 2023 18:25

chris-olszewski requested a review from a team as a code owner April 18, 2023 18:25

chris-olszewski requested review from nathanhammond and NicholasLYang April 18, 2023 18:25

nathanhammond reviewed Apr 20, 2023

View reviewed changes

NicholasLYang reviewed Apr 20, 2023

View reviewed changes

arlyon reviewed Apr 20, 2023

View reviewed changes

crates/turborepo-ffi/src/lockfile.rs Outdated Show resolved Hide resolved

chris-olszewski added 6 commits April 21, 2023 10:06

minor pr feedback changes

1c6d663

switch to protobuf enum for package manager

92f0b58

remove clone in favor of borrow

2efd867

remove type hint

57be317

move ffi conversion logic to standard traits

0ff9dbc

Merge branch 'main' into olszewski/move_lockfile_ffi_boundary

1931953

vercel bot deployed to Preview – examples-vite-web April 24, 2023 17:54 View deployment

vercel bot deployed to Preview – examples-basic-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-native-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-gatsby-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-svelte-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-nonmonorepo April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-tailwind-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-kitchensink-blog April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-designsystem-docs April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – examples-cra-web April 24, 2023 17:55 View deployment

vercel bot deployed to Preview – turbo-site April 24, 2023 17:55 View deployment

chris-olszewski mentioned this pull request Apr 24, 2023

feat: Use Rust Berry lockfile impl #4684

Merged

chris-olszewski added 3 commits April 24, 2023 13:17

Merge branch 'main' into olszewski/move_lockfile_ffi_boundary

fbdeffd

fix berry test

8c6c647

add tests for verifying external hash is empty string when no lockfil…

b0f20e7

…e is present

vercel bot had a problem deploying to Preview – examples-vite-web April 24, 2023 21:42 Failure

chris-olszewski added 2 commits April 24, 2023 15:41

add tests for ensuring empty sets are different than no lockfile

d1c52da

fix go lint

fada352

vercel bot had a problem deploying to Preview – examples-svelte-web April 25, 2023 00:19 Failure

vercel bot had a problem deploying to Preview – examples-tailwind-web April 25, 2023 00:19 Failure

add go unit test on the other side of ffi

9049e5c

NicholasLYang approved these changes Apr 26, 2023

View reviewed changes

chris-olszewski merged commit 1c8d3aa into main Apr 26, 2023

chris-olszewski deleted the olszewski/move_lockfile_ffi_boundary branch April 26, 2023 18:21

sokra mentioned this pull request Apr 26, 2023

update turbopack vercel/next.js#48893

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move lockfile ffi boundary #4629

chore: move lockfile ffi boundary #4629

chris-olszewski commented Apr 18, 2023

vercel bot commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 18, 2023 •

edited

Loading

nathanhammond left a comment

nathanhammond Apr 20, 2023

nathanhammond Apr 20, 2023

chris-olszewski Apr 21, 2023

nathanhammond Apr 20, 2023

chris-olszewski Apr 21, 2023 •

edited

Loading

nathanhammond Apr 20, 2023

chris-olszewski Apr 21, 2023

nathanhammond Apr 20, 2023

chris-olszewski Apr 21, 2023

NicholasLYang left a comment

NicholasLYang Apr 20, 2023

chore: move lockfile ffi boundary #4629

chore: move lockfile ffi boundary #4629

Conversation

chris-olszewski commented Apr 18, 2023

Description

Testing Instructions

vercel bot commented Apr 18, 2023 • edited Loading

github-actions bot commented Apr 18, 2023 • edited Loading

🟢 CI successful 🟢

nathanhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-olszewski Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicholasLYang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 18, 2023 •

edited

Loading

github-actions bot commented Apr 18, 2023 •

edited

Loading

chris-olszewski Apr 21, 2023 •

edited

Loading