Skip to content

Commit

Permalink
go_repository: add 'clean' build_file_generation (#1802)
Browse files Browse the repository at this point in the history
* go_repository: add 'clean' build_file_generation

Before bzlmod, repositories were global, and BUILD files inside upstream
repositories could load from the repositories defined at the root-level
WORKSPACE file.

With the introduction of visibility rules, these result in errors that
cannot trivially be worked around. One pattern that has emerged is
ignoring existing build files by specifying `gazelle:build_file_name
BUILD.bazel`, however this does not work for anything already using
BUILD.bazel filenames.

This does affect real-world users: an example is
github.com/google/go-jsonnet which has a BUILD.bazel file to `genrule`
its AST. The module is pure-go, but the build step contains cpp code,
and when imported under bzlmod-gazelle will not work because the cpp
code is invoked. As a workaround, there is now a 'jsonnet' BCR module.

This patch introduces a `build_file_generation = clean` option, which
removes existing build files before generating new ones. It allows
loading the jsonnet code once again, and also makes it possible to
load some of the complex protobuf repositories.

* go_repository: always call fetch_repo, even for urls

After my changes to remove build files as part of fetch_repo, I realized
that when urls=[] is specified the code path doesn't use fetch_repo at
all. In this case, fetch_repo_args would remain None, and raise an
error. Not ideal!

This left me with two general options: either I move the code into a
separate command invocation, or I have all code go through fetch_repo.

The latter seems to be the pragmatic solution: gazelle by itself doesn't
generate code with urls=[] (in either bzlmod or vanilla update-repos),
so it's the option with lower performance impact (also considering we're
stacking it on top of a network download).
  • Loading branch information
TvdW authored Jun 1, 2024
1 parent 8c16db5 commit 7d10bf7
Show file tree
Hide file tree
Showing 13 changed files with 115 additions and 52 deletions.
2 changes: 1 addition & 1 deletion README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ The following flags are accepted:
+----------------------------------------------------------------------------------------------------------+----------------------------------------------+
| Sets the ``build_extra_args attribute`` for the generated `go_repository`_ rule(s). |
+----------------------------------------------------------------------------------------------------------+----------------------------------------------+
| :flag:`-build_file_generation auto|on|off` | |
| :flag:`-build_file_generation auto|on|off|clean` | |
+----------------------------------------------------------------------------------------------------------+----------------------------------------------+
| Sets the ``build_file_generation`` attribute for the generated `go_repository`_ rule(s). |
+----------------------------------------------------------------------------------------------------------+----------------------------------------------+
Expand Down
2 changes: 2 additions & 0 deletions cmd/fetch_repo/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library", "go_test")
go_library(
name = "fetch_repo_lib",
srcs = [
"clean.go",
"copy_tree.go",
"fetch_repo.go",
"go_mod_download.go",
Expand Down Expand Up @@ -36,6 +37,7 @@ filegroup(
testonly = True,
srcs = [
"BUILD.bazel",
"clean.go",
"copy_tree.go",
"fetch_repo.go",
"fetch_repo_test.go",
Expand Down
37 changes: 37 additions & 0 deletions cmd/fetch_repo/clean.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/* Copyright 2024 The Bazel Authors. All rights reserved.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package main

import (
"io/fs"
"os"
"path/filepath"
)

func cleanBuildFiles(path string) error {
return filepath.Walk(*dest, func(path string, info fs.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
if info.Name() == "BUILD" || info.Name() == "BUILD.bazel" {
return os.Remove(path)
}
return nil
})
}
16 changes: 13 additions & 3 deletions cmd/fetch_repo/fetch_repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ var (
importpath = flag.String("importpath", "", "Go importpath to the repository fetch")
path = flag.String("path", "", "absolute or relative path to a local go module")
dest = flag.String("dest", "", "destination directory")
no_fetch = flag.Bool("no-fetch", false, "files already exist, do not fetch")
clean = flag.Bool("clean", false, "remove existing bazel build files")

// Repository flags
remote = flag.String("remote", "", "The URI of the remote repository. Must be used with the --vcs flag.")
Expand All @@ -56,8 +58,8 @@ func main() {

flag.Parse()

if *importpath == "" && *path == "" {
log.Fatal("-importpath or -path must be set")
if *importpath == "" && *path == "" && !*no_fetch {
log.Fatal("-importpath, -path, or -no-fetch must be set")
}

if *dest == "" {
Expand All @@ -67,7 +69,9 @@ func main() {
log.Fatal("fetch_repo does not accept positional arguments")
}

if *path != "" {
if *no_fetch {
// Nothing to do
} else if *path != "" {
if *importpath != "" {
log.Fatal("-importpath must not be set")
}
Expand Down Expand Up @@ -122,4 +126,10 @@ func main() {
log.Fatal(err)
}
}

if *clean {
if err := cleanBuildFiles(*dest); err != nil {
log.Fatal(err)
}
}
}
5 changes: 4 additions & 1 deletion internal/bzlmod/go_deps.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ 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"`.
doc = """One of `"auto"`, `"on"` (default), `"off"`, `"clean"`.
Whether Gazelle should generate build files for the Go module.
Expand All @@ -60,11 +60,14 @@ _GAZELLE_ATTRS = {
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.
""",
values = [
"auto",
"off",
"on",
"clean",
],
),
"build_extra_args": attr.string_list(
Expand Down
79 changes: 33 additions & 46 deletions internal/go_repository.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,22 @@ def _go_repository_impl(ctx):
# Declare Label dependencies at the top of function to avoid unnecessary fetching:
# 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")))
if not ctx.attr.urls:
fetch_repo = str(ctx.path(Label("@bazel_gazelle_go_repository_tools//:bin/fetch_repo{}".format(executable_extension(ctx)))))
generate = ctx.attr.build_file_generation == "on"
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"]
_gazelle = "@bazel_gazelle_go_repository_tools//:bin/gazelle{}".format(executable_extension(ctx))
if generate:
gazelle_path = ctx.path(Label(_gazelle))

if ctx.attr.local_path:
pass
if hasattr(ctx, "watch_tree"):
# https://github.com/bazelbuild/bazel/commit/fffa0affebbacf1961a97ef7cd248be64487d480
ctx.watch_tree(ctx.attr.local_path)
else:
print("""
WARNING: go.mod replace directives to module paths is only supported in bazel 7.1.0-rc1 or later,
Because of this changes to %s will not be detected by your version of Bazel.""" % ctx.attr.local_path)

fetch_repo_args = ["--path", ctx.attr.local_path, "--dest", ctx.path("")]
elif ctx.attr.urls:
# HTTP mode
for key in ("commit", "tag", "vcs", "remote", "version", "sum", "replace"):
Expand All @@ -153,6 +160,7 @@ def _go_repository_impl(ctx):
path = ctx.attr.importpath,
sha256 = result.sha256,
))
fetch_repo_args = ["-dest", ctx.path(""), "-no-fetch"]
elif ctx.attr.commit or ctx.attr.tag:
# repository mode
if ctx.attr.commit:
Expand Down Expand Up @@ -252,49 +260,26 @@ def _go_repository_impl(ctx):

env.update({k: ctx.os.environ[k] for k in env_keys if k in ctx.os.environ})

if ctx.attr.local_path:
local_path_env = dict(env)
local_path_env["GOSUMDB"] = "off"
# Clean existing build files if requested
if ctx.attr.build_file_generation == "clean":
fetch_repo_args += ["-clean"]

# Override external GO111MODULE, because it is needed by module mode, no-op in repository mode
local_path_env["GO111MODULE"] = "on"
# Disable sumdb in fetch_repo. In module mode, the sum is a mandatory
# attribute of go_repository, so we don't need to look it up.
fetch_repo_env = dict(env)
fetch_repo_env["GOSUMDB"] = "off"

if hasattr(ctx, "watch_tree"):
# https://github.com/bazelbuild/bazel/commit/fffa0affebbacf1961a97ef7cd248be64487d480
ctx.watch_tree(ctx.attr.local_path)
else:
print("""
WARNING: go.mod replace directives to module paths is only supported in bazel 7.1.0-rc1 or later,
Because of this changes to %s will not be detected by your version of Bazel.""" % ctx.attr.local_path)
# Override external GO111MODULE, because it is needed by module mode, no-op in repository mode
fetch_repo_env["GO111MODULE"] = "on"

command = [fetch_repo, "--path", ctx.attr.local_path, "--dest", ctx.path("")]
result = env_execute(
ctx,
command,
environment = local_path_env,
timeout = _GO_REPOSITORY_TIMEOUT,
)

if result.return_code:
fail(command)

if fetch_repo_args:
# Disable sumdb in fetch_repo. In module mode, the sum is a mandatory
# attribute of go_repository, so we don't need to look it up.
fetch_repo_env = dict(env)
fetch_repo_env["GOSUMDB"] = "off"

# Override external GO111MODULE, because it is needed by module mode, no-op in repository mode
fetch_repo_env["GO111MODULE"] = "on"

result = env_execute(
ctx,
[fetch_repo] + fetch_repo_args,
environment = fetch_repo_env,
timeout = _GO_REPOSITORY_TIMEOUT,
)
if result.return_code:
fail("%s: %s" % (ctx.name, result.stderr))
result = env_execute(
ctx,
[fetch_repo] + fetch_repo_args,
environment = fetch_repo_env,
timeout = _GO_REPOSITORY_TIMEOUT,
)
if result.return_code:
fail("%s: %s" % (ctx.name, result.stderr))

# Repositories are fetched. Determine if build file generation is needed.
build_file_names = ctx.attr.build_file_name.split(",")
Expand Down Expand Up @@ -515,15 +500,17 @@ go_repository = repository_rule(
),
"build_file_generation": attr.string(
default = "auto",
doc = """One of `"auto"`, `"on"`, `"off"`.
doc = """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.""",
directory. In `"clean"` mode, Gazelle will first remove any existing build
files.""",
values = [
"on",
"auto",
"off",
"clean",
],
),
"build_naming_convention": attr.string(
Expand Down
1 change: 1 addition & 0 deletions internal/go_repository_tools_srcs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ GO_REPOSITORY_TOOLS_SRCS = [
Label("//cmd/autogazelle:client_unix.go"),
Label("//cmd/autogazelle:server_unix.go"),
Label("//cmd/fetch_repo:BUILD.bazel"),
Label("//cmd/fetch_repo:clean.go"),
Label("//cmd/fetch_repo:copy_tree.go"),
Label("//cmd/fetch_repo:fetch_repo.go"),
Label("//cmd/fetch_repo:go_mod_download.go"),
Expand Down
2 changes: 1 addition & 1 deletion repository.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,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"`.<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. | String | optional | `"auto"` |
| <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_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
7 changes: 7 additions & 0 deletions tests/bcr/go_mod/MODULE.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,12 @@ go_deps.archive_override(
],
)

# Test a module that ships with its own BUILD files and first needs rewriting
go_deps.gazelle_override(
build_file_generation = "clean",
path = "github.com/google/go-jsonnet",
)

# Transitive dependencies have to be listed here explicitly.
go_deps.module(
indirect = True,
Expand Down Expand Up @@ -131,6 +137,7 @@ use_repo(
"com_github_datadog_sketches_go",
"com_github_envoyproxy_protoc_gen_validate",
"com_github_fmeum_dep_on_gazelle",
"com_github_google_go_jsonnet",
"com_github_google_safetext",
"com_github_stretchr_testify",
"org_golang_google_protobuf",
Expand Down
3 changes: 3 additions & 0 deletions tests/bcr/go_mod/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ require (
github.com/cloudflare/circl v1.3.7
github.com/envoyproxy/protoc-gen-validate v1.0.1
github.com/fmeum/dep_on_gazelle v1.0.0
github.com/google/go-jsonnet v0.20.0
github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2
github.com/stretchr/testify v1.6.1
golang.org/x/sys v0.15.0
Expand All @@ -27,9 +28,11 @@ require (
github.com/pmezard/go-difflib v1.0.0 // indirect
golang.org/x/text v0.9.0 // indirect
google.golang.org/protobuf v1.32.0 // indirect
gopkg.in/yaml.v2 v2.2.7 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
rsc.io/quote v1.5.2 // indirect
rsc.io/sampler v1.3.0 // indirect
sigs.k8s.io/yaml v1.1.0 // indirect
)

replace example.org/hello => ../../fixtures/hello
6 changes: 6 additions & 0 deletions tests/bcr/go_mod/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ github.com/google/go-cmp v0.5.1/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38=
github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
github.com/google/go-jsonnet v0.20.0 h1:WG4TTSARuV7bSm4PMB4ohjxe33IHT5WVTrJSU33uT4g=
github.com/google/go-jsonnet v0.20.0/go.mod h1:VbgWF9JX7ztlv770x/TolZNGGFfiHEVx9G6ca2eUmeA=
github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
github.com/google/safetext v0.0.0-20220905092116-b49f7bc46da2 h1:SJ+NtwL6QaZ21U+IrK7d0gGgpjGGvd2kz+FzTHVzdqI=
Expand Down Expand Up @@ -117,6 +119,8 @@ google.golang.org/protobuf v1.32.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHh
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b h1:QRR6H1YWRnHb4Y/HeNFCTJLFVxaq6wH4YuVdsUOr75U=
gopkg.in/check.v1 v1.0.0-20200902074654-038fdea0a05b/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo=
gopkg.in/yaml.v2 v2.2.7/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
Expand All @@ -126,3 +130,5 @@ rsc.io/quote v1.5.2 h1:w5fcysjrx7yqtD/aO+QwRjYZOKnaM9Uh2b40tElTs3Y=
rsc.io/quote v1.5.2/go.mod h1:LzX7hefJvL54yjefDEDHNONDjII0t9xZLPXsUe+TKr0=
rsc.io/sampler v1.3.0 h1:7uVkIFmeBqHfdjD+gZwtXXI+RODJ2Wc4O7MPEh/QiW4=
rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=
sigs.k8s.io/yaml v1.1.0 h1:4A07+ZFc2wgJwo8YNlQpr1rVlgUDlxXHhPJciaPY5gs=
sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
1 change: 1 addition & 0 deletions tests/bcr/go_mod/pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ go_test(
"@com_github_datadog_sketches_go//ddsketch",
"@com_github_envoyproxy_protoc_gen_validate//validate",
"@com_github_fmeum_dep_on_gazelle//:dep_on_gazelle",
"@com_github_google_go_jsonnet//:go-jsonnet",
"@com_github_google_safetext//yamltemplate",
"@com_github_stretchr_testify//require:go_default_library",
"@my_rules_go//go/runfiles",
Expand Down
6 changes: 6 additions & 0 deletions tests/bcr/go_mod/pkg/pkg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/bmatcuk/doublestar/v4"
"github.com/cloudflare/circl/dh/x25519"
"github.com/fmeum/dep_on_gazelle"
"github.com/google/go-jsonnet"
"github.com/google/safetext/yamltemplate"
"github.com/stretchr/testify/require"

Expand All @@ -38,6 +39,11 @@ func TestBuildFileGeneration(t *testing.T) {
yamltemplate.HTMLEscapeString("<b>foo</b>")
}

func TestCleanBuildFileGeneration(t *testing.T) {
// github.com/google/[email protected] requires fully replacing the BUILD files it provides
jsonnet.Version()
}

func TestGeneratedFilesPreferredOverProtos(t *testing.T) {
_, _ = ddsketch.NewDefaultDDSketch(0.01)
}
Expand Down

0 comments on commit 7d10bf7

Please sign in to comment.