Skip to content

refactor: upgrade protoc-gen-grpc-gateway to use the new Go code generator #1668

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

Merged

Conversation

adambabik
Copy link
Collaborator

References to other Issues or PRs

This is initial work to refactor how the gRPC Gateway files are generated in order to fix a problem described in #1612.

Brief description of what is fixed or changed

This change upgrades protoc-gen-grpc-gateway to use google.golang.org/protobuf/compiler/protogen which is the new package for writing protoc plugins generating Go code.

It also fixes all places to use packages from google.golang.org/protobuf module instead of github.com/golang/protobuf.

Other comments

I tested it by running go test ./... and regenerating all examples/internal/proto/examplepb/*.gw.go files using protoc with a new bin/protoc-gen-grpc-gateway binary. There were no changes.

…rator

This change upgrades protoc-gen-grpc-gateway to use
google.golang.org/protobuf/compiler/protogen
which is the new package for writing protoc plugins
generating Go code.

It also fixes all places to use packages from
google.golang.org/protobuf module instead of
github.com/golang/protobuf.

Signed-off-by: Adam Babik <[email protected]>
@googlebot
Copy link

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@adambabik
Copy link
Collaborator Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@adambabik
Copy link
Collaborator Author

It seems like I need to update Bazel BUILD files. Is there a way to regenerate them or I should fix them manually?

@achew22
Copy link
Collaborator

achew22 commented Sep 15, 2020

@adambabik thanks for your contribution! I'm extremely excited for this one. Assuming the major structure hasn't changed (looking through the PR it doesn't look like it but I won't rule it out entirely), you should be able to invoke http://godoc.org/github.com/bazelbuild/bazel-gazelle/cmd/gazelle in the root of the repo and it will generate all the BUILD file changes for you. If that doesn't work, please post back here and I'll fix them manually on your PR

@johanbrandhorst
Copy link
Collaborator

This is a really impressive piece of work. I haven't looked at it yet but I think you'll find that we can't replace the old github.com/golang/protobuf imports yet because of how bazel imports them. If you revert the import changes (for now) it should be possible to regenerate the files using the commands listed in CONTRIBUTING.md.

@adambabik
Copy link
Collaborator Author

@achew22 thanks, it seems that it worked and the CI pipeline is happy :)

@johanbrandhorst Thanks! It's just a first step actually, there will be more changes to fully embrace protogen.Plugin. Here, I merely wrapped the existing code into it.

I actually managed to run the commands from CONTRIBUTING.md regenerating files with the existing changes and they did not complain. Do you think I still should revert to github.com/golang/protobuf imports due to some reason?

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This looks great, thank you so much for taking the time to get this done! I understand it's just the start, but great work!

@johanbrandhorst johanbrandhorst merged commit ad1c1bb into grpc-ecosystem:v2 Sep 15, 2020
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

@achew22
Copy link
Collaborator

achew22 commented Sep 15, 2020

To echo Johan even more, thank you so much! I think this will really help cut down on the bugs we get where there is some weird flag parsing thing that I implement incorrectly 5 years ago 😁

@adambabik
Copy link
Collaborator Author

Thank you folks! :) I will continue working on the next PR to fully embrace protogen package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants