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

C# Proto2 feature : Finale #5936

Merged

Conversation

ObsidianMinor
Copy link
Contributor

@ObsidianMinor ObsidianMinor commented Mar 22, 2019

This wraps up all the previous feature PRs. Changes include:

  • Many tests for previous feature PRs
  • The proto3 constraint on C# code-gen has been removed
  • Conformance tests for proto2 are ran instead of skipped
  • GetOption APIs have been added to descriptors
  • CustomOptions are now deprecated
  • The generated descriptor code has been made public

When pulled, this PR will fix numerous proto2 feature issues, including: #5759, #4591, #4407, #3487, #2991

@ObsidianMinor ObsidianMinor force-pushed the csharp/proto2-feature/finale branch 2 times, most recently from 46049aa to 3de9ba0 Compare March 24, 2019 05:13
@ObsidianMinor ObsidianMinor marked this pull request as ready for review May 6, 2019 00:25
@ObsidianMinor
Copy link
Contributor Author

We're open for review @anandolee. I'll continue to add tests in places I see lacking but it's all nearly there. Feel free to point out anywhere you feel needs more tests.

cc: @jskeet if you want to review as well

Is there anything else we need to do for proto2 support like docs?

@anandolee
Copy link
Contributor

anandolee commented May 6, 2019

Yes, we also need to update the docs as well
https://developers.google.com/protocol-buffers/docs/reference/csharp-generated?

But this can be done later after the support is released

@jskeet
Copy link
Contributor

jskeet commented May 21, 2019

I'm absolutely not going to have time to review, I'm afraid :(

@anandolee
Copy link
Contributor

@jtattermusch , can you help to review the final PR for proto2 support from C# language's view. My feedback may more focus on protobuf's view.

We didn't make the other proto2 PRs( #5759, #4591, #4407, #3487, #2991 ) usable for users, so related APIs can still be changed

@ObsidianMinor
Copy link
Contributor Author

We didn't make the other proto2 PRs( #5759, #4591, #4407, #3487, #2991 ) usable for users, so related APIs can still be changed

*those issues listed are not the other proto2 PRs. These are the other PRs (#4642, #5183, #5350)

@jtattermusch
Copy link
Contributor

I will review once I have a bit of free time.

@gonzedge
Copy link

Hey all, I'm working on a project at the moment that has a mixture of proto2 and proto3 definitions and trying (unsuccessfully) to generate C# objects. It looks like this PR would solve my problems. So a couple of questions:

  • Is this planned to be merged/released soon? How can I help?
  • Is there a way to install the binaries available on this branch locally for me to try it out on my project?
  • What do you recommend I do in the meantime? Is there something out there that supports both syntaxes?

@ObsidianMinor
Copy link
Contributor Author

  • I believe Google wants it released soon, however it needs to be reviewed first and the two people that would be best for that are busy, so I don't know when it'll be released.
  • You can checkout my fork branch csharp/proto2-feature/finale to try it out. Of course you'll have to compile protoc yourself and you may want to rebase on master.
  • In the meantime you can use another protobuf lib like mgravell/protobuf-net @gonzedge

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in still in progress of reviewing (it's a lot of code), but I have a few initial comments.

csharp/src/Google.Protobuf/MessageExtensions.cs Outdated Show resolved Hide resolved
csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs Outdated Show resolved Hide resolved
csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs Outdated Show resolved Hide resolved
@jtattermusch
Copy link
Contributor

@ObsidianMinor can you also resolve the conflict?

@jtattermusch jtattermusch merged commit 37bf540 into protocolbuffers:master Sep 3, 2019
@jtattermusch
Copy link
Contributor

I merged both this PR and the the docs #6499.

@anandolee when is the planned branch cut / release date for 3.10?

@arthur-tacca
Copy link

Any idea why this want mentioned in the 3.10.0rc1 release notes? Did it not make it on? Or is it being "secretly" released to allow for testing before it's official?

@jtattermusch
Copy link
Contributor

@rafi-kamal are #6499 and #5936 part of the v3.10.x branch? If so, we should update the release notes.

If not, seems like the PRs haven't made it on time for the v3.10.x branch cut (and they will be in the next release).

@rafi-kamal
Copy link
Contributor

Unfortunately the branch cut happened about a week ago, before the PRs were merged. It will be included in the next release (ETA: 6 weeks), unless this is something critical and requires cherry-picking.

@ObsidianMinor
Copy link
Contributor Author

Cherry-picking it should fix #6509 and #6510 from the 3.9 release but I don't know if that's critical or not.

@ravgupms
Copy link

ravgupms commented Sep 8, 2019

@ObsidianMinor given the release happened only a few days back can you cherry pick your changes into current release branch?

I'm trying to implement a GRPC External Auth service for Envoy in C# and I'm currently blocked since a number of protos which Envoy uses are in proto2 version. And I'm sure there are many like me who are waiting for this feature for quite a long time.

@ObsidianMinor
Copy link
Contributor Author

@ravgupms If I worked at Google, I might. But I don't, so I can't.

Maybe we could create a 3.11 beta branch with the cherry-picked changes and put up a 3.11 beta version of Google.Protobuf on nuget so people could test the proto2 features. Could that work? @anandolee @jtattermusch

@ravgupms
Copy link

ravgupms commented Sep 9, 2019

@ObsidianMinor that would be great. Thanks!

@jtattermusch
Copy link
Contributor

We could cherry-pick this PR and #6499 (the corresponding docs) into 3.10.x branch, but it would be a very large cherry-pick (this PR is 34 commits), so even though I understand that folks are waiting for this feature and i'd like to get it tested sooner than later, backporting such a large (non-critical) change is not something we usually do. I'm open to backporting into 3.10.x branch if @rafi-kamal is ok with that (as an exception to the normal process to facilitate early testing).

Releasing 3.11.x is not a good option because that would require cutting the 3.11.x ahead of schedule and that's not a good idea for process reasons (the branch involves all other languages too).

@rafi-kamal
Copy link
Contributor

I agree with @jtattermusch, this is a pretty large change and I would prefer to release it within our normal release schedule instead of cherry-picking it to 3.10.

@tgeng
Copy link

tgeng commented Nov 13, 2019

Thanks for the awesome work! Any idea when 3.11 will be released?

@ObsidianMinor
Copy link
Contributor Author

Next week @tgeng

@tgeng
Copy link

tgeng commented Nov 18, 2019

Thank you so much ObsidianMinor for driving this! This unblocks us and saves us from migrating some huge protos to proto3! 👍 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet