Skip to content

Commit

Permalink
Allow bazel test //... to run out-of-the-box (#4150)
Browse files Browse the repository at this point in the history
- **Update to gazelle 0.39.1**
- **Wrap generate_imported_dylib.sh in genrules**

**What type of PR is this?**

Bug fix

**What does this PR do? Why is it needed?**

This PR fixes 3 issues which caused an invocation of
`bazel test //...` in a clean checkout of the repository to fail
depending on potential contributor's development environment.

1. Contributors needed to know to run `generate_imported_dylib.sh`
2. Contributors needed to have $JAVA_HOME set
3. Contributors needed NOT to have `go env GOTOOLCHAIN` set

For 1, this PR wraps the `generate_imported_dylib.sh` in genrules so
that the build will just automatically create the dynamic libraries
appropriate for the platform.

For 2, the PR adds a `--java_runtime_version=remotejdk_21` argument to
the one test in `lcov_coverage_test.go` that needs to be able to build
a Java rule.

For 3, the PR upgrades `gazellle` to version 0.39.1 and updates the
excludes lists in the `popular_repos.py` file to handle the latest
versions of those popular repos. This upgrade addresses the problem
because bazel-contrib/bazel-gazelle#1858 is
fixed in that version of gazelle.

BONUS: Also, there were a few TODOs left in the `WORKSPACE` and
`.bazelrc` files which were addressed by upgrading to the latest
`gazelle` version. So, this PR cleans those up at the same time.

**Which issues(s) does this PR fix?**

Fixes #4149

**Other notes for review**

I have tested this on the following two machines:

macos (M3 arm64)
linux (amd64)

In both cases, I can clone the repository into an fresh directory hop
onto the `build-fixes` branch and run:
```
❯ bazel test -j 8 //...
```

And, it actually has all 452 tests passing within 30 minutes or so.

On both machines, `go env GOTOOLCHAIN` is `go1.23.2` and `$JAVA_HOME`
is not set on either machine.
  • Loading branch information
eljobe authored Oct 21, 2024
1 parent 87e66bb commit 922ace0
Show file tree
Hide file tree
Showing 11 changed files with 109 additions and 55 deletions.
10 changes: 0 additions & 10 deletions .bazelci/presubmit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ tasks:
ubuntu1804_bazel400:
platform: ubuntu1804
bazel: 5.4.0 # test minimum supported version of bazel
shell_commands:
- tests/core/cgo/generate_imported_dylib.sh
build_targets:
- "//..."
- "--"
Expand All @@ -33,8 +31,6 @@ tasks:
- "-//tests/runfiles:runfiles_test"
ubuntu2004:
# enable some unflipped incompatible flags on this platform to ensure we don't regress.
shell_commands:
- tests/core/cgo/generate_imported_dylib.sh
build_flags:
- "--config=incompatible"
test_flags:
Expand All @@ -49,8 +45,6 @@ tasks:
- "//..."
debian11_zig_cc:
platform: debian11
shell_commands:
- tests/core/cgo/generate_imported_dylib.sh
build_flags:
- "--config=incompatible"
- "--extra_toolchains=@zig_sdk//toolchain:linux_amd64_gnu.2.31"
Expand Down Expand Up @@ -92,8 +86,6 @@ tasks:
test_targets:
- "//..."
macos_arm64:
shell_commands:
- tests/core/cgo/generate_imported_dylib.sh
build_flags:
- "--apple_crosstool_top=@local_config_apple_cc//:toolchain"
- "--crosstool_top=@local_config_apple_cc//:toolchain"
Expand All @@ -111,8 +103,6 @@ tasks:
test_targets:
- "//..."
rbe_ubuntu1604:
shell_commands:
- tests/core/cgo/generate_imported_dylib.sh
build_targets:
- "//..."
- "--"
Expand Down
4 changes: 1 addition & 3 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,4 @@ build:incompatible --incompatible_disable_starlark_host_transitions
build:incompatible --nolegacy_external_runfiles
build:incompatible --incompatible_enable_proto_toolchain_resolution
# Also enable all incompatible flags in go_bazel_test by default.
# TODO: Add --incompatible_disallow_empty_glob once
# https://github.com/bazelbuild/bazel-gazelle/pull/1405 has been released.
test:incompatible --test_env=GO_BAZEL_TEST_BAZELFLAGS='--incompatible_load_proto_rules_from_bzl --incompatible_enable_cc_toolchain_resolution --incompatible_config_setting_private_default_visibility --incompatible_enforce_config_setting_visibility --incompatible_disable_starlark_host_transitions --nolegacy_external_runfiles --incompatible_enable_proto_toolchain_resolution'
test:incompatible --test_env=GO_BAZEL_TEST_BAZELFLAGS='--incompatible_disallow_empty_glob --incompatible_load_proto_rules_from_bzl --incompatible_enable_cc_toolchain_resolution --incompatible_config_setting_private_default_visibility --incompatible_enforce_config_setting_visibility --incompatible_disable_starlark_host_transitions --nolegacy_external_runfiles --incompatible_enable_proto_toolchain_resolution'
18 changes: 3 additions & 15 deletions WORKSPACE
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ bazel_skylib_workspace()

http_archive(
name = "bazel_gazelle",
sha256 = "b7387f72efb59f876e4daae42f1d3912d0d45563eac7cb23d1de0b094ab588cf",
sha256 = "b760f7fe75173886007f7c2e616a21241208f3d90e8657dc65d36a771e916b6a",
urls = [
"https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.34.0/bazel-gazelle-v0.34.0.tar.gz",
"https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.34.0/bazel-gazelle-v0.34.0.tar.gz",
"https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.39.1/bazel-gazelle-v0.39.1.tar.gz",
"https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.39.1/bazel-gazelle-v0.39.1.tar.gz",
],
)

Expand Down Expand Up @@ -193,18 +193,6 @@ go_repository(
version = "v0.0.0-20221024183307-1bc688fe9f3e",
)

# TODO(sluongng): Gazelle v0.25.0 switched to static dependency resolution which cause
# build files generation in external dependencies to wrongly resolve these repositories.
# We should investigate in Gazelle why this happen and fix it.
# For now, use manual mapping as a workaround.
#
# gazelle:repository go_repository name=org_golang_x_tools importpath=golang.org/x/tools
# gazelle:repository go_repository name=org_golang_x_text importpath=golang.org/x/text
# gazelle:repository go_repository name=org_golang_x_xerrors importpath=golang.org/x/xerrors
# gazelle:repository go_repository name=org_golang_x_net importpath=golang.org/x/net
# gazelle:repository go_repository name=org_golang_x_sys importpath=golang.org/x/sys
# gazelle:repository go_repository name=org_golang_x_crypto importpath=golang.org/x/crypto

load("@io_bazel_rules_go//tests/legacy/test_chdir:remote.bzl", "test_chdir_remote")

test_chdir_remote()
Expand Down
34 changes: 32 additions & 2 deletions tests/core/cgo/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,45 @@ go_library(
tags = ["manual"],
)


genrule(
name = "generate_imported_dylib_linux",
srcs = ["imported.c"],
tools = ["generate_imported_dylib.sh"],
outs = [
"libimported.so",
"libversioned.so.2",
],
cmd = "$(location generate_imported_dylib.sh) $(location imported.c) $(@D)",
target_compatible_with = ["@platforms//os:linux"],
)

genrule(
name = "generate_imported_dylib_darwin",
srcs = ["imported.c"],
tools = ["generate_imported_dylib.sh"],
outs = [
"libimported.dylib",
"libversioned.2.dylib",
"libversioned.dylib.2",
"libversioned.dylib",
],
cmd = "$(location generate_imported_dylib.sh) $(location imported.c) $(@D)",
target_compatible_with = ["@platforms//os:macos"],
)

cc_import(
name = "darwin_imported_dylib",
shared_library = "libimported.dylib",
tags = ["manual"],
target_compatible_with = ["@platforms//os:macos"],
)

cc_import(
name = "linux_imported_dylib",
shared_library = ":libimported.so",
shared_library = "libimported.so",
tags = ["manual"],
target_compatible_with = ["@platforms//os:linux"],
)

go_test(
Expand Down Expand Up @@ -187,12 +216,14 @@ cc_import(
name = "linux_imported_versioned_dylib",
shared_library = "libversioned.so.2",
tags = ["manual"],
target_compatible_with = ["@platforms//os:linux"],
)

cc_import(
name = "darwin_imported_versioned_dylib",
shared_library = "libversioned.2.dylib",
tags = ["manual"],
target_compatible_with = ["@platforms//os:macos"],
)

go_test(
Expand All @@ -212,7 +243,6 @@ go_library(
target_compatible_with = ["@platforms//os:macos"],
)

# //tests/core/cgo:generate_imported_dylib.sh must be run first
cc_library(
name = "oracle_convention_darwin_dylib",
srcs = [
Expand Down
16 changes: 9 additions & 7 deletions tests/core/cgo/generate_imported_dylib.sh
Original file line number Diff line number Diff line change
@@ -1,23 +1,25 @@
#!/usr/bin/env bash

set -exo pipefail
set -eo pipefail

cd "$(dirname "$0")"
IMPORTED_C_PATH="$1"
OUTPUT_DIR="$2"

case "$(uname -s)" in
Linux*)
cc -shared -o libimported.so imported.c
cc -shared -o libversioned.so.2 imported.c
cc -shared -o $OUTPUT_DIR/libimported.so $IMPORTED_C_PATH
cc -shared -o $OUTPUT_DIR/libversioned.so.2 $IMPORTED_C_PATH
;;
Darwin*)
cc -shared -Wl,-install_name,@rpath/libimported.dylib -o libimported.dylib imported.c
cc -shared -Wl,-install_name,@rpath/libimported.dylib -o $OUTPUT_DIR/libimported.dylib $IMPORTED_C_PATH
# According to "Mac OS X For Unix Geeks", 4th Edition, Chapter 11, versioned dylib for macOS
# should be libversioned.2.dylib.
cc -shared -Wl,-install_name,@rpath/libversioned.2.dylib -o libversioned.2.dylib imported.c
cc -shared -Wl,-install_name,@rpath/libversioned.2.dylib -o $OUTPUT_DIR/libversioned.2.dylib $IMPORTED_C_PATH
# However, Oracle Instant Client was distributed as libclntsh.dylib.12.1 with a unversioed
# symlink (https://www.oracle.com/database/technologies/instant-client/macos-intel-x86-downloads.html).
# Let's cover this non-standard case as well.
cc -shared -Wl,-install_name,@rpath/libversioned.dylib.2 -o libversioned.dylib.2 imported.c
cc -shared -Wl,-install_name,@rpath/libversioned.dylib.2 -o $OUTPUT_DIR/libversioned.dylib.2 $IMPORTED_C_PATH
cd $OUTPUT_DIR
ln -fs libversioned.dylib.2 libversioned.dylib
;;
*)
Expand Down
1 change: 1 addition & 0 deletions tests/core/coverage/lcov_coverage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,7 @@ func TestLcovCoverageWithTool(t *testing.T) {
args := append([]string{
"coverage",
"--combined_report=lcov",
"--java_runtime_version=remotejdk_11",
"//src:lib_with_tool_test",
})

Expand Down
2 changes: 1 addition & 1 deletion tests/integration/gazelle/gazelle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ func TestUpdate(t *testing.T) {
}
got := strings.TrimSpace(string(data))
want := strings.TrimSpace(`
load("@io_bazel_rules_go//go:def.bzl", "go_library")
load("@bazel_gazelle//:def.bzl", "gazelle")
load("@io_bazel_rules_go//go:def.bzl", "go_library")
# gazelle:prefix example.com/hello
gazelle(
Expand Down
28 changes: 24 additions & 4 deletions tests/integration/popular_repos/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,15 @@ test_suite(
"@org_golang_x_crypto//blowfish:blowfish_test",
"@org_golang_x_crypto//bn256:bn256_test",
"@org_golang_x_crypto//cast5:cast5_test",
"@org_golang_x_crypto//chacha20:chacha20_test",
"@org_golang_x_crypto//chacha20poly1305:chacha20poly1305_test",
"@org_golang_x_crypto//cryptobyte:cryptobyte_test",
"@org_golang_x_crypto//curve25519:curve25519_test",
"@org_golang_x_crypto//curve25519/internal/field:field_test",
"@org_golang_x_crypto//ed25519:ed25519_test",
"@org_golang_x_crypto//hkdf:hkdf_test",
"@org_golang_x_crypto//internal/chacha20:chacha20_test",
"@org_golang_x_crypto//internal/subtle:subtle_test",
"@org_golang_x_crypto//internal/alias:alias_test",
"@org_golang_x_crypto//internal/poly1305:poly1305_test",
"@org_golang_x_crypto//md4:md4_test",
"@org_golang_x_crypto//nacl/auth:auth_test",
"@org_golang_x_crypto//nacl/box:box_test",
Expand All @@ -52,16 +54,16 @@ test_suite(
"@org_golang_x_crypto//pbkdf2:pbkdf2_test",
"@org_golang_x_crypto//pkcs12:pkcs12_test",
"@org_golang_x_crypto//pkcs12/internal/rc2:rc2_test",
"@org_golang_x_crypto//poly1305:poly1305_test",
"@org_golang_x_crypto//ripemd160:ripemd160_test",
"@org_golang_x_crypto//salsa20:salsa20_test",
"@org_golang_x_crypto//salsa20/salsa:salsa_test",
"@org_golang_x_crypto//scrypt:scrypt_test",
"@org_golang_x_crypto//sha3:sha3_test",
"@org_golang_x_crypto//ssh/internal/bcrypt_pbkdf:bcrypt_pbkdf_test",
"@org_golang_x_crypto//ssh/knownhosts:knownhosts_test",
"@org_golang_x_crypto//ssh/terminal:terminal_test",
"@org_golang_x_crypto//tea:tea_test",
"@org_golang_x_crypto//twofish:twofish_test",
"@org_golang_x_crypto//x509roots/nss:nss_test",
"@org_golang_x_crypto//xtea:xtea_test",
"@org_golang_x_crypto//xts:xts_test",
],
Expand All @@ -80,6 +82,7 @@ test_suite(
"@org_golang_x_net//http2/h2c:h2c_test",
"@org_golang_x_net//http2/hpack:hpack_test",
"@org_golang_x_net//idna:idna_test",
"@org_golang_x_net//internal/quic/cmd/interop:interop_test",
"@org_golang_x_net//internal/socks:socks_test",
"@org_golang_x_net//internal/sockstest:sockstest_test",
"@org_golang_x_net//internal/timeseries:timeseries_test",
Expand All @@ -88,6 +91,8 @@ test_suite(
"@org_golang_x_net//netutil:netutil_test",
"@org_golang_x_net//proxy:proxy_test",
"@org_golang_x_net//publicsuffix:publicsuffix_test",
"@org_golang_x_net//quic:quic_test",
"@org_golang_x_net//quic/qlog:qlog_test",
"@org_golang_x_net//route:route_test",
"@org_golang_x_net//trace:trace_test",
"@org_golang_x_net//webdav:webdav_test",
Expand Down Expand Up @@ -161,6 +166,7 @@ test_suite(
tests = [
"@org_golang_x_tools//benchmark/parse:parse_test",
"@org_golang_x_tools//cmd/benchcmp:benchcmp_test",
"@org_golang_x_tools//cmd/bisect:bisect_test",
"@org_golang_x_tools//cmd/digraph:digraph_test",
"@org_golang_x_tools//cmd/go-contrib-init:go-contrib-init_test",
"@org_golang_x_tools//cmd/splitdwarf/internal/macho:macho_test",
Expand All @@ -176,13 +182,18 @@ test_suite(
"@org_golang_x_tools//godoc/vfs:vfs_test",
"@org_golang_x_tools//godoc/vfs/gatefs:gatefs_test",
"@org_golang_x_tools//godoc/vfs/mapfs:mapfs_test",
"@org_golang_x_tools//internal/aliases:aliases_test",
"@org_golang_x_tools//internal/analysisinternal:analysisinternal_test",
"@org_golang_x_tools//internal/bisect:bisect_test",
"@org_golang_x_tools//internal/diff:diff_test",
"@org_golang_x_tools//internal/diff/lcs:lcs_test",
"@org_golang_x_tools//internal/diff/myers:myers_test",
"@org_golang_x_tools//internal/edit:edit_test",
"@org_golang_x_tools//internal/event:event_test",
"@org_golang_x_tools//internal/event/export:export_test",
"@org_golang_x_tools//internal/event/export/ocagent:ocagent_test",
"@org_golang_x_tools//internal/event/export/ocagent/wire:wire_test",
"@org_golang_x_tools//internal/event/keys:keys_test",
"@org_golang_x_tools//internal/event/label:label_test",
"@org_golang_x_tools//internal/fuzzy:fuzzy_test",
"@org_golang_x_tools//internal/gopathwalk:gopathwalk_test",
Expand All @@ -193,6 +204,7 @@ test_suite(
"@org_golang_x_tools//internal/proxydir:proxydir_test",
"@org_golang_x_tools//internal/robustio:robustio_test",
"@org_golang_x_tools//internal/stack:stack_test",
"@org_golang_x_tools//internal/tokeninternal:tokeninternal_test",
"@org_golang_x_tools//internal/typesinternal:typesinternal_test",
"@org_golang_x_tools//playground/socket:socket_test",
"@org_golang_x_tools//refactor/satisfy:satisfy_test",
Expand Down Expand Up @@ -233,6 +245,7 @@ test_suite(
build_test(
name = "build_only",
targets = [
"@org_golang_x_crypto//nacl/secretbox:secretbox",
"@org_golang_x_crypto//ssh/agent:agent",
"@org_golang_x_crypto//ssh/test:test",
"@org_golang_x_crypto//ssh:ssh",
Expand All @@ -256,6 +269,7 @@ build_test(
"@org_golang_x_tools//cmd/callgraph:callgraph",
"@org_golang_x_tools//cmd/file2fuzz:file2fuzz",
"@org_golang_x_tools//cmd/fiximports:fiximports",
"@org_golang_x_tools//cmd/gonew:gonew",
"@org_golang_x_tools//cmd/gorename:gorename",
"@org_golang_x_tools//cmd/signature-fuzzer/fuzz-driver:fuzz-driver",
"@org_golang_x_tools//cmd/signature-fuzzer/fuzz-runner:fuzz-runner",
Expand All @@ -265,6 +279,7 @@ build_test(
"@org_golang_x_tools//go/analysis/analysistest:analysistest",
"@org_golang_x_tools//go/analysis/multichecker:multichecker",
"@org_golang_x_tools//go/analysis/passes/asmdecl:asmdecl",
"@org_golang_x_tools//go/analysis/passes/appends:appends",
"@org_golang_x_tools//go/analysis/passes/assign:assign",
"@org_golang_x_tools//go/analysis/passes/atomic:atomic",
"@org_golang_x_tools//go/analysis/passes/atomicalign:atomicalign",
Expand All @@ -276,11 +291,13 @@ build_test(
"@org_golang_x_tools//go/analysis/passes/copylock:copylock",
"@org_golang_x_tools//go/analysis/passes/ctrlflow:ctrlflow",
"@org_golang_x_tools//go/analysis/passes/deepequalerrors:deepequalerrors",
"@org_golang_x_tools//go/analysis/passes/defers:defers",
"@org_golang_x_tools//go/analysis/passes/directive:directive",
"@org_golang_x_tools//go/analysis/passes/errorsas:errorsas",
"@org_golang_x_tools//go/analysis/passes/fieldalignment:fieldalignment",
"@org_golang_x_tools//go/analysis/passes/findcall:findcall",
"@org_golang_x_tools//go/analysis/passes/framepointer:framepointer",
"@org_golang_x_tools//go/analysis/passes/httpmux:httpmux",
"@org_golang_x_tools//go/analysis/passes/httpresponse:httpresponse",
"@org_golang_x_tools//go/analysis/passes/ifaceassert:ifaceassert",
"@org_golang_x_tools//go/analysis/passes/loopclosure:loopclosure",
Expand All @@ -293,8 +310,10 @@ build_test(
"@org_golang_x_tools//go/analysis/passes/shadow:shadow",
"@org_golang_x_tools//go/analysis/passes/shift:shift",
"@org_golang_x_tools//go/analysis/passes/sigchanyzer:sigchanyzer",
"@org_golang_x_tools//go/analysis/passes/slog:slog",
"@org_golang_x_tools//go/analysis/passes/sortslice:sortslice",
"@org_golang_x_tools//go/analysis/passes/stdmethods:stdmethods",
"@org_golang_x_tools//go/analysis/passes/stdversion:stdversion",
"@org_golang_x_tools//go/analysis/passes/stringintconv:stringintconv",
"@org_golang_x_tools//go/analysis/passes/structtag:structtag",
"@org_golang_x_tools//go/analysis/passes/testinggoroutine:testinggoroutine",
Expand All @@ -312,6 +331,7 @@ build_test(
"@org_golang_x_tools//go/callgraph/cha:cha",
"@org_golang_x_tools//go/callgraph/rta:rta",
"@org_golang_x_tools//go/callgraph/vta:vta",
"@org_golang_x_tools//go/cfg:cfg",
"@org_golang_x_tools//go/expect:expect",
"@org_golang_x_tools//go/gccgoexportdata:gccgoexportdata",
"@org_golang_x_tools//go/gcexportdata:gcexportdata",
Expand Down
Loading

0 comments on commit 922ace0

Please sign in to comment.