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

gRPC JSON transcoding - msbuild includes proto dependencies #44999

Merged
merged 2 commits into from
Nov 29, 2022

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 10, 2022

Fixes #43376

This PR changes the gRPC JSON transcoding package to:

  • Include the required proto dependencies in the NuGet package.
  • Adds Microsoft.AspNetCore.Grpc.JsonTranscoding.targets to the NuGet package. It automatically add proto dependencies to Grpc.Tooling protoc compilation.
  • Adds a switch <IncludeHttpRuleProtos>true</IncludeHttpRuleProtos> to control adding dependencies. Defaults to false.

This change eliminates the biggest obstacle to setting up gRPC JSON transcoding: acquiring the proto files and placing them in the right location. Now a dev just sets IncludeHttpRuleProtos to true in the csproj 🥳 🎈 🎉

@danmoseley The proto files are under Apache license to Google. I think the entries in https://github.com/dotnet/aspnetcore/blob/main/src/Grpc/THIRD-PARTY-NOTICES covers that license already. Is there other reaction to third-party code that needs to happen?

@JamesNK JamesNK added the area-grpc Includes: GRPC wire-up, templates label Nov 10, 2022
@JamesNK
Copy link
Member Author

JamesNK commented Nov 10, 2022

@jtattermusch This is what I discussed in our sync. It works well.

@bemafred
Copy link

@JamesNK awesome!

@JamesNK JamesNK force-pushed the jamesnk/grpcjsontranscoding-includeprotos branch from 1619af4 to 31b3af5 Compare November 29, 2022 03:31
@danmoseley
Copy link
Member

@danmoseley The proto files are under Apache license to Google. I think the entries in https://github.com/dotnet/aspnetcore/blob/main/src/Grpc/THIRD-PARTY-NOTICES covers that license already. Is there other reaction to third-party code that needs to happen?

Answered offline, but for others -- we can freely use third party code that is permissively licensed (at least Apache/MIT) but they must appear in a TPN and that TPN must end up packaged in the relevant packages/installers. I know there is some debt wrt the installer/shared frameworks (dotnet/runtime#74779) but our individual packages should be inspected to ensure their TPN goes in.

@JamesNK JamesNK force-pushed the jamesnk/grpcjsontranscoding-includeprotos branch from 31b3af5 to 0f0f714 Compare November 29, 2022 03:47
@JamesNK JamesNK merged commit de1541a into main Nov 29, 2022
@JamesNK JamesNK deleted the jamesnk/grpcjsontranscoding-includeprotos branch November 29, 2022 06:17
@ghost ghost added this to the 8.0-preview1 milestone Nov 29, 2022
@zidad
Copy link

zidad commented Jan 24, 2023

This is excellent! Can it be back-ported to 7.0 so we don't have to wait until .NET 8.0? ;p

@ghost
Copy link

ghost commented Jan 24, 2023

Hi @zidad. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@tobiashoeft
Copy link

Any news to downgrade it to .net 6.0 ?

Or how do i do it in .net 6 try so much ways, but it doesnt work at all

@ghost
Copy link

ghost commented Feb 16, 2023

Hi @tobiashoeft. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@zidad
Copy link

zidad commented Feb 16, 2023

@tobiashoeft Yeah I also tried to back port this feature to our own .net 7.0 nuget package but didn't manage to get it working unfortunately.

@ghost
Copy link

ghost commented Feb 16, 2023

Hi @zidad. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-grpc Includes: GRPC wire-up, templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grpc.JsonTranscoding requires manually copying annotations.proto and http.proto into projects
6 participants