dev,bazel: don't do nogo checks by default; have doctor advise

Up until this point `nogo` has been enabled by default and we've
required opting out with `--config nonogo`. Unfortunately `nogo` has a
non-negligible performance impact (see cockroachdb#73944) so having it on by
default is not what we want to be doing. Preserving the current behavior
as opt-in is still desirable though.

We have `dev doctor` advise about this case and recommend that you
explicitly set one of `--config lintonbuild` or
`--config nolintonbuild`. If you have neither or both configured,
`doctor` will detect that. (You can still override one with the other on
the command line.) We also have `dev lint` build `cockroach-short` with
`nogo` if running in non-`short` mode to cover that case for people who
have opted out of `nogo`.

`--config nonogo` is still supported as an alias for `nolintonbuild`.

Closes cockroachdb#73944.
Closes cockroachdb#78666.

Release note: None

.bazelrc

@@ -3,11 +3,16 @@
 build --flag_alias=crdb_test=//build/toolchains:crdb_test_flag
 build --flag_alias=cross=//build/toolchains:cross_flag
 build --flag_alias=dev=//build/toolchains:dev_flag
+build --flag_alias=lintonbuild=//build/toolchains:nogo_flag
+build --flag_alias=nolintonbuild=//build/toolchains:nonogo_explicit_flag
 build --flag_alias=with_ui=//pkg/ui:with_ui_flag
 
 build:cross --cross
 build:dev --dev
-build:nonogo --//build/toolchains:nonogo_flag
+build:lintonbuild --lintonbuild
+build:nolintonbuild --nolintonbuild
+# Note: nonogo is classically the name of the nolintonbuild configuration.
+build:nonogo --nolintonbuild
 build:test --crdb_test
 build:with_ui --with_ui
 
@@ -28,6 +33,7 @@ test:race --test_timeout=1200,6000,18000,72000
 
 # CI should always run with `--config=ci`.
 build:ci --experimental_convenience_symlinks=ignore
+build:ci --nogo
 # Set `-test.v` in Go tests.
 # Ref: https://github.com/bazelbuild/rules_go/pull/2456
 test:ci --test_env=GO_TEST_WRAP_TESTV=1

BUILD.bazel

@@ -143,8 +143,7 @@ nogo(
     config = "//build/bazelutil:nogo_config.json",
     visibility = ["//visibility:public"],
     deps = select({
-        "//build/toolchains:nonogo": [],
-        "//conditions:default": [
+        "//build/toolchains:nogo": [
             "@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/assign:go_default_library",
             "@org_golang_x_tools//go/analysis/passes/atomic:go_default_library",
@@ -198,5 +197,6 @@ nogo(
             "//pkg/testutils/lint/passes/timer",
             "//pkg/testutils/lint/passes/unconvert",
         ] + STATICCHECK_CHECKS,
+        "//conditions:default": [],
     }),
 )

build/bazelutil/BUILD.bazel

@@ -10,6 +10,19 @@ genrule(
     stamp = True,
 )
 
+NOGO_FAILED_TEST_OUTPUT = "echo 'must use exactly one of `--config nogo` or `--config nonogo` explicitly'; exit 1"
+
+genrule(
+    name = "test_nogo_configured",
+    outs = ["empty.txt"],
+    cmd = select({
+        "//build/toolchains:both_nogo_and_nonogo": NOGO_FAILED_TEST_OUTPUT,
+        "//build/toolchains:nogo": "touch $@",
+        "//build/toolchains:nonogo_explicit": "touch $@",
+        "//conditions:default": NOGO_FAILED_TEST_OUTPUT,
+    }),
+)
+
 lint_binary(
     name = "lint",
     test = "//pkg/testutils/lint:lint_test",

build/toolchains/BUILD.bazel

@@ -192,14 +192,44 @@ config_setting(
 )
 
 bool_flag(
-    name = "nonogo_flag",
+    name = "nogo_flag",
     build_setting_default = False,
     visibility = ["//visibility:public"],
 )
 
 config_setting(
-    name = "nonogo",
+    name = "nogo",
     flag_values = {
-        ":nonogo_flag": "true",
+        ":nogo_flag": "true",
+        ":nonogo_explicit_flag": "false",
     },
 )
+
+# Note: the flag nonogo_flag and config_setting nonogo_explicit aren't meant
+# to be directly used in select()'s. Not using nogo is the default behavior.
+# The flag and config_setting are here solely so that they can be used by `dev`
+# to check whether an option is configured.
+bool_flag(
+    name = "nonogo_explicit_flag",
+    build_setting_default = False,
+    visibility = ["//visibility:public"],
+)
+
+config_setting(
+    name = "nonogo_explicit",
+    flag_values = {
+        ":nonogo_explicit_flag": "true",
+    },
+    visibility = ["//build/bazelutil:__pkg__"],
+)
+
+# This config setting is here to check the error case where the user has
+# configured both nogo and nonogo.
+config_setting(
+    name = "both_nogo_and_nonogo",
+    flag_values = {
+        ":nogo_flag": "true",
+        ":nonogo_explicit_flag": "true",
+    },
+    visibility = ["//build/bazelutil:__pkg__"],
+)

dev

@@ -3,17 +3,21 @@
 set -euo pipefail
 
 # Bump this counter to force rebuilding `dev` on all machines.
-DEV_VERSION=26
+DEV_VERSION=27
 
 THIS_DIR=$(cd "$(dirname "$0")" && pwd)
 BINARY_DIR=$THIS_DIR/bin/dev-versions
 BINARY_PATH=$BINARY_DIR/dev.$DEV_VERSION
 
-if [[ ! -f "$BINARY_PATH" || ! -z "${DEV_FORCE_REBUILD-}" ]]; then
+if [[ -f "$BINARY_PATH" && ! -z "${DEV_FORCE_REBUILD-}" ]]; then
+    rm "$BINARY_PATH"
+fi
+
+if [[ ! -f "$BINARY_PATH" ]]; then
     echo "$BINARY_PATH not found, building..."
     mkdir -p $BINARY_DIR
-    bazel build //pkg/cmd/dev --config nonogo
-    cp $(bazel info bazel-bin --config nonogo)/pkg/cmd/dev/dev_/dev $BINARY_PATH
+    bazel build //pkg/cmd/dev --//build/toolchains:nogo_flag
+    cp $(bazel info bazel-bin --//build/toolchains:nogo_flag)/pkg/cmd/dev/dev_/dev $BINARY_PATH
     # The Bazel-built binary won't have write permissions.
     chmod a+w $BINARY_PATH
 fi

pkg/cmd/dev/cache.go

@@ -136,11 +136,11 @@ func (d *dev) setUpCache(ctx context.Context) (string, error) {
 
 	log.Printf("Configuring cache...\n")
 
-	err := d.exec.CommandContextInheritingStdStreams(ctx, "bazel", "build", bazelRemoteTarget)
+	err := d.exec.CommandContextInheritingStdStreams(ctx, "bazel", "build", bazelRemoteTarget, "--//build/toolchains:nonogo_explicit_flag")
 	if err != nil {
 		return "", err
 	}
-	bazelRemoteLoc, err := d.exec.CommandContextSilent(ctx, "bazel", "run", bazelRemoteTarget, "--run_under=//build/bazelutil/whereis")
+	bazelRemoteLoc, err := d.exec.CommandContextSilent(ctx, "bazel", "run", bazelRemoteTarget, "--//build/toolchains:nonogo_explicit_flag", "--run_under=//build/bazelutil/whereis")
 	if err != nil {
 		return "", err
 	}

pkg/cmd/dev/doctor.go

@@ -32,7 +32,7 @@ const (
 	// doctorStatusVersion is the current "version" of the status checks performed
 	// by `dev doctor``. Increasing it will force doctor to be re-run before other
 	// dev commands can be run.
-	doctorStatusVersion = 2
+	doctorStatusVersion = 3
 
 	noCacheFlag = "no-cache"
 )
@@ -220,6 +220,23 @@ Make sure one of the following lines is in the file %s/.bazelrc.user:
 		failures = append(failures, failedStampTestMsg)
 	}
 
+	// Check whether linting during builds (nogo) is explicitly configured
+	// before we get started.
+	stdout, err = d.exec.CommandContextSilent(ctx, "bazel", "build", "//build/bazelutil:test_nogo_configured")
+	if err != nil {
+		failedNogoTestMsg := "Failed to run `bazel build //build/bazelutil:test_nogo_configured. " + `
+This may be because you haven't configured whether to run lints during builds.
+Put EXACTLY ONE of the following lines in your .bazelrc.user:
+    build --config lintonbuild
+        OR
+    build --config nolintonbuild
+The former will run lint checks while you build. This will make incremental builds
+slightly slower and introduce a noticeable delay in first-time build setup.`
+		failures = append(failures, failedNogoTestMsg)
+		log.Println(failedNogoTestMsg)
+		printStdoutAndErr(string(stdout), err)
+	}
+
 	// We want to make sure there are no other failures before trying to
 	// set up the cache.
 	if !noCache && len(failures) == 0 {

pkg/cmd/dev/lint.go

@@ -76,5 +76,14 @@ func (d *dev) lint(cmd *cobra.Command, commandLine []string) error {
 		env = append(env, envvar)
 		return d.exec.CommandContextWithEnv(ctx, env, "bazel", args...)
 	}
-	return d.exec.CommandContextInheritingStdStreams(ctx, "bazel", args...)
+	err := d.exec.CommandContextInheritingStdStreams(ctx, "bazel", args...)
+	if err != nil {
+		return err
+	}
+	if !short {
+		args := []string{"build", "//pkg/cmd/cockroach-short", "--//build/toolchains:nogo_flag"}
+		logCommand("bazel", args...)
+		return d.exec.CommandContextInheritingStdStreams(ctx, "bazel", args...)
+	}
+	return nil
 }

pkg/cmd/dev/testdata/datadriven/lint

@@ -2,6 +2,7 @@ exec
 dev lint
 ----
 bazel run --config=test //build/bazelutil:lint -- -test.v
+bazel build //pkg/cmd/cockroach-short --//build/toolchains:nogo_flag
 
 exec
 dev lint --short --timeout=5m
@@ -18,6 +19,7 @@ exec
 dev lint -f TestLowercaseFunctionNames --cpus 4
 ----
 bazel run --config=test //build/bazelutil:lint --local_cpu_resources=4 -- -test.v -test.run Lint/TestLowercaseFunctionNames
+bazel build //pkg/cmd/cockroach-short --//build/toolchains:nogo_flag
 
 exec
 dev lint pkg/cmd/dev pkg/spanconfig