-
Notifications
You must be signed in to change notification settings - Fork 51
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
feat: Include protos in the Google.Api.CommonProtos package #729
Conversation
This commit separates out "update the protos" from "generate the protos". Later commits will add the protos themselves, along with handling in terms of build/packaging.
(These are the same ones as in the current release of Google.Api.CommonProtos. Regeneration creates no changes.)
Additionally, add msbuild support so that the common protos will be used implicitly by Grpc.Tools if IncludeGoogleApiCommonProtos is set to true.
@@ -0,0 +1,20 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to be called manually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. (Like generateprotos.sh is today. We do this on an ad-hoc basis.)
Will wait for other feedback before merging/releasing. |
A couple of questions:
This change might remove my need to include |
@JamesNK responses inline:
My end-to-end test was:
(I did run into some problems within that, when trying to write code using the message without having done a successful build beforehand... but I think that's just an ordering aspect, and not specific to this change.)
The repo doesn't currently contain the protos. The previous procedure would basically fetch them to a temporary directory and generate the code, but not add the protos themselves to the repo. (This is what we do for all the client libraries, too.) So the presence of the protos is new. I hope that makes sense - please let me know if it doesn't.
Right. Presumably it'll be fine to set either IncludeGoogleApiCommonProtos or IncludeHttpRuleProtos to true... we might want to recommend against doing both, as I suspect it's non-deterministic (or at least "hard to work out") which version would actually be used in that case. |
LGTM |
Hearing no further issues, I'll merge and do a release. |
(And make them easy to consume with Grpc.Tools.)
Fixes #715.
cc @JamesNK @jtattermusch @tonydnewell