From 62181b151c5298dbde8db51e585e2a77bca03d58 Mon Sep 17 00:00:00 2001 From: Tom van der Woerdt Date: Fri, 23 Aug 2024 12:49:21 +0200 Subject: [PATCH] Update go_repository defaults to only use Go code 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`. --- internal/bzlmod/default_gazelle_overrides.bzl | 71 +------------------ internal/bzlmod/go_deps.bzl | 39 ++++------ internal/go_repository.bzl | 18 ++--- language/go/config.go | 2 +- repository.md | 2 +- tests/bcr/go_mod/MODULE.bazel | 22 +----- tests/bcr/go_work/MODULE.bazel | 6 +- tools/override-generator/main.go | 2 +- tools/override-generator/main_test.go | 14 ++-- 9 files changed, 36 insertions(+), 140 deletions(-) diff --git a/internal/bzlmod/default_gazelle_overrides.bzl b/internal/bzlmod/default_gazelle_overrides.bzl index 2283f5d33..a06744229 100644 --- a/internal/bzlmod/default_gazelle_overrides.bzl +++ b/internal/bzlmod/default_gazelle_overrides.bzl @@ -15,67 +15,10 @@ 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", } 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/grpc-ecosystem/grpc-gateway": [ "gazelle:resolve go github.com/grpc-ecosystem/grpc-gateway/internal //internal:go_default_library", "gazelle:go_naming_convention import_alias", @@ -86,18 +29,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", diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index dcd0db37c..85c9c59df 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -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( @@ -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") diff --git a/internal/go_repository.bzl b/internal/go_repository.bzl index 48a9d1490..9b86a4b48 100644 --- a/internal/go_repository.bzl +++ b/internal/go_repository.bzl @@ -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)) @@ -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] @@ -553,18 +551,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( diff --git a/language/go/config.go b/language/go/config.go index e23ed5437..2cacae3a9 100644 --- a/language/go/config.go +++ b/language/go/config.go @@ -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"} ) diff --git a/repository.md b/repository.md index 36978aed3..cf592eeb8 100644 --- a/repository.md +++ b/repository.md @@ -179,7 +179,7 @@ go_repository( | 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 | `[]` | | build_external | One of `"external"`, `"static"` or `"vendored"`.

This sets Gazelle's `-external` command line flag. In `"static"` mode, Gazelle will not call out to the network to resolve imports.

**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"` | | build_extra_args | A list of additional command line arguments to pass to Gazelle when generating build files. | List of strings | optional | `[]` | -| build_file_generation | One of `"auto"`, `"on"`, `"off"`, `"clean"`.

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"` | +| build_file_generation | One of `"clean"` (default), `"update"`, `"off"`.

Whether Gazelle should generate build files in the repository. | String | optional | `"clean"` | | 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"` | | build_file_proto_mode | One of `"default"`, `"legacy"`, `"disable"`, `"disable_global"` or `"package"`.

This sets Gazelle's `-proto` command line flag. See [Directives] for more information on each mode. | String | optional | `""` | | 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`.

See the `gazelle:go_naming_convention` directive in [Directives] for more information. | String | optional | `"import_alias"` | diff --git a/tests/bcr/go_mod/MODULE.bazel b/tests/bcr/go_mod/MODULE.bazel index 7bceaa326..b19e7e3bf 100644 --- a/tests/bcr/go_mod/MODULE.bazel +++ b/tests/bcr/go_mod/MODULE.bazel @@ -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( @@ -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", ], diff --git a/tests/bcr/go_work/MODULE.bazel b/tests/bcr/go_work/MODULE.bazel index 0e4220acb..e0d812115 100644 --- a/tests/bcr/go_work/MODULE.bazel +++ b/tests/bcr/go_work/MODULE.bazel @@ -34,7 +34,7 @@ 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", ], @@ -42,9 +42,6 @@ go_deps.gazelle_default_attributes( # 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", @@ -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", ], diff --git a/tools/override-generator/main.go b/tools/override-generator/main.go index 976ec5950..46b59f19d 100644 --- a/tools/override-generator/main.go +++ b/tools/override-generator/main.go @@ -91,7 +91,7 @@ func parseArgs(stderr io.Writer, osArgs []string) (*mainArgs, error) { flag.StringVar(&a.defName, "def_name", "", "name of the macro definition") flag.StringVar(&a.outputFile, "output", "", "path to the output file") flag.StringVar(&a.gazelleRepoName, "gazelle_repo_name", "@bazel_gazelle", "name of the gazelle repo to load go_deps, (default: @bazel_gazelle)") - flag.StringVar(&a.defaultBuildFileGeneration, "default_build_file_generation", "auto", "the default value for build_file_generation attribute") + flag.StringVar(&a.defaultBuildFileGeneration, "default_build_file_generation", "clean", "the default value for build_file_generation attribute") flag.StringVar(&a.defaultBuildFileProtoMode, "default_build_file_proto_mode", "default", "the default value for build_file_proto_mode attribute") flag.Parse(osArgs) diff --git a/tools/override-generator/main_test.go b/tools/override-generator/main_test.go index b0f3fd19a..04d2e9895 100644 --- a/tools/override-generator/main_test.go +++ b/tools/override-generator/main_test.go @@ -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=", @@ -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=", @@ -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", )`, @@ -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=", @@ -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", @@ -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"], @@ -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", )`,