bufbuild / buf

The best way of working with Protocol Buffers.
https://buf.build
Apache License 2.0
8.93k stars 267 forks source link

Set Editions-related fields in CodeGeneratorResponse when proxying to protoc #3094

Closed jhump closed 3 months ago

jhump commented 3 months ago

v27 of protoc fully supports Editions, but when using the buf CLI and proxying to it (for generating Java, C++, Python, etc), it refuses to accept Editions. The reason is that buf CLI's proxying code wasn't setting the Editions-related fields in the code generator response. This fixes that.

This also updates buf alpha protoc --version to print 27.0-buf instead of 5.27.0-buf. This mirrors protoc, which prints libprotoc 27.0. (The "5" major version is the backwards-compatibility level of the C++ runtime and protoc stopped reporting that initial extra version number starting in v21, which was v3.21 of the C++ runtime.)

jhump commented 3 months ago

@pkwarren, I just realized that since we don't bother setting --experimental_editions when we run protoc, it will fail separately if the inputs include an edition it refuses to support (with a message about experimental editions).

So I think that means we can greatly simplify this and just always report EDITION_MAX, to advertise this as supporting all editions and let protoc itself generate an error if the inputs include an unsupported edition. The downside is that the error message could be a little confusing (since passing the flag that is mentioned to buf generate would obviously be incorrect).

Also, there's not currently a way to allow the user to specify an extra arg to protoc (like to provide the --experimental_editions flag). But we do have a way to do that with normal plugins: the path property in the YAML file can be a slice of strings instead of a single string. Maybe we should do the same for the protoc_path property, to allow users to set that flag?

jhump commented 3 months ago

Take a look at #3098 as a possible alternative.

bufdev commented 3 months ago

Approved #3098, assuming that usurps this PR.

jhump commented 3 months ago

Closing this. Will merge the alternative in #3098 instead.