From 155a8fa75caa925864f048d2b27ba53f0833f6f5 Mon Sep 17 00:00:00 2001 From: Ricky Stewart Date: Fri, 12 Mar 2021 15:10:35 -0600 Subject: [PATCH] bazel: provide way to run lint in bazel With this patch, lints can be run with `bazel run build/bazelutil:lint`. The test binary takes the typical Go command-line arguments, so e.g. `bazel run build/bazelutil:lint -- -test.v`. We model this as a `sh_binary` that depends on the lint test binary as well as all the dependencies that it has (`roachvet`, `crlfmt`, and others). The shell script is a light wrapper that finds the actual location of all the dependencies and stages them all in the `PATH` appropriately. We also need to munge `LDFLAGS` because the linter calls into the compiler in a couple places, and `cgo` wants to link into some of the C libraries (`libedit`, `libproj`, etc.) There are a couple tests that are still broken, which are skipped for now. As a next step, we'll want to get this running in CI. Closes #61555 Release note: None --- DEPS.bzl | 4 ++ build/bazelutil/BUILD.bazel | 6 ++ build/bazelutil/lint.bzl | 65 +++++++++++++++++++ build/bazelutil/lint.sh.in | 52 +++++++++++++++ build/patches/com_github_knz_go_libedit.patch | 29 +++++++++ c-deps/BUILD.bazel | 29 +++++++++ pkg/build/bazel/bazel.go | 2 +- pkg/testutils/lint/BUILD.bazel | 25 ++++++- pkg/testutils/lint/lint_test.go | 24 +++++++ 9 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 build/bazelutil/BUILD.bazel create mode 100644 build/bazelutil/lint.bzl create mode 100644 build/bazelutil/lint.sh.in create mode 100644 build/patches/com_github_knz_go_libedit.patch diff --git a/DEPS.bzl b/DEPS.bzl index 509a0084fccf..e8af102abc50 100644 --- a/DEPS.bzl +++ b/DEPS.bzl @@ -2343,6 +2343,10 @@ def go_deps(): name = "com_github_knz_go_libedit", build_file_proto_mode = "disable_global", importpath = "github.com/knz/go-libedit", + patch_args = ["-p1"], + patches = [ + "@cockroach//build/patches:com_github_knz_go_libedit.patch", + ], replace = "github.com/otan-cockroach/go-libedit", sum = "h1:+sIdymRXD4aKCvmVMBLL7/bO95KZFYrbz0EzQ1Jlj4A=", version = "v1.10.2-0.20201030151939-7cced08450e7", diff --git a/build/bazelutil/BUILD.bazel b/build/bazelutil/BUILD.bazel new file mode 100644 index 000000000000..0cf5e95f072e --- /dev/null +++ b/build/bazelutil/BUILD.bazel @@ -0,0 +1,6 @@ +load(":lint.bzl", "lint_binary") + +lint_binary( + name = "lint", + test = "//pkg/testutils/lint:lint_test", +) diff --git a/build/bazelutil/lint.bzl b/build/bazelutil/lint.bzl new file mode 100644 index 000000000000..d47ef2def437 --- /dev/null +++ b/build/bazelutil/lint.bzl @@ -0,0 +1,65 @@ +load("@bazel_skylib//lib:shell.bzl", "shell") + +# lint_binary works as follows: +# 1. For each test, we generate a script, which uses linttest.sh.in as a +# template. It simply bootstraps the environment by locating the go SDK, +# setting an appropriate `PATH` and `GOROOT`, and cd-ing to the right +# directory in the workspace. This roughly replicates what `go test` would +# do. +# 2. Using that script, we create a `sh_binary` using that script as an entry +# point with the appropriate dependencies. + +def _gen_script_impl(ctx): + subs = { + "@@PACKAGE@@": shell.quote(ctx.attr.test.label.package), + "@@NAME@@": shell.quote(ctx.attr.test.label.name), + } + out_file = ctx.actions.declare_file(ctx.label.name) + ctx.actions.expand_template( + template = ctx.file._template, + output = out_file, + substitutions = subs, + ) + return [ + DefaultInfo(files = depset([out_file])), + ] + +_gen_script = rule( + implementation = _gen_script_impl, + attrs = { + "test": attr.label(mandatory = True), + "_template": attr.label( + default = "@cockroach//build/bazelutil:lint.sh.in", + allow_single_file = True) + }, +) + +def lint_binary(name, test): + script_name = name + ".sh" + _gen_script( + name = script_name, + test = test, + testonly = 1, + ) + native.sh_binary( + name = name, + srcs = [script_name], + data = [ + test, + "//c-deps:libedit_files", + "//c-deps:libgeos_files", + "//c-deps:libproj_files", + "//c-deps:libroach_files", + "//pkg/cmd/returncheck", + "//pkg/cmd/roachvet", + "//pkg/sql/opt/optgen/cmd/optfmt", + "@co_honnef_go_tools//cmd/staticcheck", + "@com_github_client9_misspell//cmd/misspell:misspell", + "@com_github_cockroachdb_crlfmt//:crlfmt", + "@com_github_kisielk_errcheck//:errcheck", + "@go_sdk//:bin/go", + "@org_golang_x_lint//golint:golint", + ], + deps = ["@bazel_tools//tools/bash/runfiles"], + testonly = 1, + ) diff --git a/build/bazelutil/lint.sh.in b/build/bazelutil/lint.sh.in new file mode 100644 index 000000000000..515eb233671b --- /dev/null +++ b/build/bazelutil/lint.sh.in @@ -0,0 +1,52 @@ +# This is boilerplate taken directly from +# https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash +# See that page for an explanation of what this is and why it's necessary. +# --- begin runfiles.bash initialization v2 --- +# Copy-pasted from the Bazel Bash runfiles library v2. +set -uo pipefail; f=bazel_tools/tools/bash/runfiles/runfiles.bash +source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \ + source "$0.runfiles/$f" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ + { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e +# --- end runfiles.bash initialization v2 --- + +PACKAGE=@@PACKAGE@@ +NAME=@@NAME@@ + +# Wrap rlocation such that we immediately fail if a dep is not found. +rlocation_ck () { + loc="$(rlocation $1)" + if [ -z "${loc-}" ]; then + echo "error: could not find the location of $1" >&2 + exit 1 + fi + echo $loc +} + +test_bin="$(rlocation_ck cockroach/$PACKAGE/${NAME}_/$NAME)" +go_bin="$(rlocation_ck go_sdk/bin/go)" +crlfmt_bin="$(rlocation_ck com_github_cockroachdb_crlfmt/crlfmt_/crlfmt)" +errcheck_bin="$(rlocation_ck com_github_kisielk_errcheck/errcheck_/errcheck)" +golint_bin="$(rlocation_ck org_golang_x_lint/golint/golint_/golint)" +misspell_bin="$(rlocation_ck com_github_client9_misspell/cmd/misspell/misspell_/misspell)" +optfmt_bin="$(rlocation_ck cockroach/pkg/sql/opt/optgen/cmd/optfmt/optfmt_/optfmt)" +returncheck_bin="$(rlocation_ck cockroach/pkg/cmd/returncheck/returncheck_/returncheck)" +roachvet_bin="$(rlocation_ck cockroach/pkg/cmd/roachvet/roachvet_/roachvet)" +staticcheck_bin="$(rlocation_ck co_honnef_go_tools/cmd/staticcheck/staticcheck_/staticcheck)" + +# Need to run this so that Go can find the runfiles. +runfiles_export_envvars + +if [ -z "${BUILD_WORKSPACE_DIRECTORY-}" ]; then + echo "error: BUILD_WORKSPACE_DIRECTORY not set" >&2 + exit 1 +fi + +cd "$BUILD_WORKSPACE_DIRECTORY/$PACKAGE" + +TEST_WORKSPACE=cockroach \ + PATH="$(dirname $go_bin):$(dirname $roachvet_bin):$(dirname $errcheck_bin):$(dirname $returncheck_bin):$(dirname $staticcheck_bin):$(dirname $crlfmt_bin):$(dirname $misspell_bin):$(dirname $golint_bin):$(dirname $optfmt_bin):$PATH" \ + GOROOT="$(dirname $(dirname $go_bin))" \ + $test_bin $@ diff --git a/build/patches/com_github_knz_go_libedit.patch b/build/patches/com_github_knz_go_libedit.patch new file mode 100644 index 000000000000..8e25a7f9feb2 --- /dev/null +++ b/build/patches/com_github_knz_go_libedit.patch @@ -0,0 +1,29 @@ +diff -urN a/unix/BUILD.bazel b/unix/BUILD.bazel +--- a/unix/BUILD.bazel 1969-12-31 19:00:00.000000000 -0500 ++++ b/unix/BUILD.bazel 2000-01-01 00:00:00.000000000 -0000 +@@ -1,7 +1,7 @@ + load("@io_bazel_rules_go//go:def.bzl", "go_library") + + cc_library( +- name = "libedit_unix_cdeps", ++ name = "edit", + srcs = select({ + "@io_bazel_rules_go//go/platform:android": glob([ + "src/*.c", +@@ -45,6 +45,7 @@ + "//conditions:default": "shim", + }), + copts = ["-DGO_LIBEDIT_NO_BUILD"], ++ visibility = ["//visibility:public"], + ) + + go_library( +@@ -86,7 +87,7 @@ + "wrap-wcsdup.c", + ], + cdeps = [ +- ":libedit_unix_cdeps", ++ ":edit", + ], + cgo = True, + clinkopts = select({ diff --git a/c-deps/BUILD.bazel b/c-deps/BUILD.bazel index 51c7ac397032..4a72269ba15a 100644 --- a/c-deps/BUILD.bazel +++ b/c-deps/BUILD.bazel @@ -156,3 +156,32 @@ configure_make( out_static_libs = ["libgssapi_krb5.a"], visibility = ["//visibility:public"], ) + +# This is extremely stupid and unnecessary, but in certain cases to depend on +# the output of a `cmake` target, we need to launder through a filegroup: +# https://github.com/bazelbuild/rules_foreign_cc/issues/619#issuecomment-844473637 +# This is apparently a bug. In the meantime, people can depend on the :*_files +# targets rather than :libgeos where it matters. +filegroup( + name = "libgeos_files", + srcs = [":libgeos"], + visibility = ["//visibility:public"], +) + +filegroup( + name = "libroach_files", + srcs = [":libroach"], + visibility = ["//visibility:public"], +) + +filegroup( + name = "libproj_files", + srcs = [":libproj"], + visibility = ["//visibility:public"], +) + +filegroup( + name = "libedit_files", + srcs = ["@com_github_knz_go_libedit//unix:edit"], + visibility = ["//visibility:public"], +) diff --git a/pkg/build/bazel/bazel.go b/pkg/build/bazel/bazel.go index 920a678a6939..6f1c611de30c 100644 --- a/pkg/build/bazel/bazel.go +++ b/pkg/build/bazel/bazel.go @@ -37,7 +37,7 @@ func Runfile(path string) (string, error) { return inner.Runfile(path) } -// RunfilePath is a convenience wrapper around the rules_go variant. +// RunfilesPath is a convenience wrapper around the rules_go variant. func RunfilesPath() (string, error) { return inner.RunfilesPath() } diff --git a/pkg/testutils/lint/BUILD.bazel b/pkg/testutils/lint/BUILD.bazel index f7429be9dd40..9f5dc3fbc953 100644 --- a/pkg/testutils/lint/BUILD.bazel +++ b/pkg/testutils/lint/BUILD.bazel @@ -1,4 +1,6 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +# gazelle:build_tags lint go_library( name = "lint", @@ -6,3 +8,24 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/testutils/lint", visibility = ["//visibility:public"], ) + +go_test( + name = "lint_test", + srcs = ["lint_test.go"], + data = glob(["testdata/**"]), + embed = [":lint"], + gotags = ["lint"], + visibility = ["//build/bazelutil:__subpackages__"], + deps = [ + "//pkg/build/bazel", + "//pkg/internal/codeowners", + "//pkg/sql/sem/builtins", + "//pkg/testutils/buildutil", + "//pkg/testutils/skip", + "@com_github_cockroachdb_errors//:errors", + "@com_github_ghemawat_stream//:stream", + "@com_github_jordanlewis_gcassert//:gcassert", + "@com_github_stretchr_testify//require", + "@org_golang_x_tools//go/packages", + ], +) diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 80d28708ac6c..43ae0f6d8a0a 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -25,6 +25,7 @@ import ( "strings" "testing" + "github.com/cockroachdb/cockroach/pkg/build/bazel" "github.com/cockroachdb/cockroach/pkg/internal/codeowners" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" _ "github.com/cockroachdb/cockroach/pkg/testutils/buildutil" @@ -38,6 +39,27 @@ import ( const cockroachDB = "github.com/cockroachdb/cockroach" +func init() { + if bazel.BuiltWithBazel() { + // We need to explicitly include all the libraries in LDFLAGS. + runfiles, err := bazel.RunfilesPath() + if err != nil { + panic(err) + } + ldflags := "" + for _, dir := range []string{ + "c-deps/libgeos/lib", + "c-deps/libproj/lib", + "c-deps/libroach/lib", + "external/com_github_knz_go_libedit/unix", + } { + ldflags = ldflags + " -L" + filepath.Join(runfiles, dir) + } + os.Setenv("CGO_LDFLAGS", ldflags) + os.Setenv("LDFLAGS", ldflags) + } +} + func dirCmd( dir string, name string, args ...string, ) (*exec.Cmd, *bytes.Buffer, stream.Filter, error) { @@ -1912,6 +1934,7 @@ func TestLint(t *testing.T) { t.Run("TestGCAssert", func(t *testing.T) { skip.UnderShort(t) + skip.UnderBazelWithIssue(t, 65485, "Doesn't work in Bazel -- not really sure why yet") t.Parallel() var buf strings.Builder @@ -1972,6 +1995,7 @@ func TestLint(t *testing.T) { // See pkg/cmd/roachvet. t.Run("TestRoachVet", func(t *testing.T) { skip.UnderShort(t) + skip.UnderBazelWithIssue(t, 65517, "Some weird linkage issue") // The -printfuncs functionality is interesting and // under-documented. It checks two things: //