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

Split gRPC tools NuGet into Google.Protobuf.Tools and Grpc.Tools #5957

Closed
kkm000 opened this issue Mar 27, 2019 · 9 comments
Closed

Split gRPC tools NuGet into Google.Protobuf.Tools and Grpc.Tools #5957

kkm000 opened this issue Mar 27, 2019 · 9 comments

Comments

@kkm000
Copy link

kkm000 commented Mar 27, 2019

This is first of all a discussion, and also an action item (for me, apparently) if/when agreed to proceed.

Protobuf team, @jtattermusch, what are you thoughts on this?

What

Split package so each part goes with its own project?

Currently, and intentionally by design, there are two separate parts packaged into one nuget now. One is protoc-only tooling, named Google.Protobuf.Tools. Another is gRPC-only tooling, which can be installed separately (of course, it would depend on Google.Protobuf.Tools). For an example, please see these two subdirectories: https://github.com/grpc/grpc/tree/59ffdd99/src/csharp/Grpc.Tools/build/_protobuf and https://github.com/grpc/grpc/tree/59ffdd99/src/csharp/Grpc.Tools/build/_grpc. The current package control files are only gluing the parts together:
https://github.com/grpc/grpc/blob/59ffdd9929667d8d662d4bede6a21d4b3e35a93a/src/csharp/Grpc.Tools/build/Grpc.Tools.props#L7-L10

The C# code and the build DLLs belongs to the Protobuf part entirely; the Grpc.Tools part is only MSBuild scripting. The binaries (executable files) for the compiler and plugins could be packaged each separately. Standard include protos should go with the Protobuf.Tools.

The parts of future separate packages are designed so that the could discover each other, regardless of the order of inclusion. Also, there is a counter, (currently unused, set to 1), indicating the compatibility between packages, like an "API version" exposed by the main, Protobuf part, which allows for some discrepancy between package versions. But this is mainly for 3rd party plugins, I believe.

So, it looks like everything is nearly ready for the surgery.

When

At this point we have been through multiple GA releases of the Grpc.Tools package. At this point, we have one open issue that I really want to spend some time thinking how to best take care of (namely grpc/grpc#17672), but I believe we are feature-complete otherwise.

We should not delay this too much, as there are a few packages for protoc tooling exist in the wild. I (humble as I am, of course) believe that our version is superior to these in its dependency tracking. For me, mainly a C++ user, an extra regen of header files results in an XKCD 303, naturally.

How

This is the hardest part, or at the least it looks like that from this side of the screen. Apparently, this requires a tight coordination between the two teams (and I am an outsider, so it's not even internal only).

Also, we need to ensure that there is no gap such that user's packages would break for the lack of necessary files. For example, we ship current content of Grpc.Tools, when Protobuf.Tools already carries them too. After a version tick or two, those extra copies in Grpc.Tools may be dropped.

NuGet supports version ranges for dependencies, namely an exact version or version range for packages. I'm sure I could come up with a transition schema such that no users in any combination of current/upgrade version will be broken.

Other thoughts

Implications on the joint dotnet-grpc project?
/cc @JunTaoLuo

More information


/cc @jskeet Since he was part of the original discussion.

Supercedes: X-Ref: #1193

@jtattermusch jtattermusch changed the title Split gRPC tools NuGet into Google.Protobuf.Tools and Grpc.Cools Split gRPC tools NuGet into Google.Protobuf.Tools and Grpc.Tools Mar 27, 2019
@JunTaoLuo
Copy link

For grpc-dotnet, we'd like to ensure this splitting of the packages is introduced in a backwards-compatible way. For example, project depending on Grpc.Tools right now should still get all the correct assets to build the project when they update the version. Since Grpc.Tools takes a dependency on Google.Protobuf.Tools I think this will be the case so it's fine.

We also need to ensure marking Grpc.Tools with PrivateAssets ensures both its and Google.Protobuf.Tools' assets are build time only and does not flow to the parent project. I believe this should be the case but we'd want to double check when making the change.

@kkm000
Copy link
Author

kkm000 commented Mar 27, 2019

@JunTaoLuo yes, there were a lot of issues with NuGet regarding the transitivity of asset restrictions. I vaguely remember some of them were marked fixed some time ago, but we definitely need to check carefully.

@kkm000
Copy link
Author

kkm000 commented Mar 30, 2019

Grpc.Cools! What a cool typo I made!

@tonydnewell
Copy link
Contributor

@jtattermusch @jskeet @JamesNK

I’m revisiting this issue to see if it is still relevant or desirable or needed.

Four years on and Grpc.Tools is stable and is now widely used, is a dependency on many NuGet packages (including Grpc.AspNetCore), and referenced from Microsoft documentation.

Grpc.Tools is a ‘superset’ of what is provided by Google.Protobuf.Tools and can be configured to compile .proto files without generating any gRPC code if needed. There are currently no dependencies between these two packages.

Arguments for splitting Grpc.Tools and moving build logic into Google.Protobuf.Tools include:

Counter-arguments are:

  • If it ain’t broke, don’t fix it
  • The grpc/grpc project has to build the protoc binaries anyway since it has a dependency on those to build the C# gRPC plugin
  • It guarantees the build logic, protoc binaries and plugin binaries are all compatible with each other when changes are made
  • The build logic needs to know about the plugins (configuration and know the likely files generated by it)
  • Risks in breaking existing projects

I think there is more of an argument for moving Grpc.Tools from grpc/grpc to the grpc/grpc-dotnet project since that is now the focus for gRPC C# work. However that is non-trivial since we probably will still want the binaries built by grpc/grpc.

Do we want to still proceed with trying to split Grpc.Tools and move things into Google.Protobuf.Tools?

Are there any other interested parties that need to be involved in the discussion?

@jskeet
Copy link
Contributor

jskeet commented May 10, 2023

One benefit of moving the build logic into Google.Protobuf.Tools is that when there's a new protobuf release, the new generator becomes available immediately via Google.Protobuf.Tools, rather than having to wait for the release cycle of Grpc.Tools. That's relatively minor, but it's a more practical reason than some of the other "pros". (The compatibility guarantees provided by it being in Grpc.Tools are pretty compelling though...)

@JamesNK
Copy link
Contributor

JamesNK commented May 10, 2023

I haven't seen people ask for this. But I focus on gRPC rather than independent Protobuf serialization, so that's not surprising.

@tonydnewell
Copy link
Contributor

You can tell Grpc.Tools to use other binaries instead of those provided in the package (see BUILD-INTEGRATION.md), and this nasty hack works if you want to use the protoc from Google.Protobuf.Tools by adding this to the .csproj file:

  <PropertyGroup>
    <Protobuf_ProtocFullPath>$(PkgGoogle_Protobuf_Tools)\tools\windows_x64\protoc.exe</Protobuf_ProtocFullPath>
  </PropertyGroup>

Not nice - but possible.

@tonydnewell
Copy link
Contributor

Further notes for myself:

A better way of using binaries in Google.Protobuf.Tools in Grpc.Tools is to add a custom target in the .csproj file:

  <Target Name="DefineProtocBinary" BeforeTargets="BeforeBuild">
    <PropertyGroup>
       <Protobuf_ProtocFullPath>$(protoc_windows64)</Protobuf_ProtocFullPath>
    </PropertyGroup>
  </Target>

The PropertyGroup needs to be in a target rather than at the top level of the project file is because Google.Protobuf.Tools defines its properties in a .targets file instead of a .props file (why?) - the targets file is automatically included at the end of the project file thus the properties are not defined when we want to use them. If they had been in a .props file then they would have been included at the start of the project file.

@jtattermusch
Copy link
Contributor

Thanks for a great pros/cons summary in #5957 (comment).

A few comments:

  • From the perspective of project separation it would be cleaner to rely on the protoc executables produced by the protobuf team (as part of protobuf release), but since gRPC needs to build the protoc plugin binaries anyway, we might as well build protoc binaries while at it (it's really a one line change for us since the dependencies are almost identical). If gRPC builds both protoc binaries and protoc plugins, they'll be guaranteed to be mutually compatible and would have the same range of supported platforms (since they were produced with the same build configuration), which is generally desirable.
  • Currently Google.Protobuf.Tools is "dumb" in the sense it doesn't have almost any build integration logic, while Grpc.Tools is actually quite complex and does a lot. It took us a lot of effort to make Grpc.Tools to do everything it does and to test it and fix the bugs (and msbuild is pain to write and test). So while we could argue that some logic from Grpc.Tools logically belongs to Google.Protobuf.Tools, it would be a big undertaking to move it there and to test everything to make sure we're not breaking anything. Also, the Google.Protobuf.Tools would get much more complex as a result (requiring not only new logic but also some notrivial testing) and since the gRPC team (that has most of expertise and mileage in this area) doesn't own Google.Protobuf.Tools it would be tricky transfer the ownership.
  • Ad needing to wait for gRPC release to get Grpc.Tools with new protoc binaries: That's a good point but gRPC generally tries to follow a 6weekly release cycle so this is hopefully not that much of an extra wait. We're also trying to get better at upgrading our third_party/protobuf dependency quickly. Also, as @tonydnewell has pointed out, you can override the protoc binaries being used by Grpc.Tools quite easily, so using older Grpc.Tools with the freshly released protoc binaries is possibly.

So IMHO splitting Grpc.Tools logic between Google.Protobuf.Tools and Grpc.Tools wouldn't really be worth the effort and we'll be better off with Grpc.Tools being used for pure-protobuf codegen as well as grpc+protobuf codegen (and investing the resources we have into making Grpc.Tools improvements/fixes rather than in this complex refactoring).

Thanks everyone for their inputs, I will close this issue now.

@tonydnewell can you please go through the Grpc.Tools sources and find places where we mention splitting the logic (file layout, todos, comments etc.) and perform a cleanup - since we just decided that we're not going to pursue this?

jtattermusch pushed a commit to grpc/grpc that referenced this issue Jul 17, 2023
It has been decided that we will no longer try and split Grpc.Tools
NuGet package logic into two packages by adding logic to
Google.Protobuf.Tools. See
protocolbuffers/protobuf#5957

This PR removes some of the TODOs and redundant files.

There is further simplification work that could be done to combine the
props and targets files that were already split. But this is the first
step in the tidying up the code.
mario-vimal pushed a commit to mario-vimal/grpc that referenced this issue Jul 27, 2023
It has been decided that we will no longer try and split Grpc.Tools
NuGet package logic into two packages by adding logic to
Google.Protobuf.Tools. See
protocolbuffers/protobuf#5957

This PR removes some of the TODOs and redundant files.

There is further simplification work that could be done to combine the
props and targets files that were already split. But this is the first
step in the tidying up the code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants