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

feat: Include protos in the Google.Api.CommonProtos package #729

Merged
merged 3 commits into from
Oct 11, 2023

Conversation

jskeet
Copy link
Collaborator

@jskeet jskeet commented Oct 5, 2023

(And make them easy to consume with Grpc.Tools.)

Fixes #715.

cc @JamesNK @jtattermusch @tonydnewell

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.
@jskeet jskeet requested a review from a team as a code owner October 5, 2023 16:11
@@ -0,0 +1,20 @@
#!/bin/bash
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.)

@jskeet
Copy link
Collaborator Author

jskeet commented Oct 5, 2023

Will wait for other feedback before merging/releasing.

@JamesNK
Copy link
Contributor

JamesNK commented Oct 5, 2023

A couple of questions:

  • Have you tested this end-to-end?
  • The PR adds proto files to the repo. Are these a copy? Wouldn't the proto files already exist somewhere in the repo for tooling to generate code from? If so, I wonder if <None Include="protos\**\*.proto" Pack="true" PackagePath="content/protos" /> could be modified to use LinkBase to reference existing files and avoid duplicate.

This change might remove my need to include http.proto in Microsoft.AspNetCore.Grpc.JsonTranscoding. Someone can set the flag to get Google.Api.CommonProtos to include proto files instead.

@jskeet
Copy link
Collaborator Author

jskeet commented Oct 6, 2023

@JamesNK responses inline:

  • Have you tested this end-to-end?

My end-to-end test was:

  • Generate the package in a local repository
  • Create a new console app, and add a dependency on Grpc.Tools and the test version of the package
  • Create a proto that uses ClientLibrarySettings within a message
  • Add a Protobuf msbuild element for the proto
  • Observe that it fails to build
  • Add the msbuild property (IncludeGoogleApiCommonProtos) and observe that this fixes the build failure (and logs the location)
  • Write code to populate create an instance of the message and populate the ClientLibrarySettings
  • Build the code: all is well

(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 PR adds proto files to the repo. Are these a copy? Wouldn't the proto files already exist somewhere in the repo for tooling to generate code from? If so, I wonder if <None Include="protos\**\*.proto" Pack="true" PackagePath="content/protos" /> could be modified to use LinkBase to reference existing files and avoid duplicate.

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.

This change might remove my need to include http.proto in Microsoft.AspNetCore.Grpc.JsonTranscoding. Someone can set the flag to get Google.Api.CommonProtos to include proto files instead.

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.

@tonydnewell
Copy link
Contributor

LGTM

@jskeet
Copy link
Collaborator Author

jskeet commented Oct 11, 2023

Hearing no further issues, I'll merge and do a release.

@jskeet jskeet merged commit 4eb8a70 into googleapis:main Oct 11, 2023
4 checks passed
@jskeet jskeet deleted the pack-protos branch February 9, 2024 08:14
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.

Add proto files to Google.Api.CommonProtos
4 participants