-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
Binary compatibility broken in Google.Protobuf by introduction of Proto2 features #6379
Comments
This was my mistake, I'll fix it. To be 100% clear, reintroducing the previous method will make the next release binary-compatible, right? |
I haven't looked over all the changes - I only found that one. Ideally, you should take a copy of the public API at an earlier version, then take a copy of the public API at a later version, and diff them. Introducing a new overload and then making the currently-optional parameter required will change the compile-time behaviour (in that it will resolve to the single-parameter method rather than the two-parameter method) but that's unlikely to break anyone. The public API generator tool at https://github.com/JakeGinnivan/ApiApprover could help with this. |
Alright I ran the tool and found the FieldCodec factories are the only breaking API changes (beyond methods introduced on interfaces that should only be implemented by internal types). |
As a thought, I wonder whether we should delist 3.9.0 from NuGet for now, then release 3.9.1 with the fix when it's ready to release. That will reduce the chances of other people accidentally taking a dependency on 3.9.0 which would cause diamond dependency issues. @anandolee Are you happy to do that? |
Delisted. Thanks Sorry for late reply, just back from vacation |
After a second thought, I have a question for the compatibility. Is the compatibility break that new generated code can not compile with old C# runtime? Protobuf compatibility does not gurantee such case. We only consider old generated code not work with new runtime as breaking change |
The break is when you use the 3.9.0 runtime with old generated code. The issue was noticed when a Google Cloud Platform lib using generated code from 3.7.0 used Google.Protobuf 3.9.0 and attempted to run (it couldn't since a parameter was added to each of the FieldCodec factory methods). @anandolee |
Because we already plan to release v3.9.1 for C#, there's one more backwards compatibility item that should be fixed: |
In that case, seems we are missing a backward compatibility test? |
Ok, I was wrong, the |
@TeBoring testing backward API compatibility in an automated fashion is hard. |
https://github.com/protocolbuffers/protobuf/tree/master/csharp/compatibility_tests/v3.0.0 |
I will add compatibility test for 3.7.0 |
The 3.7.0 compatibility test is unable to catch the error. @ObsidianMinor are you sure the failure is when use 3.7.0 protoc with 3.9.0 runtime? I also noticed that in your PR, you mentioned "There was no way to generate extension identifiers at the time (before #5936) so rather than add tests for code that couldn't actually be used (unless the consumer decided to implement the interfaces and use the API manually)" Is it because users implement the interfaces and used the API manually (not the generated code)? I am wondering how to reproduce the error in compatibility test or maybe we do not want to add this to compatibility test? |
The error comes from adding the |
The test steps would need to be something like:
|
This may be the case for other types as well, but at least FieldCodec introduced binary-incompatible changes in 3.9.0, in this commit.
Each FieldCodec static factory method gained a default parameter:
became
This is a binary breaking change. Code compiled against an earlier version of Google.Protobuf fails at execution time when using the new version, as it can't find the method. We should have added overloads instead of default parameters here.
The text was updated successfully, but these errors were encountered: