Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

• User @kkm000 isn't covered by a CLA. They will need to complete the form at https://identity.linuxfoundation.org/projects/cncf

Regards,
The Linux Foundation CLA GitHub bot

@thelinuxfoundation: I signed these 4 pages of impassable legalese! Might as well have been rot13. My firstborn is yours!

Hi @jtattermusch, I am not pushing for a quicker review, especially given that probably very few people around have an experience with msbuild, and I would be surprised to find a reviewer for the VS project XAML object model integration (the new xml property file in my CL). @jskeet, can you help with this maybe? I remember discussing that with you couple years ago.

The main thing I want a kinda expedite feedback for, and you are probably the best person to give an answer, is whether do you think I am on the right track at all. I would continue working on this integration projects, by hacking protoc and grpc codegen to add options to produce predictable file names, and reworking the examples and the docs. Seems I can carve a couple hours a day these weeks to finish this! We can do similar packaging for the protoc project too.

It would be also super amazing to somehow test these build scripts. The build part can be reasonably easy be tested on Travis and Appveyor, if you are using them. What external public cloud services are you using/approving of use? I see only links to sponge, that I obviously do not see (not any more :)), VS integration is probably not easily testable tho.

Thanks!

I don't have any time to look at this at the moment. I can try to get to it next week, but I'm travelling to a conference (having been away this week) so it may be the week after before I can allocate any time to this.

@jskeet, thank you very much, appreciate it. Naturally there is no rush at all!

Xref #6085 /cc @aniongithub. Holy Guacamole, this MsBuild integration archeology uncovers quite a history! :)

@jskeet, @jtattermusch: Do you think it would be possible to allocate some time to look at this?

The current packaging scheme of gRPC is quite lacking. In fact, I do not think I have an extremely complex project, and I am crippled by the way the tools are. I have a C++ server and many C# clients. To compile a proto file with a service definition for the clients, I need, first of all, protoc (from the tools package) that is rather impossible to discover (I am just defining a property in my build file like ProtoCompiler=../../packages/Grpc.Tools.1.7.1/.... and trying not to forget to update the path with upgrades; this is a no-go for Net.SDK projects though). Second of all, I need the standard proto imports (include/google/protobuf/), and they not packaged anywhere at all, as far as I looked, so I just checked them into my project and maintain a copy :-/. The only way I allow the clients to get the interface is precompile the proto and put the assembly into a NuGet package on the internal feed. This is not a bad solution whatsoever, but I think the support for .proto files should be as simple as just dropping them into an MsBuild project, as easy as you have in a BUILD file.

I understand I am most likely preaching to the choir, but I am just corroborating that the packaging and build support does need a radical rework, and currently barely delivers. I will be more than happy to do all the work (in fact, I did most of it to support gRPC internally here). I need help from the maintainers with locating binary artifacts for multiple systems, and perhaps a certain degree of commitment so that this attempt at the solution does not sink this time. I am totally determined to support packaging and build for multiple platforms, ideally all platforms and frameworks where the gRPC runtime is supported.

I may have time to look next week, but I can't make any promises. I would point out that the standard protos are in Google.Protobuf.Tools though.

@jskeet, thanks!

As for the standard protos, thanks for the pointer. Unfortunately, they are about as discoverable by the build as the gRPC packaged tools (namely, not at all), so the amount of maintenance when using the package is about the same if not greater. It's just one directory of small files. You see, it all kinda works together in the end, but not without picking one's poison first.

Okay, having had more of a look, a few questions:

1. Dependencies

Dependency change: the package now adds a dependency on Grpc.Tools of a matching version for managed projects only

Is that dependency added as a build-only dependency? If I'm building a library, I don't want to end up with a dependency on Grpc.Tools in my resulting library or nuget package.

2. Source presence

There are two times at which I'd definitely want the generated source to be available:

• When debugging locally; I assume this is okay as they'll still be present in the obj file
• When debugging code that depends on a library that includes the protos. There are multiple ways of getting at the source code for code that you're debugging; the one I'm tending towards these days is SourceLink, but that usually expects that the source code is persisted somewhere. Would you expect the source code for the protos to be bundled into the nuget package?

3. Well-known type dependencies

I'd prefer them to only be in Google.Protobuf.Tools, personally (where they are already). With a single source of truth, there's less chance of ending up with conflicting definitions. I think it would be reasonable to have a dependency on Google.Protobuf.Tools though.

4. Protobuf-only tooling

A lot of users may well use protobuf without gRPC. Would it be feasible to have an msbuild task that enables that scenario, and somehow automatically extend it for gRPC scenarios if Grpc.Core is in the dependencies? In particular, I don't want to end up with an msbuild task that stops us from doing anything sensible with proto-only projects.

5. Options

There's currently an option for generating internal types, and in the future I expect there to be an async option. Is there a simple way for us to allow options to be set in the project file?

6. Rebuilding

Am I right in saying that msbuild will basically associate the input proto file with the output cs file, and assume that it needs to rerun protoc if and only if the proto has changed? I assume so, but I thought it worth checking.

@jskeet wrote:

Dependency change: the package now adds a dependency on Grpc.Tools of a matching version for managed projects only

Is that dependency added as a build-only dependency? If I'm building a library, I don't want to end up with a dependency on Grpc.Tools in my resulting library or nuget package.

Ugh, did I say Grpc.Tools. This is actually Grpc.Core, sorry about the confusion!

Mutatis mutandis, yes, if you build the package from proto files, you will end up with a runtime dependency on Grpc.Tools. Can you think of a scenario where that would be undesired (assuming there are some services defined, or the user is rather expected use Protobuf tools alone)? The built assembly will not work without it (and the Google.Protobuf runtime package).

Source presence
There are two times at which I'd definitely want the generated source to be available:
When debugging locally; I assume this is okay as they'll still be present in the obj file

Yes, both the debugger and IntelliSense know the files (you can hit F12 to go to the generated source).

When debugging code that depends on a library that includes the protos. There are multiple ways of getting at the source code for code that you're debugging; the one I'm tending towards these days is SourceLink, but that usually expects that the source code is persisted somewhere. Would you expect the source code for the protos to be bundled into the nuget package?

This is not what I planned for. I am not familiar with SourceLink (I'll figure out). I just tried $ dotnet pack --include-source on my little toy project, and the generated source are not inlcuded into the .symbols.nuget package (neither was the proto files). I am sure it should not be hard to tap into the packaging path in the NET.SDK scripts to include them.

A question here would be where to place them. As is, the files are generated under e.g. obj/Release/netstandard1.5, with their corresponding directories relative to the -I root. I do not want to lump them all into a single directory, as file names might conflict. Also, since the PDB records file locations, I think it is not that easy to move them even if the directory structure is preserved. Rewriting the PDB somehow? Do not know if there are tools for that.

Do you think we can leave that to the iteration 2? We can advance very far already with simple enough changes. I can look at packaging the proto with the symbol+source package with this version.

We should not generate the files under the source tree, however, due to the greediness of NET.SDK file discovery. As soon as we place them even into a subdirectory, the .csproj will discover them by default, and would yell at us for trying to include the file into the Compile collection twice (yes, I've been yelled at for that). Not that it cannot be remedied, but this should be an advanced option, well thought of. How likely is that someone will need access to the generated sources when using a source+symbols package, anyway? Indeed, your case, as the gRPC developer, is quite exceptional.

Well-known type dependencies
I'd prefer them to only be in Google.Protobuf.Tools, personally (where they are already). With a single source of truth, there's less chance of ending up with conflicting definitions. I think it would be reasonable to have a dependency on Google.Protobuf.Tools though.

Oh, no question here, of course I would prefer that!!! Can you help this happen? This would require Google.Protobuf.Tools packaging changes, very much inline with what I am doing in this CL. The main problem is the tools are not discoverable (read my "long answer" to @jtattermusch). In the ideal world, we can package the grpc codegen only, and the packages would know how to find each other and build with or without gRPC. But this requires a tight cooperation with the protobuf team. @xfxyjwf's answer to my question today made me think we have some work to do in this direction.

But I certainly do not want this project to die out again. My opinion is we should just go ahead and include the files for this version, while reworking the protoc compiler packaging too, to end up where we both, if I am understanding, want to end up.

Protobuf-only tooling
A lot of users may well use protobuf without gRPC. Would it be feasible to have an msbuild task that enables that scenario, and somehow automatically extend it for gRPC scenarios if Grpc.Core is in the dependencies? In particular, I don't want to end up with an msbuild task that stops us from doing anything sensible with proto-only projects.

I am not sure I understand the problem here (they say where the wise one does not see a solution, a fool does not see a problem, so forgive me :)).

In addition to my above comment about repackaging proto tools, one thing that is hard to tack is to know in advance whether the protonameGrpc.cs will be generated. The C++ codegen just creates a (nearly empty) file for a proto without services. I think the most sensible thing to do is to change the C# codegen to match this behavior. This will enable dependency checking in build to work correctly. As written, the script always causes the proto to recompile on every build, as the *Grpc.cs files are missing.

I could script around that, but there is no sound solution. What if someone refactors their proto files and moves the service definition? We can end up with the stale *Grpc.cs that still defines the old service, and compile errors (hopefully!).

Options
There's currently an option for generating internal types, and in the future I expect there to be an async option. Is there a simple way for us to allow options to be set in the project file?

Easy-peasy in the simplest case. See the MSBuild attributes in my answer to @jtattermusch.

But more complex cases are harder to handle. What if you compile a set of protobuf files that refer each other, and set the option to different values? I do not think we can express that in the command line. In this case, probably we can (a) produce an error or (b) take one value to trump another, (c) maybe with a warnings. Which would you go with?

Rebuilding
Am I right in saying that msbuild will basically associate the input proto file with the output cs file, and assume that it needs to rerun protoc if and only if the proto has changed? I assume so, but I thought it worth checking.

Yes. Since the .cs files are added to the Compile collection, MSBuild transitively checks their dependencies as well.

@jskeet: SourceLink looks amazing, thanks for bringing me up to date on it. Looks like a better alternative to source packages!

It seems that the files it pulls should be under source control and pushed up to the server. The generated files generally should not. This is an interesting part.

@jtattermusch, @jskeet: The question now is to use an MSBuild task or not.

@xfxyjwf suggested using the dep file generated by protoc. I like the idea, because it provides real, minimal, granular dependencies. Otherwise we need to either assume that the compile depends on all *.proto in all directories supplied by -I. This also solves the problem with file name mangling. Likely also solves the problem of missing xGrpc.cs file if x.proto has no services. And then the .sh script that guesses the Unix-like OS can go away too. This is a win all round.

However, this requires writing an MSBuild task, as scripting that inside MSBuild XML is nearly impossible. This certainly raises compatibility issues.

I played with MSBuild tasks yesterday a bit, and I can compile and package a task DLL so that it works on all versions of MsBuild and VS from 2010 on, and also with .NET.SDK on Linux (I did not yet test Mono, but it is using the same sources of MSBuild as VS, so I do not expect problems there). Essentially, the task can be packaged in two different builds: one for netstandard1.3 and another for net40. The first is below the requirements for gRPC on a machine, and the latter depends only on assemblies that come with the .NET runtime itself, not even a particular version of VS (that was my biggest concern, as these MsBuild runtime DLLs are even named differently!)

Do you have any opinion on this, pros and cons?

MSBuild task vs not: It feels like it's an upfront hit but one that would pay dividends over time. There may be various opportunities which are just a little too expensive if we have to do everything within the XML, but which would be easy from C#.

One other aspect that might be relevant: would using an MSBuild task allow you to run protoc once across all .proto files? There are probably some problematic cases there, but also potentially useful ones.

Protobuf-only tooling: I would have started with this, personally. Indeed, it may be all that's required in the long run. I'd start with a task that turns a .proto file into a .cs file, and doesn't know anything about gRPC. A second iteration would check whether the project has any gRPC dependencies, and generates the gRPC code if so. (Or even uses an explicit option for it.) What I don't want is a situation where we can't later add that task because we'd then have two tasks fighting for who gets to build things with a .proto file.

Debugging: The lack of SourceLink support would probably stop me from using this for my current work, but it's a reasonable thing to defer. (I believe it does have options for source files to be in the package though, as well as other files being in source control.)

@jskeet:

MSBuild task vs not: It feels like it's an upfront hit but one that would pay dividends over time.

Agree. So the task it is!

One other aspect that might be relevant: would using an MSBuild task allow you to run protoc once across all .proto files?

Since <Exec> is also a task, everything applies to either. Batching is the MSBuild feature, so it calls the task as many times as needed, determined by the scripts around it.

Protobuf-only tooling: I would have started with this, personally. Indeed, it may be all that's required in the long run. I'd start with a task that turns a .proto file into a .cs file, and doesn't know anything about gRPC.

Since a task (of this type, derived from ToolTask) is concerned only with running a tool, there should be only one protoc task that takes same parameters as protoc beneath it does. Specify gRPC plugin, its options and output directory, get the stubs file.

My current plan is to package the tools in one package but with separate .props and .targets files, as if they came from separate packages, one for protoc and another for gRPC plugins. This way it would be easier to transplant the protoc part into the Protobuf.Tools package. The most important part is tools cross-detection, and it will be much easier to debug in a single package (as long as nothing actually depends on their being in the single package).

Debugging: The lack of SourceLink support would probably stop me from using this for my current work, but it's a reasonable thing to defer. (I believe it does have options for source files to be in the package though, as well as other files being in source control.)

You know that tool much better than I do. I agree this is for the next iteration, probably what I have is already quite a bite to swallow. If you can tell me what needs to be done to the generated files so that they are recognized by the tool, I'll probably figure out a way to do that.

I actually know very little about the details of SourceLink, but we can discover what to do later on. (ctaggart has been very helpful in the past.)

I'm fine with there being multiple tasks in a single package. I do think it would be best of in Google.Protobuf.Tools (or even Google.Protobuf.MSBuild) from the start though. While it's fairly natural for a gRPC user to refer to protobuf tools, it's odd for a Protobuf-only user to need a gRPC package. (Again, we need to make sure that the package doesn't end up in end-project dependencies etc, but I'm assuming that's easy enough to do.)

I think you've answered all my questions at this point, so I'll leave the details to the gRPC and Protobuf teams :)

@kkm000 the plan is for the change to land in the next release v1.17.0 (branch will be cut around Nov 20 and the release will go out beginning of Dec). Until then, the package will be available in nightly builds on packages.grpc.io

Before the branch cut, we should update the documentation and do some cleanup, the example update PR should be merged after the packages are pushed to nuget.org (but it should be in ready-to-merge state before then).

@jtattermusch,

I had similar problems in the past (when still the legacy nuget), that's why the original version of Grpc.Tools.nuspec had full paths including the destination filename. I also vaguely remember the / and \ distinction was important.

Yep, I had a similar experience. I had an internal package containing a text file called DEPLOY for internal tooling, and also had a trouble putting it in place. This issue is different, I believe. I'll look even closer into this when we are done with this release. Let's wait if the NuGet guys respond.

Knowing that some versions of the SDK may silently mess with our packaging is kinda unnerving. Glad we have daily builds easy to eyeball, but an automated verification that the tools behaved sane would be helpful, too. I'll keep that on my list. I am worrying that (1) we do not accidentally release a package with messed paths and (2) someone who gets the code and builds their own (modified, presumably) version with their SDK gets a consistent result. (1) is a pretty major point, (2) is much less so. I have no idea if anyone in the wild could be RYO a modified Tools package, seems not very likely. Overall, (2) is safe to postpone until a bug report of it, IMO.

the example update PR should be merged after the packages are pushed to nuget.org

But before the version is tagged with a Git tag, right? So the users who git checkout v1.17.0 get examples pointing to the new package. Just trying to grok the release sequence. This part is a bit circular in my mind. Should I make a copy/cherrypick PR targeting the branch after it's cut in order to get examples into it, and then separately into master after the packages are shipped? Or the release branch is merged back into master, so that would happens automagically, and the examples PR should target only the 1.17 branch?

Examples are so independent from the rest of the versioning and branching of the core development, it often make sense to put them in own repo. Maybe that's our case, too.

(but it should be in ready-to-merge state before then).

The examples are in the PR #14684, ready for a review.

@kkm000 the best way is probably to create the examples PR against v1.17.x (once it exists - i.e. approx 2 weeks before the actual release where the version is git tagged with v1.17.0 and nugets are pushed), merge into the release branch and then integrate the example updates into master branch.
For now we can review #14684 against master and later close it and create a copy of that PR against v1.17.x

@jtattermusch Sure, makes the most sense. FYI, It's easy to change the target, there's not even a need to open/close a PR. The Edit button to the right of title turns target name after "Wants to merge into..." into a drop-down.

@jtattermusch @kkm000 May I ask you when does it plan to release this version (1.17) on nuget.org? Alternatively, could you make a relase of 1.17-dev, please? Thank you very much.

@jurby, as far as I can see, the branch has not been cut yet. You can pull the package from https://packages.grpc.io/ (click in the BuildId column to get to the files). I'd be grateful if you could report any problems you'd find!

Tried the dev nuget package. Seemed to work well in OS-X and Windows :-) .
The linux x86 nuget protoc segfaulted in alpine :-( (After apt add protobuf, protoc on the path worked fine but the nuget protoc still segfaulted.)

(FROM microsoft/dotnet:2.1-sdk-alpine)

Tried the dev nuget package. Seemed to work well in OS-X and Windows :-) .
The linux x86 nuget protoc segfaulted in alpine :-( (After apt add protobuf, protoc on the path worked fine but the nuget protoc still segfaulted.)

(FROM microsoft/dotnet:2.1-sdk-alpine)

If it helps any: gdb backtrace is

#0  0x000000000066790b in ?? ()
#1  0x00000000006306bb in ?? ()
#2  0x0000000000631735 in ?? ()
#3  0x00007ffff7dc4d47 in __pthread_once_full () from /lib64/ld-linux-x86-64.so.2
#4  0x0000000000001000 in ?? ()
#5  0x00007ffff7dc4cc6 in pthread_mutexattr_settype () from /lib64/ld-linux-x86-64.so.2
#6  0x000000000097f9b8 in ?? ()
#7  0x0000000000000000 in ?? ()

@Harald-Improbable can you run
ldd path/to/protoc and paste the input here?

@Harald-Improbable, @gilescope, @jtattermusch, AFAIK, Alpine is a quite a squashed distribution, designed to minimize Docker image footprint for production on disk-strained systems. If I were choosing one for development, I'd go with Ubuntu. Then you can deploy the built stuff with the Alpine image. It also might be worth a try opening the issue against google/ProtoBuffers repo.

It looks like the gRPC plugin worked--or at the least loaded correctly when the distro's protoc was used, assuming all the package defaults were used. @Harald-Improbable, do you:

• have any services declared in your proto files?
• have perchance disabled gRPC plugin by setting GrpcServices="None" on .proto files?

Also, to clarify, @jtattermusch is probably asking for the ldd output from our packaged protoc, the one in linux_x64 subdirectory of the package (the backtrace indicates you are x64, but this is probably the only thing possible to infer from it, lacking debug symbols). The file is in ~/.nuget/packages/grpc.tools/1.17.0-dev/tools/linux_x64/protoc, unless you configure NuGet in your setup differently.

FTR, see #17255 which has a similar problem for grpc_csharp_ext.

@Harald-Improbable
I got this output:

ldd /root/.nuget/packages/grpc.tools/1.15.0/tools/linux_x64/protoc
	/lib64/ld-linux-x86-64.so.2 (0x7fdcf778e000)
	libm.so.6 => /lib64/ld-linux-x86-64.so.2 (0x7fdcf778e000)
	libpthread.so.0 => /lib64/ld-linux-x86-64.so.2 (0x7fdcf778e000)
	libc.so.6 => /lib64/ld-linux-x86-64.so.2 (0x7fdcf778e000)
Error loading shared library ld-linux-x86-64.so.2: No such file or directory (needed by /root/.nuget/packages/grpc.tools/1.15.0/tools/linux_x64/protoc)

Based on that, installing libc6-compat should be enough to fix the problem: apk add libc6-compat (it should solve the missing ld-linux-x86-64.so.2 problem)

@jtattermusch We do use libc6-compat in our docker config, but unfortunately it's still dying:

FROM microsoft/dotnet:2.1-sdk-alpine
RUN apk update && apk add libc6-compat

COPY ./csharp /src
WORKDIR /src
RUN dotnet build -c Release

@kkm000 Thanks for the advice, we've followed your suggestion and went with Ubuntu

@Harald-Improbable, sure, as long as it's for build only, why bother with a few extra MB of the size. musl is not easy to combine with glibc.

FWIW, I stepped across this: https://github.com/frol/docker-alpine-glibc. Looks maintained (glibc is 2.28, the latest I believe). But this still inevitably introduces some custom bits that haven't conceivably received the same level of testing as either of the base libraries. So yeah, I'm with you, I'd better stay on the safe side.

A similar problem to what is described with alpine above seems to also be present when running under Windows Docker (Nanoserver SAC2016, microsoft/dotnet:2.2-sdk). After debugging it with the process explorer it looks like it crashes as protoc is trying to load SysWOW64 which is unavialable in Nanoserver and therefore also unavailable in all the dotnet Docker images for Windows. Is there a fix for this?

@hagen93: Uh-oh. So it looks like nanoserver does not support 32-bit executables? Let me investigate, thanks for the heads-up!

@hagen93: Thank you, that is an excellent catch! This is actually the case! https://docs.microsoft.com/windows-server/get-started/getting-started-with-nano-server#important-differences-in-nano-server, second bullet point:

• Only 64-bit applications, tools, and agents are supported.

I'll send in a fix today.

/cc @jtattermusch

@hagen93, you may temporarily work around the issue by either setting the environment variable PROTOBUF_TOOLS_CPU=x64, or passing it as property with the same value to dotnet build or dornet pack (not possible with `dotner run), e. g.,

dotnet build -v:n --no-incremental -p:PROTOBUF_TOOLS_CPU=x64

I just opened #17437 for the fix. I would really appreciate if you confirm that the new package, when available, works in this image without this workaround.

@kkm000 Sure, when and where can I find the new package? For the workaround I already built a custom .NET Core image based on Server Core instead of Nanoserver where SysWOW64 is available, but the workaround you are suggesting looks like it is quite a bit nicer than that :)

@hagen93, that's not under my control, I'm just a contributor. I hope 1.17.2, but I do not understand how minor points are released. I'll @ you when it available. You may also subscribe to watch #17437, if you do not mind extra notification fuzz. :)

Alright 😄 An @ would do nicely. I'll take the new version for a spin when I hear from you 😄

@kkm000 FYI: The workaround you suggested above does not seem to work. I tried both the env var and the msbuild property, but no luck. Just in case any one else tries to use it 😄.