Skip to content

Commit

Permalink
bazel: provide way to run lint in bazel
Browse files Browse the repository at this point in the history
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
  • Loading branch information
rickystewart committed May 27, 2021
1 parent 9cf809c commit 155a8fa
Show file tree
Hide file tree
Showing 9 changed files with 234 additions and 2 deletions.
4 changes: 4 additions & 0 deletions DEPS.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 6 additions & 0 deletions build/bazelutil/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
load(":lint.bzl", "lint_binary")

lint_binary(
name = "lint",
test = "//pkg/testutils/lint:lint_test",
)
65 changes: 65 additions & 0 deletions build/bazelutil/lint.bzl
Original file line number Diff line number Diff line change
@@ -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,
)
52 changes: 52 additions & 0 deletions build/bazelutil/lint.sh.in
Original file line number Diff line number Diff line change
@@ -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 $@
29 changes: 29 additions & 0 deletions build/patches/com_github_knz_go_libedit.patch
Original file line number Diff line number Diff line change
@@ -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({
29 changes: 29 additions & 0 deletions c-deps/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
)
2 changes: 1 addition & 1 deletion pkg/build/bazel/bazel.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
25 changes: 24 additions & 1 deletion pkg/testutils/lint/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,8 +1,31 @@
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",
srcs = ["lint.go"],
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",
],
)
24 changes: 24 additions & 0 deletions pkg/testutils/lint/lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
//
Expand Down

0 comments on commit 155a8fa

Please sign in to comment.