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

buf breaking with .txtpb files #2355

Closed
edzaffirm opened this issue Aug 8, 2023 · 17 comments
Closed

buf breaking with .txtpb files #2355

edzaffirm opened this issue Aug 8, 2023 · 17 comments

Comments

@edzaffirm
Copy link

I have protoc outputting a file descriptor set, with a .txtpb extension. When I use the protoc-gen-buf-breaking plugin, the breaking change detector is able to compare against the file descriptor set as expected. However, when I try to do the same via the CLI:

$ buf breaking --limit-to-input-files --against .../foo.txtpb --path bar.proto

I get:

Failure: could not unmarshal image: proto: syntax error (line 2:1): invalid field name: �

Is this expected?

@bufdev
Copy link
Member

bufdev commented Aug 8, 2023

Hmm - no, not expected heh. Can you provide us a reproducible example?

@edzaffirm
Copy link
Author

proto.zip

The zip includes the .proto files, compiled Python files, and .txtpb file, used as a basis for comparison.

@bufdev
Copy link
Member

bufdev commented Aug 9, 2023

I'm not sure what to do with the files - can you provide a specific, reproducible example with reproduction steps? For example, indicate which commands to run to see the error.

@edzaffirm
Copy link
Author

buf_example.zip

I remade the zip so that it matches more closely with my repo structure. You'll want to run the commands from the parent of this zip once it's unarchived.

Using the Python grpc tools plugin (which in turn uses protoc) works as expected:

$ python -m grpc_tools.protoc --proto_path=. --include_imports --include_source_info -o buf_example/proto/image.txtpb --python_out=. --grpc_python_out=. --buf-breaking_out=. '--buf-breaking_opt={"against_input":"buf_example/proto/image.txtpb","limit_to_input_files":true}' buf_example/proto/messages.proto buf_example/proto/interface.proto

--buf-breaking_out: buf_example/proto/messages.proto:10:3:Field "1" on message "GetExampleEndpointResponse" changed type from "string" to "double"

Using the buf cli, I get this error:

$ buf breaking --against buf_example/proto/image.txtpb --path buf_example/proto/messages.proto --path buf_example/proto/interface.proto

Failure: could not unmarshal image: proto: syntax error (line 2:1): invalid field name: �

@bufdev
Copy link
Member

bufdev commented Aug 9, 2023

You're creating a binary format FileDescriptorSet in your first command. protoc -o outputs binary format, not text format. protoc-gen-buf-breaking is not reading image.txtpb to run, it is instead using the build result of protoc internally to then run protoc-gen-buf-breaking. In your example, image.txtpb is never used by protoc or protoc-gen-buf-breaking. As a result, when you do buf breaking --against image.txtpb, buf expects a text image, but is getting a binary image, which results in the unmarshal error.

However, if you were using buf build to create image.txtpb (unrelated to the above example), the following example is indeed broken:

buf build -o image.txtpb # incorrectly creates a binary image
buf breaking --against image.txtpb

This is being fixed in #2361. As a temporary workaround, this will correctly create a text image:

buf build -o image.txtpb#format=txtpb
buf breaking --against image.txtpb

@edzaffirm
Copy link
Author

edzaffirm commented Aug 9, 2023

Thanks for looking into it! A few questions:

protoc-gen-buf-breaking is not reading image.txtpb to run, it is instead using the build result of protoc internally to then run protoc-gen-buf-breaking

Why do I need to supply the against_input field in '--buf-breaking_opt={"against_input":"image.txtpb","limit_to_input_files":true}' \ when using protoc-gen-buf-breaking then?

protoc -o outputs binary format, not text format

Is it possible for it to output a text format instead, or do you have another recommendation here? Since my org uses protoc to generate all of our protobufs, I wanted to build the image to compare against in the same invocation -- it seems like if I switch to buf build, I'd need to run protoc ... then buf build -o ... for buf breaking to work.

@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

Why do I need to supply the against_input field in '--buf-breaking_opt={"against_input":"image.txtpb","limit_to_input_files":true}' \ when using protoc-gen-buf-breaking then?

Actually I was mistaken - protoc-gen-buf-breaking IS supposed to use image.txtpb for the against input, but uses the result from protoc for the main input. Why image.txtpb is working in your above command I am not sure - you'd need to give us a specific, reproducible example for how you originally created image.txtpb prior to this invocation of protoc. If you are saying that this same invocation of protoc is both creating image.txtpb AND running breaking change detection, then this is invalid - the against input is meant to check against previous imput, and what is likely happening here is that image.txtpb is empty when you are running protoc-gen-buf-breaking. However, without a complete reproducible end-to-end example of the exact commands you are running, including image.txtpb creation, I can't be sure.

Is it possible for it to output a text format instead

With buf, it's easy - just buf build -o to a *.txtpb file. With protoc, it's very complicated, and not worth pursuing for your case - for completeness, you need to do something like protoc -o /dev/stdout path/to/your/file.proto | protoc -I /path/to/google-wkt --decode=google.protobuf.FileDescriptorSet /path/to/google-wkt/google/protobuf/descriptor.proto, but it's complicated to explain what all of that is.

it seems like if I switch to buf build, I'd need to run protoc ... then buf build -o ... for buf breaking to work.

You don't need to use protoc at all with buf. buf build -o is all you need, you don't need to do any protoc compilation.

@edzaffirm
Copy link
Author

If you are saying that this same invocation of protoc is both creating image.txtpb AND running breaking change detection, then this is invalid - the against input is meant to check against previous imput, and what is likely happening here is that image.txtpb is empty when you are running protoc-gen-buf-breaking. However, without a complete reproducible end-to-end example of the exact commands you are running, including image.txtpb creation, I can't be sure.

Ah so my current implementation is this. For reference, this is the command I'm using:

python -m grpc_tools.protoc --proto_path=. --include_imports --include_source_info -o buf_example/proto/image.txtpb --python_out=. --grpc_python_out=. --buf-breaking_out=. '--buf-breaking_opt={"against_input":"buf_example/proto/image.txtpb","limit_to_input_files":true}' buf_example/proto/messages.proto buf_example/proto/interface.proto
  1. Check if an image.txtpb file exists. If it doesn't, I don't append the buf_breaking_out... and --buf-breaking_opt flags to the protoc. command.
  2. The protoc command would then proceed and write the binary file image.txtpb to disk
  3. On the next invocation, since image.txtpb exists, I add the flags for protoc-gen-buf-breaking.
  4. Buf breaking, via the plugin, either warns me or if it's successful, the protoc files and a new image.txtpb are generated.

@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

I've attempted to create exact end-to-end reproduction steps, but am unable to reproduce the success of this command. If I run:

rm -f image.txtpb
protoc \
  --proto_path=. \
  --include_imports \
  --include_source_info \
  -o image.txtpb \
    buf_example/proto/messages.proto \
    buf_example/proto/interface.proto
protoc \
  --proto_path=. \
  --buf-breaking_out=. \
  '--buf-breaking_opt={"against_input":"image.txtpb","limit_to_input_files":true}' \
    buf_example/proto/messages.proto \
    buf_example/proto/interface.proto

The result is:

could not unmarshal image: proto: syntax error (line 2:1): invalid field name:
--buf-breaking_out: protoc-gen-buf-breaking: Plugin failed with status code 1.

Which is as expected.

@edzaffirm
Copy link
Author

Hmm weird, I ran those same commands on my machine and it worked perfectly fine. I'm using libprotoc 3.13. on MacOS, not sure if that makes a difference.

@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

I'm not sure, but perhaps you have some other installation issue. If you're able to provide reproduction steps for us, we're happy to take a look, but since we're not able to reproduce this issue, I'm not sure there's more we can do.

Closing for now, but let us know if you run into other difficulties.

@bufdev bufdev closed this as completed Aug 10, 2023
@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

I just had a potential reason this might be the case - can you confirm what version of protoc-gen-buf-breaking you are using? If it is before v1.25.0, then txtpb was not recognized as text format, meaning protoc-gen-buf-breaking would interpret image.txtpb as binary by default.

@edzaffirm
Copy link
Author

I'm unable to get the version of protoc-gen-buf-breaking directly (protoc-gen-buf-breaking --version just hangs), but I'm using buf 1.25.1.

@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

protoc-gen-buf-breaking doesn't accept any flags, but I would find out if you are using a version <1.25.0. If you are, then upgrade and you'll be fine.

@edzaffirm
Copy link
Author

Oh, how can I check the version of protoc-gen-buf-breaking?

@bufdev
Copy link
Member

bufdev commented Aug 10, 2023

There's no builtin way - you'll need to check from where you're installing.

@edzaffirm
Copy link
Author

So, I just upgraded my buf install to 1.26.1 -- and that resulted in the same behavior that you were experiencing. If that's the expected result, I'll go ahead and update my usage. Thanks again for the assistance.

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

No branches or pull requests

2 participants