Grpc-dotnet: Tooling design

[JunTaoLuo]

A few questions regarding https://github.com/grpc/grpc/pull/13207. I didn't read through all 192 comments but I did briefly read through the code after using it in a prototype.

1. .proto files in the project directory is not compiled by default

I found this to be a deliberate decision: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets#L41-L46. What was the reason for this? I don't think that comment applies here and would rather compile .proto files by default for a more frictionless experience. MSBuild properties are not very discoverable so it's best that the user doesn't have to look up the fact that you need to set EnableDefaultProtoBufItems to true to compile .proto files.

1. Design-time build

The goal here is to enable design-time build experience in VS. Currently, the *.cs files are generated when build is called, but ideally we should be generating these files when we detect *.proto files are being edited. We can enable this via Single File Generators and using the FastUpToDateCheck mechanism in VS. I see some code to this effect that I don't see running https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets#L370-L383 as well as code that needs to be modified https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets#L250. The stretch goal here may be to write a VSCode extension to enable the same design time experience.

1. Protodep

Investigate if this file can be removed. Currently it seems like it's used to communicate which files to generate (Foo.cs and FooGrpc.cs). It may be possible to substitute with MSBuild ItemGroups which is more natural and would reduce the files that are added to the /obj folder

cc @jtattermusch @shirhatti

[jtattermusch]

CC @kkm000

[JunTaoLuo]

1. Service Reference

We should also explore the service reference pattern for client generation. An example is shown at https://github.com/glennc/clientgen/blob/master/ConsoleClient/ConsoleClient.csproj#L16-L19.

[kkm000]

Thanks for the suggestions!

1 .proto files in the project directory is not compiled by default
I found this to be a deliberate decision: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets#L41-L46. What was the reason for this?

The reason is explained in the comment that you are referring to: According to the docs linked to from the comment, the current recommendation is against globbing. You can always drop a DirectoryBuild.props setting EnableDefaultProtoBufItems to true. In reality, this convenience really bites the user sooner or later, e. g. when creating a copy of a file in project, so that the recommendation sounds entirely sensible to me. Dropping a source file into the project directory (or a sub-sub-directory!) should better not dehermetize the build. You can also find the relevant discussion in the comments.

1. Design-time build
   The goal here is to enable design-time build experience in VS. Currently, the *.cs files are generated when build is called, but ideally we should be generating these files when we detect *.proto files are being edited.

Codegen is expensive; it takes creating external processes, and in fact more than one (protoc and the plugin). Maybe not everybody would want these processes be invoked continuously as they type. And this would also need VS integration way beyond a NuGet package. I certainly understand it would be cool if one would add a field in a proto file, and Intellisense magically recognized a new member on a C# class w/o recompiling, but this sounds like a hefty project on its own.

Single File Generators

That's a different animal, requiring a Visual Studio extension to function, if I understand. Speaking of it, there are much more possibilities, such as syntax highlighting, IntelliSense for .proto files, and so on, you name it. I have seen a couple extensions in the wild, but not one really impressed me enough not to uninstall it. :) That was a couple years ago, though. Maybe new/improved ones exist somewhere, or maybe I did not look hard enough.

FastUpToDateCheck

The tooling allows VS to checks if any .proto files have been in fact touched w.r.t build output by way of SourceFilesProjectOutputGroup, and does not start a build unnecessarily. Did you find any issues with this? If so, a repro would be helpful!

Or am I not understanding? The reference you gave is only to a property that may be used to disable this check.

1. Protodep
   Investigate if this file can be removed. Currently it seems like it's used to communicate which files to generate (Foo.cs and FooGrpc.cs).

This is used to track dependencies, i. e. imports, of .proto files (both in project and external). These files are created by protoc and read by tasks to check dependencies. This is akin to what tracker does in C++ tooling (.tlog files), except it is neither x-plat nor open source or redistributable, so we use a protoc facility to arrive at essentially same behavior.

1. Service Reference

This I am entirely unaware of! What, in your opinion, it would buy us?

[kkm000]

@jtattermusch, @JunTaoLuo, what is the scope of this project? 'fraid my answers above make not much sense out of context. I actually just realized I'm not commenting in the main code repo!

[kkm000]

Regarding (1), the starting comment that touches on the globbing is https://github.com/grpc/grpc/pull/13207#issuecomment-363265418

In summary, @jskeet expressed a mild preference for not globbing, and mine was (and, speaking of general tooling, still is) rather strong for same. I could not find any other inputs into this decision.

[JunTaoLuo]

Thanks for the feedback!

In reality, this convenience really bites the user sooner or later, e. g. when creating a copy of a file in project, so that the recommendation sounds entirely sensible to me.

Yes let's file an issue to track this work. I think this is valuable.

Codegen is expensive ... this sounds like a hefty project on its own

I was able to have design time compilation enabled with just a few MSBuild properties, see https://github.com/JunTaoLuo/GrpcSandbox/commit/7997a2883f7d6a826a14aa40870abdabab3d6522 which can all be done via the msbuild targets in Grpc.Tooling. I don't know the state two years ago but several new tooling mechanisms were added for other aspnet projects that now makes this extremely simple. In terms of performance what would be considered expensive? I was seeing compilation completing on the order of 0.5 seconds, and taking 0 seconds if there are no changes to the .proto file. FYI, these builds are triggered on file save.

Granted this is a very simple project with only a few small proto files.

The tooling allows VS to checks if any .proto files have been in fact touched w.r.t build output by way of SourceFilesProjectOutputGroup, and does not start a build unnecessarily.

I see. I saw the section mentioning design time build and supposed that it was there to support what I described in the previous section. In terms of what you described, it works as intended. However, I'm not sure this is the correct mechanism for that. We should instead rely on the Input and Output items of the Compile target. That is what we usually use to keep track of what has updated and what to build. In this regard, I see opportunity to simplify some of these targets.

These files are created by protoc and read by tasks to check dependencies.

I think we can simplify this with msbuild as well. I'll investigate a little further.

Service reference

@shirhatti care to elaborate?

[kkm000]

Aha!!! That's a great feature, no VSIX required! I've never seen the Generator metadata before, it must be fairly new? I noticed that ServiceProjectReference's Generator attribute is read by ASP MVC tooling tasks, starting with .NET 2.2 (looks like it was checked in less than a month ago).

As for the target in the attribute, should the whole project be recompiled, as in MSBuild:Compile? Or is it enough to run the generators, and then changed .cs files will be automagically reparsed, as long as they are in the Compile list (or some other collection)?

All in all, it's not easy to me to understand what can be changed not knowing what this new project is all about.

In terms of performance what would be considered expensive?

Personally, I think it is entirely fine to do this in a small client project with just a couple .proto files. 500 ms on file save is pretty much unnoticeable, especially given VS is running it entirely in background. (And my own project is close to a thousand of C++ files, and a C# client library :) ).

There were concerns in the original two hundred comment discussion about the optimality of the build when compiling a large tree of .proto files. I'm only a contributor of this feature, in the end, the decision is upon the gRPC team. My answers above reflect the consensus that we came to while we discussed design of these tools.

These files are created by protoc and read by tasks to check dependencies.

I think we can simplify this with msbuild as well. I'll investigate a little further.

That would be indeed interesting. Please keep in mind that the tools package as it currently stands is targeting a multitude of scenarios, potentially with, I dunno, tens or hundreds of proto files with import dependencies between them, and also future .vcxproj support. I had to go a long way to avoid regenerating source files if .protos were not changed, and at the same time track dependencies so that if a dependent .proto changes, even one entirely outside of the project, the files that import the changed file (and those that import them too) are recompiled. Excessive regenerating may trigger unnecessary C++ recompilations, and now that is something certainly undesirable.

I do not know if there is any new development for MSBuild akin to tracker and the TrackingToolTask (if I spell the name right; it's part of C++ tooling, not open sourced) that would report the lists of files in fact read and written by process it spawns (and its child processes -- I am not sure even if tracker.exe is capable of this). That could work as a replacement; but then you would get .tlog files instead of .protodep, so it does not seem a big gain on the surface of it. Tracker does allow for a better inference of dependencies in multi-file compilations though, when tracked tool is called for multiple files in a single command. protoc is capable of this.

[chwarr]

1 .proto files in the project directory is not compiled by default
I found this to be a deliberate decision: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets#L41-L46. What was the reason for this?

The reason is explained in the comment that you are referring to: According to the docs linked to from the comment, the current recommendation is against globbing. You can always drop a DirectoryBuild.props setting EnableDefaultProtoBufItems to true. In reality, this convenience really bites the user sooner or later, e. g. when creating a copy of a file in project, so that the recommendation sounds entirely sensible to me. Dropping a source file into the project directory (or a sub-sub-directory!) should better not dehermetize the build. You can also find the relevant discussion in the comments.

I checked with the .NET Core tooling team. The recommendation mentioned (see below for its text) was written to help people migrate from project.json to the .NET Core MSBuild project style. project.json files typically had explicit globs in them. When such a project was automatically migrated, the genrated .csproj included a bunch of explicit item globs as well, like

<Compile Include="**\*.cs" />

Since .NET Core MSBuild projects automatically compile all .cs files under the project directory by default, this can result the "Duplicate Compile items were included." error. The recommendation was attempting to say "To resolve the 'Duplicate Compile items were included.' error, either remove the explicit <Compile Include="**\*.cs" /> glob or disable EnableDefaultCompileItems.

It was not recommending that EnableDefaultCompileItems be disabled and Compile items be individually listed as a general practice.

I will try to get an explicit recommendation from the .NET Core tooling team about whether custom items should automatically "do their thing" or not.

Since this was confusing, I've removed the recommendation, as the previous section outlines the two ways to fix the problem.

For posterity, the recommendation that kkm000 was referring to read

With csproj, we recommend that you remove the default globs from your project and only add file paths with globs for those artifacts that your app/library needs for various scenarios (for example, runtime and NuGet packaging).

[kkm000]

@chwarr, thanks, a word from the Core tooling team would be helpful. And indeed it would be super valuable to know what they've learned about the real world use of this feature from telemetry, if such analysis has been performed.

@jtattermusch, if our globbing-by-default behavior is going to change, I'd rather do it before the first version of the tools is GA. Breaking users afterward would be really bad IMO.

Pragmatically, now is perhaps the latest moment this decision can be made without a significant fallout.

Technically, that's flipping one false to true (this is also gated by EnableDefaultItems, which, when not set at all, turns globbing off unconditionally in a classic project). Examples, the readme and the pending blog post must also be updated. I can take care of this quickly if need be.

Personally I am not a huge fan of globs in makefiles, much less of those implicit: easy make, easy break. But ultimately it's the gRPC team's decision. On the occasion of the recommendation having been revoked from docs today, I'm changing my "strongly against" to join @jskeet in his "mildly against" preference, if mine has any comparative weight at all. :)

To give you more of factual context:

• In SDK-style tooling, not everything follows the C#/VB globbing behavior; F# projects, where order of files in compilation is important, have globbing off. But we can be compiled only in C# projects, where globbing is on default.
• C++, which is another near future target, is classic-only, where glob-hogging is not provided as an MSBuild boolean property option. That might change by the time we get there, but I am not aware of any such plans.
• C# and C++ currently exhaust the intersection of (MS-tooled ∩ gRPC-generated) languages.

[chwarr]

The guidance I received from @dsplaisted was:

I would generally suggest following the pattern that the .NET SDK does and globbing by default. There could be exceptions to this for various reasons, for example if the file extension isn’t enough to infer the item type (ie if it was “.txt”), then you might not want to glob.

It could be possible to change the default for the gRPC package without breaking in the common case. To do this, the package could include a target to deduplicate the ProtoBuf items, possibly generating a warning or a message specifying that they can be removed from the project. The .NET SDK does similar checks in some cases.

[kkm000]

@chwarr, thank you for the info!

It could be possible to change the default for the gRPC package without breaking in the common case. To do this, the package could include a target to deduplicate the ProtoBuf items[...]

FWIW (if this is related to my point of breaking users after a GA), my concern was breaking of the build semantics. This statement only deals with potential problems caused by duplicate proto items in project, which is an implementation detail. What I mean by the potentially breaking scenario is, if the default changes after a GA, the tooling package suddenly starts globbing all .proto files when it previously did not. In other words, by "breaking" I refer chiefly to a possible unexpected discontinuity in user's experience.

[jtattermusch]

I'd be totally happy with globbing by default (and thus following the recommendation), but as @kkm000
mentioned, the challenge is that many users are already including Grpc.Tools in their projects (the old version of it, which doesn't provide any automatic behavior, it just provides the protoc binaries that users are responsible for running via e.g. a script), so if we suddenly start providing automatic codegen of .proto files, that can break people's projects. But if disabling the automatic codegen is as easy as adding a single tag into the .csproj file (and we announce it in advance), it think it might be ok to go with globbing by default, because automatic codegen is a great improvement in developer experience and most users will be happy about this change.

Another option is to provide a new nuget package (other name than Grpc.Tools), which will provide the automatic codegen by default and we deprecate Grpc.Tools (but I'm not sure if this is necessary).

[dsplaisted]

@jtattermusch I think you can avoid breaking existing projects in the common case by detecting and ignoring duplicate ProtoBuf items (ie my suggestion from here

[jtattermusch]

@dsplaisted I think you are talking about a slightly different situation: You are proposing a solution for deduplicating tags in the .csproj (which solves the problem when transitioning from non-globbing to globbing behavior) and I'm talking about backward compatibility for project that are already using Grpc.Tools just as a carrier for protoc.exe and grpc_csharp_plugin.exe binaries and they have their own shell scripts to generate C# code from .proto file by executing protoc directly. For such projects, detecting duplicate items is not helpful as their builds will be broken after upgrading to a Grpc.Tools package that does automatic codegen (they will have two definitions on of the same generate class - one from the script-generated .cs file and the other one by the automated .cs generator).

[markdchurchill]

I'd like to throw in here that the current <Protobuf Include="..." GrpcServices="Server,Client" /> would be improved for me by adding the message types as a separate option, to alllow generation of proto messages into shared projects. Perhaps extending this enum to:

Messages, BareServer, BareClient, Server = Messages | BareServer, Client = Messages | BareClient