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

Migrate github.com/golang/protobuf to google.golang.org/protobuf #743

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

jbduncan
Copy link
Contributor

Fixes #470.

This PR fixes the merge conflicts in #490 and uses the latest protoc and protoc-gen-go to fix simple_message.proto with command protoc --go_out=ghttp/protobuf ghttp/protobuf/simple_message.proto.

@onsi
Copy link
Owner

onsi commented Mar 16, 2024

hey - thanks. just wanted to check (I'm not super familiar with the protobuf ecosystem so bear with me if this is a dumb question): can I safely assume this is a backward compatible change?

Thanks!

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 17, 2024

@onsi Good question! I'm just as unfamiliar with the protobuf ecosystem as you are, but I had to regenerate ghttp/protobuf/simple_message.pb.go with the latest protoc compiler to prevent Go compilation errors when passing it to gomega.VerifyProtoRepresenting, so I think it is incompatible, sadly.

It looks like it's possible to pass instances of old *.pb.go files like the master version of simple_message.pb.go to VerifyProtoRepresenting with VerifyProtoRepresenting(protoadapt.MessageV2Of(simpleMessage)), but existing code will still be broken and I don't know what the recommended way forward for libraries like Gomega is.

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 17, 2024

@onsi I think I've found a way to maintain source backwards compatibility (see e61836c).

On master (which uses github.com/golang/protobuf v1.5.3), VerifyProtoRepresenting accepts proto.Message which is an alias for protoiface.MessageV1. But as of this PR changing things to google.golang.org/protobuf v1.33.0, Protobuf broke backwards compatibility by changing proto.Message to be an alias for protoreflect.ProtoMessage, which seems to be Protobuf's "message V2" API.

Therefore, I've changed VerifyProtoRepresenting to accept protoiface.MessageV1, which is structurally the same interface as what proto.Message was before. Also the new version of simple_message.pb.go implements it, so V2 message types are backwards-compatible too, thankfully.

I decided not to revert simple_message.pb.go back to its old implementation because it used the old package github.com/golang/protobuf, and it didn't feel right to leave Gomega's tests stuck with that package given it's been deprecated.

Any thoughts? Otherwise, I think we're good to go.

@onsi
Copy link
Owner

onsi commented Mar 18, 2024

thanks @jbduncan - you're now the expert 😉

I'll merge this in and cut a release. If folks open up issues I'll tag you on them so we can figure out what to do together if need be. Sound ok?

@onsi onsi merged commit a350b95 into onsi:master Mar 18, 2024
6 checks passed
@jbduncan jbduncan deleted the golang-protobuf branch March 18, 2024 19:41
@jbduncan
Copy link
Contributor Author

thanks @jbduncan - you're now the expert 😉

I'll merge this in and cut a release. If folks open up issues I'll tag you on them so we can figure out what to do together if need be. Sound ok?

@onsi Haha! I suppose I am! Yep, that's totally fine with me. Thanks for merging this in so quickly. 👍

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.

Migrate to google.golang.org/protobuf
2 participants