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

wants to merge 1 commit into from

Conversation

stefanpenner
Copy link
Contributor

@stefanpenner stefanpenner commented May 7, 2024 •
edited
Loading

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

Note I am really not an expert in the proto / go world, so I could be misunderstanding some important details. I've attempted to do some research, but someone with more expertise here should likely sanity check this approach.

What type of PR is this?

Uncomment one line below and remove others.

Bug fix

Feature
Documentation
Other

What package or component does this PR mostly affect?

For example:

language/go
cmd/gazelle

go_repository

all

What does this PR do? Why is it needed?

By convention go modules with proto files are to generate pb.go bindings at publish time, this PR ensures we don't regenerate those bindings for external dependencies, as doing so is quite wasteful as it introduces the full protoc toolchain which itself also needs to be built from scratch.

For folks that want the old behavior, they can add an override for the go mod in question by doing the following:

go_deps.gazelle_override(
    directives = [ ],
    path = "<go module id>",
)

Which issues(s) does this PR fix?

Fixes #1736

Other notes for review

Tasks

  • prototype
  • Ensure it doesn't break CI
  • test in real application
  • sanity check approach with someone more familiar with this domain then myself
  • discuss automated testing strategy for this PR with gazelle team
  • doc updates?

@google-cla Google CLA
Copy link

google-cla bot commented May 7, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@stefanpenner stefanpenner force-pushed the flip-proto-generation-default branch from 2c70b62 to 3858e9a Compare May 7, 2024 20:46
stefanpenner
stefanpenner commented May 7, 2024
View reviewed changes
internal/bzlmod/default_gazelle_overrides.bzl
],
"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.

internal/bzlmod/go_deps.bzl
@@ -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

@stefanpenner stefanpenner marked this pull request as ready for review May 7, 2024 22:49
linzhp
linzhp approved these changes May 18, 2024
View reviewed changes
internal/bzlmod/default_gazelle_overrides.bzl
],
"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.

internal/bzlmod/default_gazelle_overrides.bzl
],
"github.com/gogo/protobuf": [
"gazelle:proto disable",
"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.

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.

internal/bzlmod/go_deps.bzl
@@ -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

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

@hunshcn
Copy link
Contributor

hunshcn commented Jul 15, 2024

Any update?

@hunshcn
Copy link
Contributor

hunshcn commented Jul 28, 2024

ping @stefanpenner
I think this work is very meaningful, and it would be great if it could be merged.

@TvdW TvdW mentioned this pull request Aug 23, 2024
Open
@stefanpenner
Copy link
Contributor Author

Looks Like im working on bazel stuff again, so I will try to carve out some time to help Shepard this forward.

@fmeum
Copy link
Member

fmeum commented Aug 28, 2024

There's also #1888, so @TvdW and you might want to align.

@TvdW
Copy link
Contributor

TvdW commented Aug 29, 2024

There's also #1888, so @TvdW and you might want to align.

Indeed, I created mine after noticing that this one was not moving forward anymore. They do the same thing. Mine covers both build files and proto files, so if we land this one first it reduces the diff on mine. That said, I'm happy to push them both forward at the same time in my PR as well. Your call, @stefanpenner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linzhp linzhp linzhp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proto generation is not yet disabled in go.mods provided by go_deps in bzlmod
5 participants
@stefanpenner @hunshcn @fmeum @TvdW @linzhp