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

How to handle compatibility issues with v1.4? #1126

Closed
RainbowMango opened this issue May 13, 2020 · 5 comments
Closed

How to handle compatibility issues with v1.4? #1126

RainbowMango opened this issue May 13, 2020 · 5 comments

Comments

@RainbowMango
Copy link

The struct type InternalMessageInfo has been deprecated by github.com/golang/protobuf@v1.4.0:

// Deprecated: Do not use.
type InternalMessageInfo struct{}
func (*InternalMessageInfo) DiscardUnknown(Message) { panic("not implemented") }
func (*InternalMessageInfo) Marshal([]byte, Message, bool) ([]byte, error) { panic("not implemented") }
func (*InternalMessageInfo) Merge(Message, Message) { panic("not implemented") }
func (*InternalMessageInfo) Size(Message) int { panic("not implemented") }
func (*InternalMessageInfo) Unmarshal(Message, []byte) error { panic("not implemented") }

But there are a whole bunch of modules still using *.pb.go file which contains a reference of InternalMessageInfo, such as

Kubernetes wants to involve a new module version which depends on github.com/golang/protobuf@v1.4.0, but this work has been blocked as several modules in Kubernetes' vendor still referring InternalMessageInfo, more details please refer to kubernetes/kubernetes#90582 (comment).

Does it's the right approach to update all those modules by re-compile *.proto?
If yes, there are two protoc-gen-go, one and two, which one should I choose? and should I use the latest version?

@dsnet I will be appreciate if you can give some guidence.

@dsnet
Copy link
Member

dsnet commented May 13, 2020

Does it's the right approach to update all those modules by re-compile *.proto?

There shouldn't be a need to regenerate existing .pb.go files for things to work with v1.4.0. However, it is certainly recommended to regenerate with a newer version of protoc-gen-go. In what ways was this causing a "compatibility issue"?

The InternalMessageInfo is a pseudo-internal type that should be only referenced by generated .pb.go files. They are used to implement the XXX_Marshal, XXX_Size, etc methods. However, the
compatibility document explicitly calls out identifiers with an XXX prefix as being internal. The v1.3.x runtime used the XXX methods to perform serialization, however the v1.4.x runtime uses an entirely different implementation to do its work (but one that should be backwards compatible), so the XXX methods are effectively dead code.

If yes, there are two protoc-gen-go, one and two, which one should I choose? and should I use the latest version?

github.com/golang/protobuf/protoc-gen-go@v1.4.1 and google.golang.org/protobuf/cmd/protoc-gen-go@v1.22.0 are identical except that the former has support for grpc bindings, while the latter does not. We are in the process of trying to pass off ownership of the grpc generator over to the grpc-go project.

  • If you do not use grpc, I recommend using protoc-gen-go@v1.22.0.
  • If you use grpc, I recommend using protoc-gen-go@v1.4.1. Once cmd/protoc-gen-go-grpc: add code generator grpc/grpc-go#3453 is merged, I recommend eventually migrating to protoc-gen-go@v1.22.0 and calling the grpc-specific generator to produce grpc bindings.

@RainbowMango
Copy link
Author

In what ways was this causing a "compatibility issue"?

I tried to update github.com/golang/protobuf to v1.4.0 in Kubernetes, but found a panic. I suggest you can check the discussion here for more details.

The panic triggered by github.com/gogo/protobuf which called XXX_Size() method.

There shouldn't be a need to regenerate existing .pb.go files for things to work with v1.4.0.

The thing is there are a bunch of .pb.go files which generated by older protoc-gen-go, thus the InternalMessageInfo is exported, How can we check if the methods of InternalMessageInfo will be called?

Involve @liggitt here too, there is a huge effort to regenerate existing .pb.go files.

@dsnet
Copy link
Member

dsnet commented May 14, 2020

The panic triggered by github.com/gogo/protobuf which called XXX_Size() method.

There's the problem. The gogo/protobuf module is calling an internal method to this module that is explicitly documented as not for external use:

// This exists to support protoc-gen-go generated messages.
// The proto package will stop type-asserting to this interface in the future.
//
// DO NOT DEPEND ON THIS.

We're technically under no obligation to address this regression, but to ease migration, #1129 adds minimal functionally back to InternalMessageInfo so that panics don't happen. Can you verify that it resolves the issues you're encountering?

@RainbowMango
Copy link
Author

Can you verify that it resolves the issues you're encountering?

Yes it works! And I answered here too.

I'm looking forward to new release now.

@dsnet
Copy link
Member

dsnet commented May 14, 2020

v1.4.2 is released.

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