Update go_repository defaults to only use Go code #1888
Conversation
TvdW
force-pushed
the
go-get-as-default
branch
2 times, most recently
from
6a4804e to
82029b0
Compare
|
Bit of context: a recent Go module update pulled in a dependency-of-a-dependency that has BUILD files but doesn't actually build. |
| 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): |
There was a problem hiding this comment.
What's the reason for the explicit none check?
There was a problem hiding this comment.
An empty list is False, but we do want to consider that to be an override.
In the default list of overrides I'm adding
"github.com/bazelbuild/buildtools": [],
Since buildtools doesn't like proto:disable. However, the None check is needed since this is still considered False
There was a problem hiding this comment.
The above is partially incorrect: buildtools doesn't even have proto code used by gazelle. That said, the rest is still true: the None check is needed so that we can specify an empty list, which otherwise would get reverted back to the default (proto:disable).
| "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", |
There was a problem hiding this comment.
Your PR contains an override that switches this to off for WORKSPACE reps, is that intentional?
There was a problem hiding this comment.
Hm, no, I think that's accidental. It should be off, I believe. Will test it to see which one it was 😄
There was a problem hiding this comment.
Ok, found it.
Any of update, off, or clean will work here in a 'vanilla' bazel repo. The reason I set it to update and off is that my own code was importing from the buildtools repo, and since that was my test case I accidentally blindly copied that over. The reality is that the majority of users have no need for this and only have buildtools as a transient dep of gazelle and rules_go, so... I think it can go? 😄
Since this can never happen in a transient dependency anyway, I will update my PR to remove this.
TvdW
force-pushed
the
go-get-as-default
branch
2 times, most recently
from
b19e843 to
9ca907f
Compare
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`.
TvdW
force-pushed
the
go-get-as-default
branch
from
9ca907f to
62181b1
Compare
What type of PR is this?
Feature, I guess?
What package or component does this PR mostly affect?
What does this PR do? Why is it needed?
This is an attempt at making
go_repositoryand its bzlmod equivalent pull Go modules as similar as possible to whatgo getwould 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
genrules and other code meant for updating the sources, and in some cases Gazelle just can't fix those.I previously added
build_file_generation=cleanfor this, and it works great undergazelle_default_attributesin 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 disableeverywhere, because we import modules without using the proto files.So, we can insert
gazelle:proto disableby 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.Which issues(s) does this PR fix?
Fixes #1736
Closes #1799
... and a bunch more, because this is a very popular issue topic
Other notes for review
The commit, as-is, is backwards incompatible. I first want to gather whether people are aligned on this idea (to update the defaults) before trying to figure out how to make this backwards compatible (if needed).