Skip to content

Commit

Permalink
Update go_repository defaults to only use Go code
Browse files Browse the repository at this point in the history 
This is an attempt at making `go_repository` and its bzlmod equivalent
pull Go modules as similar as possible to what `go get` would do: no
proto generation, and no BUILD files.

In my experience, the bulk of Go modules that ship with BUILD files
don't actually compile, so we need Gazelle to fix them for us. In
general Gazelle does a great job at these, but sometimes they will
contain `genrule`s and other code meant for updating the sources, and
in some cases Gazelle just can't fix those.

I previously added `build_file_generation=clean` for this, and it works
great under `gazelle_default_attributes` in my local tests. So, this
commit makes that the default. Gazelle's build file _updating_ is great
for local development, but it almost never makes sense for an external
Go module pulled in via go_repository/go_deps.

The same thing applies to protobuf. Some module authors ship .proto
files in their codebase, which is great! However, since Gazelle doesn't
really implement dependency resolution for proto files (which is fair,
because that's _hard_), we end up with a lot of `gazelle:proto disable`
everywhere, because we import modules without using the proto files.

So, we can insert `gazelle:proto disable` by default for external
repositories. When the proto targets are actually needed, they can be
reinstated by passing either an empty directives list, or (more likely)
by providing the dependency resolution rules.

The end result is that go_repository and go_deps now basically don't
need special configuration or overrides anymore, as evidenced by my
cleanup in `default_gazelle_overrides.bzl`.
  • Loading branch information
@TvdW
TvdW committed Aug 28, 2024
1 parent 1a98d35 commit b19e843
Show file tree
Hide file tree
Showing 10 changed files with 39 additions and 140 deletions.
1 change: 1 addition & 0 deletions deps.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ def gazelle_dependencies(
_maybe(
go_repository,
name = "com_github_bazelbuild_buildtools",
build_file_generation = "update",
importpath = "github.com/bazelbuild/buildtools",
sum = "h1:VNqmvOfFzn2Hrtoni8vqgXlIQ4C2Zt22fxeZ9gOOkp0=",
version = "v0.0.0-20240313121412-66c605173954",
Expand Down
73 changes: 3 additions & 70 deletions internal/bzlmod/default_gazelle_overrides.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,67 +15,12 @@
visibility("private")

DEFAULT_BUILD_FILE_GENERATION_BY_PATH = {
"github.com/envoyproxy/protoc-gen-validate": "on",
"github.com/google/safetext": "on",
"github.com/grpc-ecosystem/grpc-gateway/v2": "on",
"google.golang.org/grpc": "on",
"github.com/grpc-ecosystem/grpc-gateway/v2": "update",
"github.com/bazelbuild/buildtools": "update",
}

DEFAULT_DIRECTIVES_BY_PATH = {
"github.com/argoproj/argo-workflows/v3": [
"gazelle:proto disable",
],
"github.com/argoproj/argo-events": [
"gazelle:proto disable",
"gazelle:go_naming_convention import_alias",
],
"github.com/census-instrumentation/opencensus-proto": [
"gazelle:proto disable",
],
"github.com/envoyproxy/protoc-gen-validate": [
"gazelle:build_file_name BUILD.bazel",
],
"github.com/cockroachdb/errors": [
"gazelle:proto disable",
],
"github.com/colinmarc/hdfs/v2": [
"gazelle:go_naming_convention import_alias",
"gazelle:proto disable",
],
"github.com/containerd/containerd": [
"gazelle:proto disable",
],
"github.com/containerd/containerd/api": [
"gazelle:proto disable",
],
"github.com/containerd/ttrpc": [
"gazelle:proto disable",
],
"github.com/gogo/googleapis": [
"gazelle:proto disable",
],
"github.com/gogo/protobuf": [
"gazelle:proto disable",
],
"github.com/google/cel-go": [
"gazelle:go_naming_convention go_default_library",
],
"github.com/google/gnostic": [
"gazelle:proto disable",
],
"github.com/google/gnostic-models": [
"gazelle:proto disable",
],
"github.com/google/safetext": [
"gazelle:build_file_name BUILD.bazel",
"gazelle:build_file_proto_mode disable_global",
],
"github.com/googleapis/gax-go/v2": [
"gazelle:proto disable",
],
"github.com/googleapis/gnostic": [
"gazelle:proto disable",
],
"github.com/bazelbuild/buildtools": [],
"github.com/grpc-ecosystem/grpc-gateway": [
"gazelle:resolve go github.com/grpc-ecosystem/grpc-gateway/internal //internal:go_default_library",
"gazelle:go_naming_convention import_alias",
Expand All @@ -86,18 +31,6 @@ DEFAULT_DIRECTIVES_BY_PATH = {
# in go files are generated by gogo proto. Resolving to the gogo proto target preserves the behavior of Go modules.
"gazelle:resolve go github.com/mwitkow/go-proto-validators @com_github_mwitkow_go_proto_validators//:validators_gogo",
],
"google.golang.org/grpc": [
"gazelle:proto disable",
],
"google.golang.org/protobuf": [
"gazelle:proto disable",
],
"k8s.io/api": [
"gazelle:proto disable",
],
"k8s.io/apiextensions-apiserver": [
"gazelle:proto disable",
],
"k8s.io/apimachinery": [
"gazelle:go_generate_proto false",
"gazelle:proto_import_prefix k8s.io/apimachinery",
Expand Down
39 changes: 15 additions & 24 deletions internal/bzlmod/go_deps.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,27 +47,23 @@ https://github.com/bazelbuild/bazel-gazelle/tree/master/internal/bzlmod/default_

_GAZELLE_ATTRS = {
"build_file_generation": attr.string(
default = "on",
doc = """One of `"auto"`, `"on"` (default), `"off"`, `"clean"`.
default = "clean",
doc = """One of `"clean"` (default), `"update"`, `"off"`
Whether Gazelle should generate build files for the Go module.
Although "auto" is the default globally for build_file_generation,
if a `"gazelle_override"` or `"gazelle_default_attributes"` tag is present
for a Go module, the `"build_file_generation"` attribute will default to "on"
since these tags indicate the presence of `"directives"` or `"build_extra_args"`.
In `"clean"` mode (default), Gazelle will ignore bazel files from the repository,
and create new files.
In `"auto"` mode, Gazelle will run if there is no build file in the Go
module's root directory.
In `"clean"` mode, Gazelle will first remove any existing build files.
In `"update"` mode, Gazelle will attempt to use and update the existing files
in the repository.
In `"off"` mode, Gazelle will leave the BUILD files from the repository intact.
""",
values = [
"auto",
"off",
"on",
"clean",
"update",
"off",
],
),
"build_extra_args": attr.string_list(
Expand Down Expand Up @@ -123,35 +119,30 @@ def _get_override_or_default(specific_overrides, gazelle_default_attributes, def
# 1st: Check for user-provided specific overrides. If a specific override is found,
# all of its attributes will be applied (even if left to the tag's default). This is to allow
# users to override the gazelle_default_attributes tag back to the tag's default.
#
# This will also cause "build_file_generation" to default to "on" if a specific override is found.
specific_override = specific_overrides.get(path)
if specific_override and hasattr(specific_override, attribute_name):
if specific_override != None and hasattr(specific_override, attribute_name):
return getattr(specific_override, attribute_name)

# 2nd. Check for default attributes provided by the user. This must be done before checking for
# gazelle's defaults path overrides to prevent Gazelle from overriding a user-specified flag.
#
# This will also cause "build_file_generation" to default to "on" if default attributes are found.
global_override_value = getattr(gazelle_default_attributes, attribute_name, None)
if global_override_value:
if global_override_value != None:
return global_override_value

# 3rd: Check for default overrides for specific path.
default_path_override = default_path_overrides.get(path)
if default_path_override:
if default_path_override != None:
return default_path_override

# 4th. Return the default value if no override was found.
# This will cause "build_file_generation" to default to "auto".
return default_value

def _get_directives(path, gazelle_overrides, gazelle_default_attributes):
return _get_override_or_default(gazelle_overrides, gazelle_default_attributes, DEFAULT_DIRECTIVES_BY_PATH, path, [], "directives")
return _get_override_or_default(gazelle_overrides, gazelle_default_attributes, DEFAULT_DIRECTIVES_BY_PATH, path, ["gazelle:proto disable"], "directives")

def _get_build_file_generation(path, gazelle_overrides, gazelle_default_attributes):
# The default value for build_file_generation is "auto" if no override is found, but will default to "on" if an override is found.
return _get_override_or_default(gazelle_overrides, gazelle_default_attributes, DEFAULT_BUILD_FILE_GENERATION_BY_PATH, path, "auto", "build_file_generation")
# The default value for build_file_generation is "clean" if no override is found
return _get_override_or_default(gazelle_overrides, gazelle_default_attributes, DEFAULT_BUILD_FILE_GENERATION_BY_PATH, path, "clean", "build_file_generation")

def _get_build_extra_args(path, gazelle_overrides, gazelle_default_attributes):
return _get_override_or_default(gazelle_overrides, gazelle_default_attributes, DEFAULT_BUILD_EXTRA_ARGS_BY_PATH, path, [], "build_extra_args")
Expand Down
18 changes: 6 additions & 12 deletions internal/go_repository.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def _go_repository_impl(ctx):
# https://docs.bazel.build/versions/main/skylark/repository_rules.html#when-is-the-implementation-function-executed
go_env_cache = str(ctx.path(Label("@bazel_gazelle_go_repository_cache//:go.env")))
fetch_repo = str(ctx.path(Label("@bazel_gazelle_go_repository_tools//:bin/fetch_repo{}".format(executable_extension(ctx)))))
generate = ctx.attr.build_file_generation in ["on", "clean"]
generate = ctx.attr.build_file_generation in ["clean", "update"]
_gazelle = "@bazel_gazelle_go_repository_tools//:bin/gazelle{}".format(executable_extension(ctx))
if generate:
gazelle_path = ctx.path(Label(_gazelle))
Expand Down Expand Up @@ -295,8 +295,6 @@ def _go_repository_impl(ctx):
existing_build_file = name
break

generate = generate or (not existing_build_file and ctx.attr.build_file_generation == "auto")

if generate:
# Build file generation is needed. Populate Gazelle directive at root build file
build_file_name = existing_build_file or build_file_names[0]
Expand Down Expand Up @@ -543,18 +541,14 @@ go_repository = repository_rule(
file systems.""",
),
"build_file_generation": attr.string(
default = "auto",
doc = """One of `"auto"`, `"on"`, `"off"`, `"clean"`.
default = "clean",
doc = """One of `"clean"` (default), `"update"`, `"off"`.
Whether Gazelle should generate build files in the repository. In `"auto"`
mode, Gazelle will run if there is no build file in the repository root
directory. In `"clean"` mode, Gazelle will first remove any existing build
files.""",
Whether Gazelle should generate build files in the repository.""",
values = [
"on",
"auto",
"off",
"clean",
"update",
"off",
],
),
"build_naming_convention": attr.string(
Expand Down
2 changes: 1 addition & 1 deletion language/go/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ type moduleRepo struct {

var (
validBuildExternalAttr = []string{"external", "vendored"}
validBuildFileGenerationAttr = []string{"auto", "on", "off"}
validBuildFileGenerationAttr = []string{"clean", "update", "off"}
validBuildFileProtoModeAttr = []string{"default", "legacy", "disable", "disable_global", "package"}
)

Expand Down
2 changes: 1 addition & 1 deletion repository.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ go_repository(
| <a id="go_repository-build_directives"></a>build_directives | A list of directives to be written to the root level build file before Calling Gazelle to generate build files. Each string in the list will be prefixed with `#` automatically. A common use case is to pass a list of Gazelle directives. | List of strings | optional | `[]` |
| <a id="go_repository-build_external"></a>build_external | One of `"external"`, `"static"` or `"vendored"`.<br><br>This sets Gazelle's `-external` command line flag. In `"static"` mode, Gazelle will not call out to the network to resolve imports.<br><br>**NOTE:** This cannot be used to ignore the `vendor` directory in a repository. The `-external` flag only controls how Gazelle resolves imports which are not present in the repository. Use `build_extra_args = ["-exclude=vendor"]` instead. | String | optional | `"static"` |
| <a id="go_repository-build_extra_args"></a>build_extra_args | A list of additional command line arguments to pass to Gazelle when generating build files. | List of strings | optional | `[]` |
| <a id="go_repository-build_file_generation"></a>build_file_generation | One of `"auto"`, `"on"`, `"off"`, `"clean"`.<br><br>Whether Gazelle should generate build files in the repository. In `"auto"` mode, Gazelle will run if there is no build file in the repository root directory. In `"clean"` mode, Gazelle will first remove any existing build files. | String | optional | `"auto"` |
| <a id="go_repository-build_file_generation"></a>build_file_generation | One of `"clean"` (default), `"update"`, `"off"`.<br><br>Whether Gazelle should generate build files in the repository. | String | optional | `"clean"` |
| <a id="go_repository-build_file_name"></a>build_file_name | Comma-separated list of names Gazelle will consider to be build files. If a repository contains files named `build` that aren't related to Bazel, it may help to set this to `"BUILD.bazel"`, especially on case-insensitive file systems. | String | optional | `"BUILD.bazel,BUILD"` |
| <a id="go_repository-build_file_proto_mode"></a>build_file_proto_mode | One of `"default"`, `"legacy"`, `"disable"`, `"disable_global"` or `"package"`.<br><br>This sets Gazelle's `-proto` command line flag. See [Directives] for more information on each mode. | String | optional | `""` |
| <a id="go_repository-build_naming_convention"></a>build_naming_convention | Sets the library naming convention to use when resolving dependencies against this external repository. If unset, the convention from the external workspace is used. Legal values are `go_default_library`, `import`, and `import_alias`.<br><br>See the `gazelle:go_naming_convention` directive in [Directives] for more information. | String | optional | `"import_alias"` |
Expand Down
22 changes: 2 additions & 20 deletions tests/bcr/go_mod/MODULE.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,31 +38,12 @@ go_deps.config(
# Validate a go.mod replace directive works.
go_deps.from_file(go_mod = "//:go.mod")
go_deps.gazelle_default_attributes(
build_file_generation = "on",
build_file_generation = "clean",
directives = [
"gazelle:proto disable",
],
)

# By defining `gazelle_default_attributes`, we also must individually
# specify certain overrides from internal/bzlmod/default_gazelle_overrides.bzl
#
# "build_file_generation" defaults to "on" because we provided a "gazelle_override"
# (which contains either directives or build extra args).
go_deps.gazelle_override(
directives = [
"gazelle:build_file_name BUILD.bazel",
"gazelle:build_file_proto_mode disable_global",
],
path = "github.com/google/safetext",
)
go_deps.gazelle_override(
directives = [
"gazelle:build_file_name BUILD.bazel",
],
path = "github.com/envoyproxy/protoc-gen-validate",
)

# Verify that the gazelle:go_naming_convention directive in an override is
# respected.
go_deps.module(
Expand All @@ -88,6 +69,7 @@ go_deps.module_override(

# Test an archive override from a known archive.
go_deps.gazelle_override(
build_file_generation = "off",
directives = [
"gazelle:go_naming_convention go_default_library",
],
Expand Down
6 changes: 2 additions & 4 deletions tests/bcr/go_work/MODULE.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,14 @@ go_deps.config(
# Validate a full go workspace
go_deps.from_file(go_work = "//:go.work")
go_deps.gazelle_default_attributes(
build_file_generation = "on",
build_file_generation = "update",
directives = [
"gazelle:proto disable",
],
)

# By defining `gazelle_default_attributes`, we also must individually
# specify certain overrides from internal/bzlmod/default_gazelle_overrides.bzl
#
# "build_file_generation" defaults to "on" because we provided a "gazelle_override"
# (which contains either directives or build extra args).
go_deps.gazelle_override(
directives = [
"gazelle:build_file_name BUILD.bazel",
Expand Down Expand Up @@ -84,6 +81,7 @@ go_deps.module_override(

# Test an archive override from a known archive.
go_deps.gazelle_override(
build_file_generation = "off",
directives = [
"gazelle:go_naming_convention go_default_library",
],
Expand Down
2 changes: 1 addition & 1 deletion tools/override-generator/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ const (
)

var _defaultValues = map[string]string{
_buildFileGenerationAttr: "auto",
_buildFileGenerationAttr: "clean",
_buildFileProtoModeAttr: "default",
}

Expand Down
14 changes: 7 additions & 7 deletions tools/override-generator/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestBzlmodOverride(t *testing.T) {
go_repository(
name = "com_github_apache_thrift",
build_file_generation = "auto",
build_file_generation = "update",
build_file_proto_mode = "default",
importpath = "github.com/apache/thrift",
sum = "h1:cMd2aj52n+8VoAtvSvLn4kDC3aZ6IAkBuqWQ2IDu7wo=",
Expand All @@ -36,7 +36,7 @@ func TestBzlmodOverride(t *testing.T) {
go_repository(
name = "com_github_apache_thrift",
build_extra_args = ["-go_naming_convention_external=go_default_library"],
build_file_generation = "on",
build_file_generation = "update",
build_file_proto_mode = "disable",
importpath = "github.com/apache/thrift",
sum = "h1:cMd2aj52n+8VoAtvSvLn4kDC3aZ6IAkBuqWQ2IDu7wo=",
Expand All @@ -46,7 +46,7 @@ func TestBzlmodOverride(t *testing.T) {
go_deps.gazelle_override(
build_extra_args = ["-go_naming_convention_external=go_default_library"],
build_file_generation = "on",
build_file_generation = "update",
directives = ["gazelle:proto disable"],
path = "github.com/apache/thrift",
)`,
Expand Down Expand Up @@ -95,7 +95,7 @@ func TestBzlmodOverride(t *testing.T) {
"gazelle:resolve go github.com/ClickHouse/clickhouse-go/v2/external @com_github_clickhouse_clickhouse_go_v2//external",
],
build_extra_args = ["-go_naming_convention_external=go_default_library"],
build_file_generation = "on",
build_file_generation = "update",
build_file_proto_mode = "disable",
importpath = "github.com/ClickHouse/clickhouse-go/v2",
sum = "h1:Nbl/NZwoM6LGJm7smNBgvtdr/rxjlIssSW3eG/Nmb9E=",
Expand All @@ -105,7 +105,7 @@ func TestBzlmodOverride(t *testing.T) {
go_deps.gazelle_override(
build_extra_args = ["-go_naming_convention_external=go_default_library"],
build_file_generation = "on",
build_file_generation = "update",
directives = [
"gazelle:resolve go github.com/ClickHouse/clickhouse-go/v2/external @com_github_clickhouse_clickhouse_go_v2//external",
"gazelle:proto disable",
Expand All @@ -121,7 +121,7 @@ func TestBzlmodOverride(t *testing.T) {
"-go_prefix=golang.org/x/tools",
"-exclude=**/testdata",
],
build_file_generation = "on",
build_file_generation = "update",
build_file_proto_mode = "disable",
importpath = "golang.org/x/tools/cmd/goimports",
patch_args = ["-p1"],
Expand Down Expand Up @@ -154,7 +154,7 @@ func TestBzlmodOverride(t *testing.T) {
"-go_prefix=golang.org/x/tools",
"-exclude=**/testdata",
],
build_file_generation = "on",
build_file_generation = "update",
directives = ["gazelle:proto disable"],
path = "golang.org/x/tools/cmd/goimports",
)`,
Expand Down

0 comments on commit b19e843

Please sign in to comment.