Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go_repository: add 'clean' build_file_generation #1802

Merged
merged 2 commits into from
Jun 1, 2024

Conversation

TvdW
Copy link
Contributor

@TvdW TvdW commented May 14, 2024

What type of PR is this?

Feature

What package or component does this PR mostly affect?

go_repository

What does this PR do? Why is it needed?

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 https://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.

Which issues(s) does this PR fix?

Fixes #1773, #1769, #1535, bazelbuild/rules_go#3685

Other notes for review

I have not made 'clean' the default, though I think that for most use cases it will be the correct behavior.

An example for fixing jsonnet:

go_deps.gazelle_override(
    build_file_generation = "clean",
    path = "github.com/google/go-jsonnet",
)

With some gymnastics this does also make it possible to import googleapis as if it were a regular Go dependency, without any switched_rules:

go_deps.module(
    path = "github.com/googleapis/googleapis",
    sum = "unused",
    version = "0.0.0",
)
go_deps.gazelle_override(
    build_file_generation = "clean",
    directives = [
        "gazelle:proto file",
    ],
    path = "github.com/googleapis/googleapis",
)
go_deps.archive_override(
    path = "github.com/googleapis/googleapis",
    sha256 = "7faad14f34810d1165a9873c140b378bc783a1448e46ebdd20a65ca66afa7bb1",
    strip_prefix = "googleapis-7002406181eb300da880701035a25157a5099abb",
    urls = ["https://github.com/googleapis/googleapis/archive/7002406181eb300da880701035a25157a5099abb.zip"],
)

# resolution
 "gazelle:resolve proto google/rpc/code.proto @com_github_googleapis_googleapis//google/rpc:code_proto",
 "gazelle:resolve proto google/rpc/status.proto @com_github_googleapis_googleapis//google/rpc:status_proto",
 "gazelle:resolve proto google/api/annotations.proto @com_github_googleapis_googleapis//google/api:annotations_proto",
 "gazelle:resolve proto google/api/field_behavior.proto @com_github_googleapis_googleapis//google/api:field_behavior_proto",

@TvdW
Copy link
Contributor Author

TvdW commented May 14, 2024

I've marked this PR as draft as I have not yet added tests, but I am planning to.

@fmeum
Copy link
Collaborator

fmeum commented May 14, 2024

This is highly appreciated, let me know if you get stuck anywhere.

@fmeum
Copy link
Collaborator

fmeum commented May 14, 2024

Cc @linzhp for googleapis

@TvdW TvdW force-pushed the build-file-generation-clean branch from 1cf1c77 to d0d39df Compare May 15, 2024 09:05
@TvdW TvdW marked this pull request as ready for review May 15, 2024 09:06
@TvdW TvdW marked this pull request as draft May 15, 2024 09:08
@TvdW TvdW force-pushed the build-file-generation-clean branch 2 times, most recently from 6adecf6 to 6939d6a Compare May 15, 2024 09:58
@TvdW
Copy link
Contributor Author

TvdW commented May 15, 2024

Ok, I've added tests. Turns out it broke on Windows, so I fixed that and then it broke on Bazel 6.x, so I ended up adding the code into cmd/fetch_repo. My initial attempt was to add it into Gazelle itself, however between fetch_repo and Gazelle the go_repository code first writes a BUILD file with directives, and that would have been removed/ignored by my changes. Tests are green now 😄

This adds a test to see if this error is fixed:

ERROR: no such package '@@[unknown repo 'cpp_jsonnet' requested from @@gazelle~~go_deps~com_github_google_go_jsonnet]//stdlib': The repository '@@[unknown repo 'cpp_jsonnet' requested from @@gazelle~~go_deps~com_github_google_go_jsonnet]' could not be resolved: No repository visible as '@cpp_jsonnet' from repository '@@gazelle~~go_deps~com_github_google_go_jsonnet'
ERROR: /private/var/tmp/_bazel_tom.vanderwoerdt/c65fedc35a8ffb2abdd111f841d4694d/external/gazelle~~go_deps~com_github_google_go_jsonnet/astgen/BUILD.bazel:3:8: no such package '@@[unknown repo 'cpp_jsonnet' requested from @@gazelle~~go_deps~com_github_google_go_jsonnet]//stdlib': The repository '@@[unknown repo 'cpp_jsonnet' requested from @@gazelle~~go_deps~com_github_google_go_jsonnet]' could not be resolved: No repository visible as '@cpp_jsonnet' from repository '@@gazelle~~go_deps~com_github_google_go_jsonnet' and referenced by '@@gazelle~~go_deps~com_github_google_go_jsonnet//astgen:dumpstdlibast'

I'm not testing any of the googleapis things I mentioned: since googleapis is technically not a Go repository, getting it working is a happy side-effect.

@TvdW TvdW marked this pull request as ready for review May 15, 2024 10:04
@TvdW TvdW force-pushed the build-file-generation-clean branch from 6939d6a to 3329c41 Compare May 15, 2024 10:06
@TvdW
Copy link
Contributor Author

TvdW commented May 15, 2024

I've added one more commit on top, reordering (and deduplicating) some code so that fetch_repo is always called. From the commit message:

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).

Copy link
Collaborator

@fmeum fmeum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyler-french Could you test this?

@fmeum
Copy link
Collaborator

fmeum commented May 21, 2024

Thanks for getting rid of some unnecessary special casing, the code is easier to follow now even though it's longer.

TvdW added 2 commits June 1, 2024 09:21
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.
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).
@fmeum fmeum force-pushed the build-file-generation-clean branch from 750e401 to 1fc5587 Compare June 1, 2024 07:21
@fmeum fmeum merged commit 7d10bf7 into bazelbuild:master Jun 1, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose bazel_deps to go_deps extension
2 participants