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

Author: sjbarag

pkg/cmd/dev/build.go

@@ -29,10 +29,12 @@ import (
 )
 
 const (
-	crossFlag       = "cross"
-	nogoDisableFlag = "--//build/toolchains:nogo_disable_flag"
-	geosTarget      = "//c-deps:libgeos"
-	devTarget       = "//pkg/cmd/dev:dev"
+	crossFlag          = "cross"
+	cockroachTargetOss = "//pkg/cmd/cockroach-oss:cockroach-oss"
+	cockroachTarget    = "//pkg/cmd/cockroach:cockroach"
+	nogoDisableFlag    = "--//build/toolchains:nogo_disable_flag"
+	geosTarget         = "//c-deps:libgeos"
+	devTarget          = "//pkg/cmd/dev:dev"
 )
 
 type buildTarget struct {
@@ -77,9 +79,9 @@ var buildTargetMapping = map[string]string{
 	"bazel-remote":         bazelRemoteTarget,
 	"buildifier":           "@com_github_bazelbuild_buildtools//buildifier:buildifier",
 	"buildozer":            "@com_github_bazelbuild_buildtools//buildozer:buildozer",
-	"cockroach":            "//pkg/cmd/cockroach:cockroach",
+	"cockroach":            cockroachTarget,
 	"cockroach-sql":        "//pkg/cmd/cockroach-sql:cockroach-sql",
-	"cockroach-oss":        "//pkg/cmd/cockroach-oss:cockroach-oss",
+	"cockroach-oss":        cockroachTargetOss,
 	"cockroach-short":      "//pkg/cmd/cockroach-short:cockroach-short",
 	"crlfmt":               "@com_github_cockroachdb_crlfmt//:crlfmt",
 	"dev":                  devTarget,
@@ -95,7 +97,7 @@ var buildTargetMapping = map[string]string{
 	"obsservice":           "//pkg/obsservice/cmd/obsservice:obsservice",
 	"optgen":               "//pkg/sql/opt/optgen/cmd/optgen:optgen",
 	"optfmt":               "//pkg/sql/opt/optgen/cmd/optfmt:optfmt",
-	"oss":                  "//pkg/cmd/cockroach-oss:cockroach-oss",
+	"oss":                  cockroachTargetOss,
 	"reduce":               "//pkg/cmd/reduce:reduce",
 	"roachprod":            "//pkg/cmd/roachprod:roachprod",
 	"roachprod-stress":     "//pkg/cmd/roachprod-stress:roachprod-stress",
@@ -171,6 +173,10 @@ func (d *dev) build(cmd *cobra.Command, commandLine []string) error {
 	}
 	args = append(args, additionalBazelArgs...)
 
+	if err := d.assertNoLinkedNpmDeps(buildTargets); err != nil {
+		return err
+	}
+
 	if cross == "" {
 		// Do not log --build_event_binary_file=... because it is not relevant to the actual call
 		// from the user perspective.

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

@@ -208,6 +208,34 @@ func (o *OS) Readlink(filename string) (string, error) {
 	})
 }
 
+// ReadDir is a thin wrapper around os.ReadDir, which returns the names of files
+// or directories within dirname.
+func (o *OS) ReadDir(dirname string) ([]string, error) {
+	command := fmt.Sprintf("ls %s", dirname)
+	if !o.knobs.silent {
+		o.logger.Print(command)
+	}
+
+	output, err := o.Next(command, func() (string, error) {
+		var ret []string
+		entries, err := os.ReadDir(dirname)
+		if err != nil {
+			return "", err
+		}
+
+		for _, entry := range entries {
+			ret = append(ret, entry.Name())
+		}
+
+		return fmt.Sprintf("%s\n", strings.Join(ret, "\n")), nil
+	})
+
+	if err != nil {
+		return nil, err
+	}
+	return strings.Split(strings.TrimSpace(output), "\n"), nil
+}
+
 // IsDir wraps around os.Stat, which returns the os.FileInfo of the named
 // directory. IsDir returns true if and only if it is an existing directory.
 // If there is an error, it will be of type *PathError.
@@ -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())
 		}
 		return fmt.Sprintf("%s\n", strings.Join(ret, "\n")), nil
 	})

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

@@ -49,6 +49,14 @@ cp sandbox/pkg/cmd/cockroach-short/cockroach-short_/cockroach-short crdb-checkou
 exec
 dev build -- --verbose_failures --sandbox_debug
 ----
+bazel info workspace --color=no
+ls crdb-checkout/pkg/ui/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/eslint-plugin-crdb/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/db-console/src/js/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/db-console/ccl/src/js/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/cluster-ui/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/db-console/node_modules/@cockroachlabs
+ls crdb-checkout/pkg/ui/workspaces/e2e-tests/node_modules/@cockroachlabs
 bazel build //pkg/cmd/cockroach:cockroach --verbose_failures --sandbox_debug --build_event_binary_file=/tmp/path
 bazel info workspace --color=no
 mkdir crdb-checkout/bin

pkg/cmd/dev/ui.go

@@ -10,6 +10,7 @@
 package main
 
 import (
+	"errors"
 	"fmt"
 	"log"
 	"net/url"
@@ -48,6 +49,8 @@ func makeUICmd(d *dev) *cobra.Command {
 
 // UIDirectories contains the absolute path to the root of each UI sub-project.
 type UIDirectories struct {
+	workspace string
+	// workspace is the absolute path to ./ .
 	// root is the absolute path to ./pkg/ui.
 	root string
 	// clusterUI is the absolute path to ./pkg/ui/workspaces/cluster-ui.
@@ -58,6 +61,10 @@ type UIDirectories struct {
 	e2eTests string
 	// eslintPlugin is the absolute path to ./pkg/ui/workspaces/eslint-plugin-crdb.
 	eslintPlugin string
+	// protoOss is the absolute path to ./pkg/ui/workspaces/db-console/src/js/.
+	protoOss string
+	// protoCcl is the absolute path to ./pkg/ui/workspaces/db-console/ccl/src/js/.
+	protoCcl string
 }
 
 // getUIDirs computes the absolute path to the root of each UI sub-project.
@@ -68,14 +75,168 @@ func getUIDirs(d *dev) (*UIDirectories, error) {
 	}
 
 	return &UIDirectories{
+		workspace:    workspace,
 		root:         filepath.Join(workspace, "./pkg/ui"),
 		clusterUI:    filepath.Join(workspace, "./pkg/ui/workspaces/cluster-ui"),
 		dbConsole:    filepath.Join(workspace, "./pkg/ui/workspaces/db-console"),
 		e2eTests:     filepath.Join(workspace, "./pkg/ui/workspaces/e2e-tests"),
 		eslintPlugin: filepath.Join(workspace, "./pkg/ui/workspaces/eslint-plugin-crdb"),
+		protoOss:     filepath.Join(workspace, "./pkg/ui/workspaces/db-console/src/js"),
+		protoCcl:     filepath.Join(workspace, "./pkg/ui/workspaces/db-console/ccl/src/js"),
 	}, nil
 }
 
+// assertNoLinkedNpmDeps looks for JS packages linked outside the Bazel
+// workspace (typically via `pnpm link`). It returns an error if:
+//
+// 'targets' contains a Bazel target that requires the web UI
+// AND
+// a node_modules/ tree exists within pkg/ui (or its subtrees)
+// AND
+// a @cockroachlabs-scoped package is symlinked to an external directory
+//
+// (or if any error occurs while performing one of those checks).
+func (d *dev) assertNoLinkedNpmDeps(targets []buildTarget) error {
+	uiWillBeBuilt := false
+	for _, target := range targets {
+		// TODO: This could potentially be a bazel query, e.g.
+		// 'somepath(${target.fullName}, //pkg/ui/workspaces/db-console:*)' or
+		// similar, but with only two eligible targets it doesn't seem quite
+		// worth it.
+		if target.fullName == cockroachTarget || target.fullName == cockroachTargetOss {
+			uiWillBeBuilt = true
+			break
+		}
+	}
+	if !uiWillBeBuilt {
+		// If no UI build is required, the presence of an externally-linked
+		// package doesn't matter.
+		return nil
+	}
+
+	// Find the current workspace and build some relevant absolute paths.
+	uiDirs, err := getUIDirs(d)
+	if err != nil {
+		return fmt.Errorf("could not check for linked NPM dependencies: %w", err)
+	}
+
+	jsPkgRoots := []string{
+		uiDirs.root,
+		uiDirs.eslintPlugin,
+		uiDirs.protoOss,
+		uiDirs.protoCcl,
+		uiDirs.clusterUI,
+		uiDirs.dbConsole,
+		uiDirs.e2eTests,
+	}
+
+	type LinkedPackage struct {
+		name string
+		dir  string
+	}
+
+	anyPackageEscapesWorkspace := false
+
+	// Check for symlinks in each package's node_modules/@cockroachlabs/ dir.
+	for _, jsPkgRoot := range jsPkgRoots {
+		crlModulesPath := filepath.Join(jsPkgRoot, "node_modules/@cockroachlabs")
+		crlDeps, err := d.os.ReadDir(crlModulesPath)
+
+		// If node_modules/@cockroachlabs doesn't exist, it's likely that JS
+		// dependencies haven't been installed outside the Bazel workspace.
+		// This is expected for non-UI devs, and is a safe state.
+		if errors.Is(err, os.ErrNotExist) {
+			continue
+		}
+		if err != nil {
+			return fmt.Errorf("could not @cockroachlabs/ packages: %w", err)
+		}
+
+		linkedPackages := []LinkedPackage{}
+
+		// For each dependency in node_modules/@cockroachlabs/ ...
+		for _, depName := range crlDeps {
+			// Ignore empty strings, which are produced by d.os.ReadDir in
+			// dry-run mode.
+			if depName == "" {
+				continue
+			}
+
+			// Resolve the possible symlink.
+			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)
+			}
+
+			// Convert it to a path relative to the Bazel workspace root.
+			relativeToWorkspace, err := filepath.Rel(
+				uiDirs.workspace,
+				filepath.Join(crlModulesPath, resolved),
+			)
+			if err != nil {
+				return fmt.Errorf("could not relitivize path %s: %w", resolved, err)
+			}
+
+			// If it doesn't start with '..', it doesn't escape the Bazel
+			// workspace.
+			// TODO: Once Go 1.20 is supported here, switch to filepath.IsLocal.
+			if !strings.HasPrefix(relativeToWorkspace, "..") {
+				continue
+			}
+
+			// This package escapes the Bazel workspace! Add it to the queue.
+			abs, err := filepath.Abs(relativeToWorkspace)
+			if err != nil {
+				return fmt.Errorf("could not absolutize path %s: %w", resolved, err)
+			}
+
+			linkedPackages = append(
+				linkedPackages,
+				LinkedPackage{
+					name: "@cockroachlabs/" + depName,
+					dir:  abs,
+				},
+			)
+		}
+
+		// If this internal package has no dependencies provided by pnpm link,
+		// move on without logging anything.
+		if len(linkedPackages) == 0 {
+			continue
+		}
+
+		if !anyPackageEscapesWorkspace {
+			anyPackageEscapesWorkspace = true
+			log.Println("Externally-linked package(s) detected:")
+		}
+
+		log.Printf("pkg/ui/workspaces/%s:", filepath.Base(jsPkgRoot))
+		for _, pkg := range linkedPackages {
+			log.Printf("    %s <- %s\n", pkg.name, pkg.dir)
+		}
+		log.Println()
+	}
+
+	if anyPackageEscapesWorkspace {
+		msg := strings.TrimSpace(`
+At least one JS dependency is linked to another directory on your machine.
+Bazel cannot see changes in these packages, which could lead to both
+false-positive and false-negative behavior in the UI.
+This build has been pre-emptively failed.
+
+To build without the UI, run:
+    dev build short
+To remove all linked dependencies, run:
+    dev ui clean --all
+`) + "\n"
+
+		return fmt.Errorf("%s", msg)
+	}
+
+	return nil
+}
+
 // makeUIWatchCmd initializes the 'ui watch' subcommand, which sets up a
 // live-reloading HTTP server for db-console and a file-watching rebuilder for
 // cluster-ui.