From 2892546e561b18722837b74185dabd9cfc3cbb95 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Wed, 30 Mar 2022 11:53:17 -0500 Subject: [PATCH] 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 #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 #73944. Closes #78666. Release note: None --- .bazelrc | 8 ++++++- BUILD.bazel | 4 ++-- build/bazelutil/BUILD.bazel | 13 ++++++++++ build/toolchains/BUILD.bazel | 36 +++++++++++++++++++++++++--- dev | 12 ++++++---- pkg/cmd/dev/cache.go | 4 ++-- pkg/cmd/dev/doctor.go | 19 ++++++++++++++- pkg/cmd/dev/lint.go | 11 ++++++++- pkg/cmd/dev/testdata/datadriven/lint | 2 ++ 9 files changed, 95 insertions(+), 14 deletions(-) diff --git a/.bazelrc b/.bazelrc index 8aca4198cb52..7f57f565677a 100644 --- a/.bazelrc +++ b/.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 diff --git a/BUILD.bazel b/BUILD.bazel index a3bb825b1423..7e798c788655 100644 --- a/BUILD.bazel +++ b/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": [], }), ) diff --git a/build/bazelutil/BUILD.bazel b/build/bazelutil/BUILD.bazel index 7124380eb6a5..335f1421c102 100644 --- a/build/bazelutil/BUILD.bazel +++ b/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", diff --git a/build/toolchains/BUILD.bazel b/build/toolchains/BUILD.bazel index d58fabd02181..71c3b2ea8752 100644 --- a/build/toolchains/BUILD.bazel +++ b/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__"], +) diff --git a/dev b/dev index 2e6aa325b802..26b02ca7c4c0 100755 --- a/dev +++ b/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 diff --git a/pkg/cmd/dev/cache.go b/pkg/cmd/dev/cache.go index ad81cddd23a9..6ee837424586 100644 --- a/pkg/cmd/dev/cache.go +++ b/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 } diff --git a/pkg/cmd/dev/doctor.go b/pkg/cmd/dev/doctor.go index 9bc1cf607cea..c97e2c565c19 100644 --- a/pkg/cmd/dev/doctor.go +++ b/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 { diff --git a/pkg/cmd/dev/lint.go b/pkg/cmd/dev/lint.go index 612a2312f097..44fdece75d10 100644 --- a/pkg/cmd/dev/lint.go +++ b/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 } diff --git a/pkg/cmd/dev/testdata/datadriven/lint b/pkg/cmd/dev/testdata/datadriven/lint index d0555704f9bc..5ea64f647a5b 100644 --- a/pkg/cmd/dev/testdata/datadriven/lint +++ b/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