Merge cockroachdb#107110 cockroachdb#107111 cockroachdb#107129 cockroachdb#107149 cockroachdb#107181 cockroachdb#107183

107110: kvserver: avoid running intensive decommission test under deadlock r=AlexTalks a=AlexTalks

The kvserver test `TestDecommission`, which runs a 5-node cluster and decommissions 4 of those 5 nodes, has trouble completing fast enough when under a race or deadlock configuration. While race configurations were already skipped, this modifies the test to be skipped under deadlock configurations as well.

Fixes: cockroachdb#106096

Release note: None

107111: compose: start `docker-compose` with a non-empty `PATH` r=rail a=rickystewart

`docker-compose` invokes `docker`, but obviously this will fail if there is nothing in the `PATH`.

Epic: none
Release note: None

107129: dev: reject builds when CRL JS dependencies are 'pnpm link'ed r=rickystewart a=sjbarag

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

-----

Example output: 
<img width="998" alt="image" src="https://github.com/cockroachdb/cockroach/assets/665775/3fd43abe-f5c2-4ddd-bc60-16a73db12836">

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.

107149: opt: make functional dependency calculation deterministic r=DrewKimball a=DrewKimball

We recently added additional logic for inferring functional dependencies for join expressions. However, this logic iterates through a map, which leads to nondeterminism in which order functional dependencies are added to the FD set. Functional dependency calculation is best-effort, so this can lead to a different resulting FD set, which causes flaky tests. This patch makes the calculation deterministic by iterating instead through a `intsets.Fast` set.

Fixes cockroachdb#107148
Fixes cockroachdb#107162

Release note: None

107181: dev: when cross-building, use `-c opt` r=rail a=rickystewart

This enables optimizations which you probably want for a cross-build.

Epic: CRDB-17171
Release note: None

107183: acceptance: add log dir as a writable path r=rail a=rickystewart

Otherwise you get a sandbox error.

Epic: CRDB-17171
Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Drew Kimball <[email protected]>

Author: 5 people

dev

@@ -8,7 +8,7 @@ fi
 set -euo pipefail
 
 # Bump this counter to force rebuilding `dev` on all machines.
-DEV_VERSION=80
+DEV_VERSION=81
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 BINARY_DIR=$THIS_DIR/bin/dev-versions

pkg/cmd/dev/acceptance.go

@@ -93,6 +93,7 @@ func (d *dev) acceptance(cmd *cobra.Command, commandLine []string) error {
 		args = append(args, "--test_arg", "-test.v")
 	}
 	args = append(args, fmt.Sprintf("--test_arg=-l=%s", logDir))
+	args = append(args, fmt.Sprintf("--sandbox_writable_path=%s", logDir))
 	args = append(args, "--test_env=TZ=America/New_York")
 	args = append(args, fmt.Sprintf("--test_arg=-b=%s", cockroachBin))
 	args = append(args, additionalBazelArgs...)

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",
@@ -173,6 +175,10 @@ func (d *dev) build(cmd *cobra.Command, commandLine []string) error {
 	configArgs := getConfigArgs(args)
 	configArgs = append(configArgs, getConfigArgs(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.
@@ -200,7 +206,7 @@ func (d *dev) crossBuild(
 	volume string,
 	dockerArgs []string,
 ) error {
-	bazelArgs = append(bazelArgs, fmt.Sprintf("--config=%s", crossConfig), "--config=ci")
+	bazelArgs = append(bazelArgs, fmt.Sprintf("--config=%s", crossConfig), "--config=ci", "-c", "opt")
 	configArgs := getConfigArgs(bazelArgs)
 	dockerArgs, err := d.getDockerRunArgs(ctx, volume, false, dockerArgs)
 	if err != nil {

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.

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,170 @@ 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 to make
+			// it obvious when a link escapes the workspace.
+			relativeToWorkspace, err := filepath.Rel(
+				uiDirs.workspace,
+				filepath.Join(crlModulesPath, resolved),
+			)
+			if err != nil {
+				return fmt.Errorf("could not relativize 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
+			// with its absolute path for simpler presentation to users.
+			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.

pkg/compose/compose_test.go

@@ -115,6 +115,7 @@ func TestComposeCompare(t *testing.T) {
 		fmt.Sprintf("COMPARE_DIR_PATH=%s", compareDir),
 		fmt.Sprintf("ARTIFACTS=%s", *flagArtifacts),
 		fmt.Sprintf("COCKROACH_DEV_LICENSE=%s", envutil.EnvOrDefaultString("COCKROACH_DEV_LICENSE", "")),
+		fmt.Sprintf("PATH=%s", os.Getenv("PATH")),
 	}
 	out, err := cmd.CombinedOutput()
 	if err != nil {

pkg/kv/kvserver/client_decommission_test.go

@@ -34,9 +34,10 @@ func TestDecommission(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	// Five nodes is too much to reliably run under testrace with our aggressive
-	// liveness timings.
-	skip.UnderRace(t, "#39807 and #37811")
+	// Five nodes is too much to reliably run under race/deadlock with our
+	// aggressive liveness timings.
+	skip.UnderRaceWithIssue(t, 39807, "#39807 and #37811")
+	skip.UnderDeadlockWithIssue(t, 39807, "#39807 and #37811")
 
 	ctx := context.Background()
 	tc := testcluster.StartTestCluster(t, 5, base.TestClusterArgs{