dev: reject builds when CRL JS dependencies are 'pnpm link'ed #107129
Conversation
|
This change is |
sjbarag
force-pushed
the
reject-cockroach-builds-when-linked-npm-deps-detected
branch
from
f3e8c9d to
3b1d54e
Compare
|
super nit: should those arrows point right instead of left? I copied the 
|
pkg/cmd/dev/io/os/os.go
Outdated
| @@ -434,6 +462,7 @@ func (o *OS) ListSubdirectories(path string) ([]string, error) { | |||
| if entry.IsDir() { | |||
| ret = append(ret, entry.Name()) | |||
| } | |||
| fmt.Printf("entry %q is a dir? %t\n", entry.Name(), entry.IsDir()) | |||
| depPath := filepath.Join(crlModulesPath, depName) | ||
| resolved, err := d.os.Readlink(depPath) | ||
| if err != nil { | ||
| return fmt.Errorf("could not evaluate symlink %s: %w", depPath, err) |
There was a problem hiding this comment.
Is this an error condition (e.g. the function must fail if this is not a symlink), or something we can safely ignore? My intuition tells me that if it's e.g. a normal directory, then it's not a symlink and so we have passed the check. But maybe there is something I'm not understanding. I also don't fully understand the differences in behavior if you are using pnpm directly vs. bazel.
There was a problem hiding this comment.
The good news is that pnpm guarantees everything here will be a symlink into pkg/ui/node_modules/.pnpm/, so there's no chance of getting a non-symlink here. I can add an isDir() (or use filepath.EvalSymlinks(), which should be safe to use on non-links) if you'd prefer!
There was a problem hiding this comment.
No worries, I'm satisfied as long as this is pnpm's default behavior.
There was a problem hiding this comment.
Yep — doc link for the future: https://pnpm.io/symlinked-node-modules-structure
pkg/cmd/dev/ui.go
Outdated
| filepath.Join(crlModulesPath, resolved), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("could not relitivize path %s: %w", resolved, err) |
When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running 'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within pkg/ui/workspaces/* creates a symbolic link at node_modules/@cockroachlabs/foo that points to ../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a 'pnpm webpack --watch' running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc. Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked @cockroachlabs/ packages are detected. [1] https://pnpm.io/cli/link [2] aspect-build/rules_js#1165 Release note: None Epic: none
sjbarag
force-pushed
the
reject-cockroach-builds-when-linked-npm-deps-detected
branch
from
3b1d54e to
3ba9bcb
Compare
|
bors r=rickystewart |
|
Build succeeded: |
When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running
pnpm link /path/to/github.com/cockroachdb/ui/packages/foofrom within pkg/ui/workspaces/* creates a symbolic link atnode_modules/@cockroachlabs/foo that points to
../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a
pnpm webpack --watchrunning in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc.Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow
pnpm linkto be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked @cockroachlabs/ packages are detected.[1] https://pnpm.io/cli/link
[2] aspect-build/rules_js#1165
Release note: None
Epic: none
Example output:
Total duration is 16ms on my machine since we're checking so few files. We can drop these into a goroutine per first-party JS if we want, but this is certainly fast enough to be conversational.