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

Add support for protobuf editions #54

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

solidDoWant
Copy link

Version bump + 4 line change to add support for protobuf editions up to 2023 (current latest supported by protoc-gen-go). This has no effect on the program output, but without it, protoc will fail with

<proto file name>.proto: is an editions file, but code generator protoc-gen-go-grpcmock hasn't been updated to support editions yet.  Please ask the owner of this code generator to add support or switch back to proto2/proto3.

See https://protobuf.dev/editions/overview/ for more information.

on any proto files that use edition instead of syntax.

Comment on lines 45 to 47
gen.SupportedFeatures = uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL) | uint64(pluginpb.CodeGeneratorResponse_FEATURE_SUPPORTS_EDITIONS)
gen.SupportedEditionsMinimum = descriptorpb.Edition_EDITION_PROTO2
gen.SupportedEditionsMaximum = descriptorpb.Edition_EDITION_2023
Copy link
Collaborator

Choose a reason for hiding this comment

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

@solidDoWant Thank you for your contribution! Adding support for editions is really appreciated. The only thing I'm a bit skeptical about is specifying a min/max version.

What is the benefit of adding the minimum and maximum supported versions here? To my knowledge we are not making use of any version specific features. Adding a maximum version means that we would have to maintain this from now on and bump it every time a new edition comes out, which I would like to avoid if possible.

Copy link
Author

Choose a reason for hiding this comment

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

Adding a maximum version means that we would have to maintain this from now on and bump it every time a new edition comes out, which I would like to avoid if possible.

Completely get that. Unfortunately I think it's required. I did some local testing and without these set, the generator with a very generic error about unsupported versions. Every other tool with editions support seems to have these set.

I think I found a workaround to solve the issue. protoc-gen-go sets these using constants, which the upstream project updates when new versions release. 58237fb sets the values to these variables, so while they will need to be updated upstream (and a new release cut), the code itself won't need to change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, that's unfortunate. Though in this scenario, I'd prefer to explicitly define the constants for Min/MaxSupportedVersions (sorry for the back and forth) since I think it would be weird to tie this to a specific protoc-gen-go version. If we have to increment these every time a new edition is released, then I'd prefer it to actually be a code change instead of changing something in the background by just updating the dependencies.

Would you be able to also add a file with editions syntax to the examples? E.g. just taking https://github.com/lovoo/protoc-gen-go-grpcmock/blob/main/examples/helloworld/helloworld.proto and add another version of this file with editions syntax? Just so we can verify that we are able to support it?

…ions

Signed-off-by: solidDoWant <fred.heinecke@yahoo.com>
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.

2 participants