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

Prefer go_grpc_library #1711

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mering
Copy link
Contributor

@mering mering commented Jan 4, 2024

What type of PR is this?

Bug fix

What package or component does this PR mostly affect?

language/go

What does this PR do? Why is it needed?

See issue.

Which issues(s) does this PR fix?

Fixes #1709

Other notes for review

Depends on bazelbuild/rules_go#3812

func migrateGrpcCompilers(c *config.Config, f *rule.File) {
for _, r := range f.Rules {
if r.Kind() != "go_grpc_library" || r.ShouldKeep() || r.Attr("compilers") != nil {
if r.Kind() != "go_proto_library" || r.ShouldKeep() || r.Attr("compilers") == nil || len(r.AttrStrings("compilers")) != 1 || !strListAttrContains(r, "compilers", oldGrpcCompilerLabel) {
Copy link
Contributor Author

@mering mering Jan 4, 2024

Choose a reason for hiding this comment

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

How can I check if the corresponding proto .hasServices() like in generate.go instead of checking for the old compiler name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this fixer to unblock this PR as we do no longer need to convert in this direction. Existing rules will just be kept as is which should continue working. Adding a fixer to convert the opposite direction might be added in a future PR.

repository.md Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried updating the docs via bazel run //docs:update but the tests keep failing on GitHub (the docs tests were successful on my local machine though).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can look into that when the other issues are resolved, don't worry.

@ryanpbrewster
Copy link

Is there a way to configure the behavior of gazelle depending on what version of rules_go it is being used with?

@mering
Copy link
Contributor Author

mering commented Jan 4, 2024

Is there a way to configure the behavior of gazelle depending on what version of rules_go it is being used with?

I don't know but I would avoid such things whenever possible as they just introduce a lot of additional complexity and locations one has to check when debugging.

@mering mering marked this pull request as ready for review January 5, 2024 13:22
@fmeum
Copy link
Collaborator

fmeum commented Jan 5, 2024

I would also avoid this. With Bzlmod, each version of Gazelle can declare which minimum version of rules_go it is compatible with. Without Bzlmod, version resolution is non-existent anyway, so I'm fine with "users have to update Gazelle and rules_go in lockstep" since that's the most promising approach to solve any kind of WORKSPACE issue anyway.

@fmeum
Copy link
Collaborator

fmeum commented Jan 7, 2024

I submitted bazelbuild/rules_go#3818 to temporarily undo the deprecation until we figure out the Gazelle migration.

language/go/fix_test.go Outdated Show resolved Hide resolved
This allows for easier compiler upgrades via rules_go upgrades
@mering
Copy link
Contributor Author

mering commented Jan 9, 2024

Tests are passing locally now. This should be enough to unblock this PR and adding back the deprecation in rules_go. As my company does not need any such Gazelle changes, unfortunately I can't spend more time on this.

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.

@linzhp Could you review and test this as well?

Copy link
Contributor

@linzhp linzhp left a comment

Choose a reason for hiding this comment

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

I prefer generating go_proto_library and set compilers attributes, which is more flexible and more consistent with Gazelle's philosophy of "no hidden magic" (Yes, there are codebases not using Gazelle and extensively use macros to make magic happen, so people don't need to write too much build files). This is the direction we are moving to, unless there is something I am missing.

It's OK for Gazelle to generate the new v2 grpc plugin by default when # gazelle:go_grpc_compilers is not set to encourage new codebase to use the new plugin. But for existing codebase, they can decide their timeline of migration by setting # gazelle:go_grpc_compilers at different levels of directory.

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.

gazelle generates and recreates grpc targets that rules_go complains about
5 participants