diff --git a/pkg/cmd/dev/build.go b/pkg/cmd/dev/build.go index 9a46e511baff..328d08cb5372 100644 --- a/pkg/cmd/dev/build.go +++ b/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. diff --git a/pkg/cmd/dev/io/os/os.go b/pkg/cmd/dev/io/os/os.go index e078fc6cde27..a2322b703835 100644 --- a/pkg/cmd/dev/io/os/os.go +++ b/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 }) diff --git a/pkg/cmd/dev/testdata/datadriven/dev-build b/pkg/cmd/dev/testdata/datadriven/dev-build index 075e909a1ff1..1b849a275557 100644 --- a/pkg/cmd/dev/testdata/datadriven/dev-build +++ b/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 diff --git a/pkg/cmd/dev/ui.go b/pkg/cmd/dev/ui.go index f2cd4269a05c..45d9252600a0 100644 --- a/pkg/cmd/dev/ui.go +++ b/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.