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.

Sign up for GitHub

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

Jump to bottom

[Fixes #1736] disable proto generation for external go modules by default #1799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[Fixes #1736] disable proto generation for external go modules by default #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 5 additions & 36 deletions internal/bzlmod/default_gazelle_overrides.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,60 +22,29 @@ DEFAULT_BUILD_FILE_GENERATION_BY_PATH = {
}

DEFAULT_DIRECTIVES_BY_PATH = {
"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/gogo/googleapis": [
"gazelle:proto disable",
],
"github.com/gogo/protobuf": [
"gazelle:proto disable",
"gazelle:proto disable_global",
Copy link
Contributor Author

@stefanpenner stefanpenner May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Context for this line (and the others that re-introduce gazelle:proto disable_global)

given how overrides work, when an override is provided it will override the default behavior. This means existing overrides that wish to have the new default behavior, will need to now include gazelle:proto disable_global themselves.

This feels a little odd, so we should discuss. As far as I see we have the following options:

a. do what this PR does
b. don't bother introducing gazelle:proto disable_global to this DEFAULT_DIRECTIVES unless the mod has proto files already
c. invest in more advanced merging so that the defaults are not lost during merging. This would then also require the ability selectively override existing defaults
d. explore deeper gazelle support for this change in default for when being run in "repo mode"

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should do b: if those modules don't have gazelle:proto disable already, it either means they don't have proto files or people still need the proto files.

],
"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",
"gazelle:proto disable_global",
],
"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",
"gazelle:proto disable_global",
],
"github.com/pseudomuto/protoc-gen-doc": [
# The build file in github.com/mwitkow/go-proto-validators has both go_proto and gogo_proto targets, but the checked
# 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",
"gazelle:proto disable_global",
],
"k8s.io/apimachinery": [
"gazelle:go_generate_proto false",
"gazelle:proto_import_prefix k8s.io/apimachinery",
"gazelle:proto disable_global",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's don't disable proto here. People will need the proto targets from this module, otherwise they will have to download it again with http_archive.

],
}

Expand Down
2 changes: 1 addition & 1 deletion internal/bzlmod/go_deps.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def _get_override_or_default(specific_overrides, gazelle_default_attributes, def
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_global"], "directives")
Copy link
Contributor Author

@stefanpenner stefanpenner May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want this to be disable or disable_global ?

Copy link
Contributor

Choose a reason for hiding this comment

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

For a Go module without any proto file, disable and disable_global no longer have a difference. I slightly prefer "disable" just because it's shorter and doesn't let people wonder what "global" means


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.
Expand Down