Skip to content

Commit 3b1d54e

Browse files
committed
dev: reject builds when CRL JS dependencies are 'pnpm link'ed
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
1 parent 5ff1c3f commit 3b1d54e

File tree

4 files changed

+211
-7
lines changed

4 files changed

+211
-7
lines changed

pkg/cmd/dev/build.go

+13-7
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,12 @@ import (
2929
)
3030

3131
const (
32-
crossFlag = "cross"
33-
nogoDisableFlag = "--//build/toolchains:nogo_disable_flag"
34-
geosTarget = "//c-deps:libgeos"
35-
devTarget = "//pkg/cmd/dev:dev"
32+
crossFlag = "cross"
33+
cockroachTargetOss = "//pkg/cmd/cockroach-oss:cockroach-oss"
34+
cockroachTarget = "//pkg/cmd/cockroach:cockroach"
35+
nogoDisableFlag = "--//build/toolchains:nogo_disable_flag"
36+
geosTarget = "//c-deps:libgeos"
37+
devTarget = "//pkg/cmd/dev:dev"
3638
)
3739

3840
type buildTarget struct {
@@ -77,9 +79,9 @@ var buildTargetMapping = map[string]string{
7779
"bazel-remote": bazelRemoteTarget,
7880
"buildifier": "@com_github_bazelbuild_buildtools//buildifier:buildifier",
7981
"buildozer": "@com_github_bazelbuild_buildtools//buildozer:buildozer",
80-
"cockroach": "//pkg/cmd/cockroach:cockroach",
82+
"cockroach": cockroachTarget,
8183
"cockroach-sql": "//pkg/cmd/cockroach-sql:cockroach-sql",
82-
"cockroach-oss": "//pkg/cmd/cockroach-oss:cockroach-oss",
84+
"cockroach-oss": cockroachTargetOss,
8385
"cockroach-short": "//pkg/cmd/cockroach-short:cockroach-short",
8486
"crlfmt": "@com_github_cockroachdb_crlfmt//:crlfmt",
8587
"dev": devTarget,
@@ -95,7 +97,7 @@ var buildTargetMapping = map[string]string{
9597
"obsservice": "//pkg/obsservice/cmd/obsservice:obsservice",
9698
"optgen": "//pkg/sql/opt/optgen/cmd/optgen:optgen",
9799
"optfmt": "//pkg/sql/opt/optgen/cmd/optfmt:optfmt",
98-
"oss": "//pkg/cmd/cockroach-oss:cockroach-oss",
100+
"oss": cockroachTargetOss,
99101
"reduce": "//pkg/cmd/reduce:reduce",
100102
"roachprod": "//pkg/cmd/roachprod:roachprod",
101103
"roachprod-stress": "//pkg/cmd/roachprod-stress:roachprod-stress",
@@ -171,6 +173,10 @@ func (d *dev) build(cmd *cobra.Command, commandLine []string) error {
171173
}
172174
args = append(args, additionalBazelArgs...)
173175

176+
if err := d.assertNoLinkedNpmDeps(buildTargets); err != nil {
177+
return err
178+
}
179+
174180
if cross == "" {
175181
// Do not log --build_event_binary_file=... because it is not relevant to the actual call
176182
// from the user perspective.

pkg/cmd/dev/io/os/os.go

+29
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,34 @@ func (o *OS) Readlink(filename string) (string, error) {
208208
})
209209
}
210210

211+
// ReadDir is a thin wrapper around os.ReadDir, which returns the names of files
212+
// or directories within dirname.
213+
func (o *OS) ReadDir(dirname string) ([]string, error) {
214+
command := fmt.Sprintf("ls %s", dirname)
215+
if !o.knobs.silent {
216+
o.logger.Print(command)
217+
}
218+
219+
output, err := o.Next(command, func() (string, error) {
220+
var ret []string
221+
entries, err := os.ReadDir(dirname)
222+
if err != nil {
223+
return "", err
224+
}
225+
226+
for _, entry := range entries {
227+
ret = append(ret, entry.Name())
228+
}
229+
230+
return fmt.Sprintf("%s\n", strings.Join(ret, "\n")), nil
231+
})
232+
233+
if err != nil {
234+
return nil, err
235+
}
236+
return strings.Split(strings.TrimSpace(output), "\n"), nil
237+
}
238+
211239
// IsDir wraps around os.Stat, which returns the os.FileInfo of the named
212240
// directory. IsDir returns true if and only if it is an existing directory.
213241
// If there is an error, it will be of type *PathError.
@@ -434,6 +462,7 @@ func (o *OS) ListSubdirectories(path string) ([]string, error) {
434462
if entry.IsDir() {
435463
ret = append(ret, entry.Name())
436464
}
465+
fmt.Printf("entry %q is a dir? %t\n", entry.Name(), entry.IsDir())
437466
}
438467
return fmt.Sprintf("%s\n", strings.Join(ret, "\n")), nil
439468
})

pkg/cmd/dev/testdata/datadriven/dev-build

+8
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,14 @@ cp sandbox/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short crdb-checkou
4949
exec
5050
dev build -- --verbose_failures --sandbox_debug
5151
----
52+
bazel info workspace --color=no
53+
ls crdb-checkout/pkg/ui/node_modules/@cockroachlabs
54+
ls crdb-checkout/pkg/ui/workspaces/eslint-plugin-crdb/node_modules/@cockroachlabs
55+
ls crdb-checkout/pkg/ui/workspaces/db-console/src/js/node_modules/@cockroachlabs
56+
ls crdb-checkout/pkg/ui/workspaces/db-console/ccl/src/js/node_modules/@cockroachlabs
57+
ls crdb-checkout/pkg/ui/workspaces/cluster-ui/node_modules/@cockroachlabs
58+
ls crdb-checkout/pkg/ui/workspaces/db-console/node_modules/@cockroachlabs
59+
ls crdb-checkout/pkg/ui/workspaces/e2e-tests/node_modules/@cockroachlabs
5260
bazel build //pkg/cmd/cockroach:cockroach --verbose_failures --sandbox_debug --build_event_binary_file=/tmp/path
5361
bazel info workspace --color=no
5462
mkdir crdb-checkout/bin

pkg/cmd/dev/ui.go

+161
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
package main
1111

1212
import (
13+
"errors"
1314
"fmt"
1415
"log"
1516
"net/url"
@@ -48,6 +49,8 @@ func makeUICmd(d *dev) *cobra.Command {
4849

4950
// UIDirectories contains the absolute path to the root of each UI sub-project.
5051
type UIDirectories struct {
52+
workspace string
53+
// workspace is the absolute path to ./ .
5154
// root is the absolute path to ./pkg/ui.
5255
root string
5356
// clusterUI is the absolute path to ./pkg/ui/workspaces/cluster-ui.
@@ -58,6 +61,10 @@ type UIDirectories struct {
5861
e2eTests string
5962
// eslintPlugin is the absolute path to ./pkg/ui/workspaces/eslint-plugin-crdb.
6063
eslintPlugin string
64+
// protoOss is the absolute path to ./pkg/ui/workspaces/db-console/src/js/.
65+
protoOss string
66+
// protoCcl is the absolute path to ./pkg/ui/workspaces/db-console/ccl/src/js/.
67+
protoCcl string
6168
}
6269

6370
// getUIDirs computes the absolute path to the root of each UI sub-project.
@@ -68,14 +75,168 @@ func getUIDirs(d *dev) (*UIDirectories, error) {
6875
}
6976

7077
return &UIDirectories{
78+
workspace: workspace,
7179
root: filepath.Join(workspace, "./pkg/ui"),
7280
clusterUI: filepath.Join(workspace, "./pkg/ui/workspaces/cluster-ui"),
7381
dbConsole: filepath.Join(workspace, "./pkg/ui/workspaces/db-console"),
7482
e2eTests: filepath.Join(workspace, "./pkg/ui/workspaces/e2e-tests"),
7583
eslintPlugin: filepath.Join(workspace, "./pkg/ui/workspaces/eslint-plugin-crdb"),
84+
protoOss: filepath.Join(workspace, "./pkg/ui/workspaces/db-console/src/js"),
85+
protoCcl: filepath.Join(workspace, "./pkg/ui/workspaces/db-console/ccl/src/js"),
7686
}, nil
7787
}
7888

89+
// assertNoLinkedNpmDeps looks for JS packages linked outside the Bazel
90+
// workspace (typically via `pnpm link`). It returns an error if:
91+
//
92+
// 'targets' contains a Bazel target that requires the web UI
93+
// AND
94+
// a node_modules/ tree exists within pkg/ui (or its subtrees)
95+
// AND
96+
// a @cockroachlabs-scoped package is symlinked to an external directory
97+
//
98+
// (or if any error occurs while performing one of those checks).
99+
func (d *dev) assertNoLinkedNpmDeps(targets []buildTarget) error {
100+
uiWillBeBuilt := false
101+
for _, target := range targets {
102+
// TODO: This could potentially be a bazel query, e.g.
103+
// 'somepath(${target.fullName}, //pkg/ui/workspaces/db-console:*)' or
104+
// similar, but with only two eligible targets it doesn't seem quite
105+
// worth it.
106+
if target.fullName == cockroachTarget || target.fullName == cockroachTargetOss {
107+
uiWillBeBuilt = true
108+
break
109+
}
110+
}
111+
if !uiWillBeBuilt {
112+
// If no UI build is required, the presence of an externally-linked
113+
// package doesn't matter.
114+
return nil
115+
}
116+
117+
// Find the current workspace and build some relevant absolute paths.
118+
uiDirs, err := getUIDirs(d)
119+
if err != nil {
120+
return fmt.Errorf("could not check for linked NPM dependencies: %w", err)
121+
}
122+
123+
jsPkgRoots := []string{
124+
uiDirs.root,
125+
uiDirs.eslintPlugin,
126+
uiDirs.protoOss,
127+
uiDirs.protoCcl,
128+
uiDirs.clusterUI,
129+
uiDirs.dbConsole,
130+
uiDirs.e2eTests,
131+
}
132+
133+
type LinkedPackage struct {
134+
name string
135+
dir string
136+
}
137+
138+
anyPackageEscapesWorkspace := false
139+
140+
// Check for symlinks in each package's node_modules/@cockroachlabs/ dir.
141+
for _, jsPkgRoot := range jsPkgRoots {
142+
crlModulesPath := filepath.Join(jsPkgRoot, "node_modules/@cockroachlabs")
143+
crlDeps, err := d.os.ReadDir(crlModulesPath)
144+
145+
// If node_modules/@cockroachlabs doesn't exist, it's likely that JS
146+
// dependencies haven't been installed outside the Bazel workspace.
147+
// This is expected for non-UI devs, and is a safe state.
148+
if errors.Is(err, os.ErrNotExist) {
149+
continue
150+
}
151+
if err != nil {
152+
return fmt.Errorf("could not @cockroachlabs/ packages: %w", err)
153+
}
154+
155+
linkedPackages := []LinkedPackage{}
156+
157+
// For each dependency in node_modules/@cockroachlabs/ ...
158+
for _, depName := range crlDeps {
159+
// Ignore empty strings, which are produced by d.os.ReadDir in
160+
// dry-run mode.
161+
if depName == "" {
162+
continue
163+
}
164+
165+
// Resolve the possible symlink.
166+
depPath := filepath.Join(crlModulesPath, depName)
167+
resolved, err := d.os.Readlink(depPath)
168+
if err != nil {
169+
return fmt.Errorf("could not evaluate symlink %s: %w", depPath, err)
170+
}
171+
172+
// Convert it to a path relative to the Bazel workspace root.
173+
relativeToWorkspace, err := filepath.Rel(
174+
uiDirs.workspace,
175+
filepath.Join(crlModulesPath, resolved),
176+
)
177+
if err != nil {
178+
return fmt.Errorf("could not relitivize path %s: %w", resolved, err)
179+
}
180+
181+
// If it doesn't start with '..', it doesn't escape the Bazel
182+
// workspace.
183+
// TODO: Once Go 1.20 is supported here, switch to filepath.IsLocal.
184+
if !strings.HasPrefix(relativeToWorkspace, "..") {
185+
continue
186+
}
187+
188+
// This package escapes the Bazel workspace! Add it to the queue.
189+
abs, err := filepath.Abs(relativeToWorkspace)
190+
if err != nil {
191+
return fmt.Errorf("could not absolutize path %s: %w", resolved, err)
192+
}
193+
194+
linkedPackages = append(
195+
linkedPackages,
196+
LinkedPackage{
197+
name: "@cockroachlabs/" + depName,
198+
dir: abs,
199+
},
200+
)
201+
}
202+
203+
// If this internal package has no dependencies provided by pnpm link,
204+
// move on without logging anything.
205+
if len(linkedPackages) == 0 {
206+
continue
207+
}
208+
209+
if !anyPackageEscapesWorkspace {
210+
anyPackageEscapesWorkspace = true
211+
log.Println("Externally-linked package(s) detected:")
212+
}
213+
214+
log.Printf("pkg/ui/workspaces/%s:", filepath.Base(jsPkgRoot))
215+
for _, pkg := range linkedPackages {
216+
log.Printf(" %s <- %s\n", pkg.name, pkg.dir)
217+
}
218+
log.Println()
219+
}
220+
221+
if anyPackageEscapesWorkspace {
222+
msg := strings.TrimSpace(`
223+
At least one JS dependency is linked to another directory on your machine.
224+
Bazel cannot see changes in these packages, which could lead to both
225+
false-positive and false-negative behavior in the UI.
226+
This build has been pre-emptively failed.
227+
228+
To build without the UI, run:
229+
dev build short
230+
To remove all linked dependencies, run:
231+
dev ui clean --all
232+
`) + "\n"
233+
234+
return fmt.Errorf("%s", msg)
235+
}
236+
237+
return nil
238+
}
239+
79240
// makeUIWatchCmd initializes the 'ui watch' subcommand, which sets up a
80241
// live-reloading HTTP server for db-console and a file-watching rebuilder for
81242
// cluster-ui.

0 commit comments

Comments
 (0)