Add support for additional protoc arguments in Grpc.Tools #25374

moserware · 2021-02-08T18:48:11Z

Using "optional" presence tracking in proto3 (before protobuf 3.15)
required the --experimental_allow_proto3_optional protoc option
but there was no existing Grpc.Tools feature that would allow specifying
these arguments.

This commit adds an optional Protobuf.AdditionalProtocArguments option
that allows you to specify arbitrary protoc arguments. For example:

<Protobuf Include="**\*.proto" CompileOutputs="true" AdditionalProtocArguments="--experimental_allow_proto3_optional" />

Fixes #22975

@jtattermusch

linux-foundation-easycla · 2021-02-08T18:48:14Z

The committers are authorized under a signed CLA.

✅ Jeff Moser (6230b1d)

moserware · 2021-02-08T18:49:22Z

src/csharp/Grpc.Tools/ProtoCompile.cs

+        /// Additional options that will be passed directly to protoc.
+        /// For example, "experimental_allow_proto3_optional"
+        /// </summary>
+        public string[] AdditionalProtocOptions { get; set; }


I'm open to alternative parameter name suggestions. Also, I was unsure on if people should provide the -- prefix in the options. The current implementation automatically provides the -- prefix in order to encourage folks to not to forget it.

Am also open to making the option explicit to experimental_allow_proto3_optional such as:

public bool ExperimentalAllowProto3Optional { get; set; }

The downside is that any future flags wouldn't be supported without a code update. The upside however is that it'd be safer (i.e. you couldn't inject arbitrary options)

I think having a way of passing arbitrary command line flags is useful. In fact even having a way of passing extra command line args (not necessarily -- flags) can be useful at time.
Of course the users would be doing this at their own risk but I think when you're passing arbitrary flags to your protoc invocation, that's kind of obvious.

Also technically, "Options" is not the best term here, since in protoc terminology the "options" is what gets passed to --csharp_opt= (OutputOptions) and --grpc_opt= (GrpcOutputOptions), not arbitrary command line flags.

@jtattermusch I went ahead and renamed it to AdditionalProtocArguments and made it so that I didn't automatically prefix it.

jtattermusch · 2021-02-24T15:08:53Z

src/csharp/Grpc.Tools/ProtoCompile.cs

+            {
+                foreach (var additionalProtocOption in AdditionalProtocOptions)
+                {
+                    cmd.AddArg("--" + additionalProtocOption);


I think this automatic addition of "--" prefix is actually not very helpful as it can:

can cause confusion in terms of whether you should pass the args with or without -- while only offering very little extra convenience

it limits the generality (I can imagine in some use cases you'd want to add some extra arguments that are not necessarily cmdline flags to your protoc invocation and Grpc.Tools adding the extra -- to everything would spoil that).

I went ahead and removed the automatic prefix.

jtattermusch · 2021-02-24T15:13:42Z

src/csharp/Grpc.Tools/ProtoCompile.cs

+        /// Additional options that will be passed directly to protoc.
+        /// For example, "experimental_allow_proto3_optional"
+        /// </summary>
+        public string[] AdditionalProtocOptions { get; set; }


I think having a way of passing arbitrary command line flags is useful. In fact even having a way of passing extra command line args (not necessarily -- flags) can be useful at time.
Of course the users would be doing this at their own risk but I think when you're passing arbitrary flags to your protoc invocation, that's kind of obvious.

jtattermusch · 2021-02-24T15:24:52Z

src/csharp/Grpc.Tools/ProtoCompile.cs

+        /// Additional options that will be passed directly to protoc.
+        /// For example, "experimental_allow_proto3_optional"
+        /// </summary>
+        public string[] AdditionalProtocOptions { get; set; }


Also technically, "Options" is not the best term here, since in protoc terminology the "options" is what gets passed to --csharp_opt= (OutputOptions) and --grpc_opt= (GrpcOutputOptions), not arbitrary command line flags.

moserware · 2021-02-25T14:28:54Z

src/csharp/Grpc.Tools/ProtoCompile.cs

+        /// Additional arguments that will be passed unmodified to protoc (and before any file names).
+        /// For example, "--experimental_allow_proto3_optional"
+        /// </summary>
+        public string[] AdditionalProtocArguments { get; set; }


One concern is that this is a string array (one string per argument) rather than a single string (containing all arguments). I'm not sure if that'll make any practical difference in practice with how these get serialized to a "response" file that MSBuild creates. Regardless, I wanted to copy the style of other options that used arrays.

I think the approach you used is the correct one.

jtattermusch · 2021-02-26T10:35:51Z

src/csharp/Grpc.Tools.Tests/ProtoCompileCommandLineGeneratorTest.cs

@@ -140,6 +140,15 @@ public void GenerateGrpcWithOptions()
                        Does.Contain("--grpc_opt=baz,quux"));
        }

+        [Test]
+        public void AdditionalProtocOptions()


nit: rename the test method name to AdditionProtocArguments as well

Good catch! Fixed

jtattermusch · 2021-02-26T10:39:19Z

Adhoc run of artifacts - packages - distribtest flow: https://fusion2.corp.google.com/invocations/52aa4ab0-ec46-40e2-a573-676d2efde755/targets (I'll post the results later). This will also produce a build of Grpc.Tools nupkg @moserware will you be able to test the adhoc nuget manually (I'll provide a link) and report back whether things are working as expected?

jtattermusch

LGTM pending green tests.
Thanks for the contribution!

Using "optional" presence tracking in proto3 (before protobuf 3.15) required the `--experimental_allow_proto3_optional` protoc option but there was no existing Grpc.Tools feature that would allow specifying these arguments. This commit adds an optional `Protobuf.AdditionalProtocArguments` option that allows you to specify arbitrary protoc arguments. For example: ``` <Protobuf Include="**\*.proto" CompileOutputs="true" AdditionalProtocArguments="--experimental_allow_proto3_optional" /> ``` Fixes #22975

moserware · 2021-02-26T17:07:00Z

LGTM pending green tests.
Thanks for the contribution!

@jtattermusch I went ahead and did a manual test by having a proto3 proto with an optional in it. When I initially built it, I get the expected:

  example.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

I then added AdditionalProtocArguments="--experimental_allow_proto3_optional" and then it worked (and compiled the optional field) without issues.

As discussed, the fact that it's a string array can cause a bit of a surprise. For example, if you pass:

AdditionalProtocArguments="--descriptor_set_out=/tmp/protos.bin --experimental_allow_proto3_optional"

you'll have protoc generate a descriptor_set_out named /tmp/protos.bin --experimental_allow_proto3_optional but won't get proto3 optional support.

To pass both parameters, you'd need to use the MSbuild list separator ; as in:

AdditionalProtocArguments="--descriptor_set_out=/tmp/protos.bin;--experimental_allow_proto3_optional"

This works and enables the experimental flag as well as outputs the descriptor set to /tmp/protos.bin.

Anything else you'd like me to test?

jtattermusch · 2021-02-26T17:42:53Z

@moserware can you test with the ad-hoc build of Grpc.Tools package:
The nuget can be downloaded here: gs://grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/linux/grpc_build_packages/15809/20210226-081551/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip (not sure how you obtained a Grpc.Tools yourself, building it from scratch is a bit difficult).

jtattermusch · 2021-02-26T17:45:30Z

another distribtest build: https://fusion2.corp.google.com/invocations/926793c8-5b44-42dc-bdfe-61d74c2407cd/targets (the previous one has failed with a tool error).

jtattermusch · 2021-02-26T17:47:51Z

LGTM pending green tests.
Thanks for the contribution!

@jtattermusch I went ahead and did a manual test by having a proto3 proto with an optional in it. When I initially built it, I get the expected:
  example.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.
I then added AdditionalProtocArguments="--experimental_allow_proto3_optional" and then it worked (and compiled the optional field) without issues.

As discussed, the fact that it's a string array can cause a bit of a surprise. For example, if you pass:
AdditionalProtocArguments="--descriptor_set_out=/tmp/protos.bin --experimental_allow_proto3_optional"
you'll have protoc generate a descriptor_set_out named /tmp/protos.bin --experimental_allow_proto3_optional but won't get proto3 optional support.

To pass both parameters, you'd need to use the MSbuild list separator ; as in:
AdditionalProtocArguments="--descriptor_set_out=/tmp/protos.bin;--experimental_allow_proto3_optional" 
This works and enables the experimental flag as well as outputs the descriptor set to /tmp/protos.bin.

Anything else you'd like me to test?

the tests themselves seem sufficient, but how did you obtain a build of Grpc.Tools nuget? (it's tricky to build)

moserware · 2021-02-26T18:20:50Z

@jtattermusch I just tested it by copying gs://grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/linux/grpc_build_packages/15809/20210226-081551/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip locally and then unzipped it:

$ md5sum Grpc.Tools.2.36.0-dev202102261633.nupkg 
16d80a7c4111f85cbcf2f7816388eabd  Grpc.Tools.2.36.0-dev202102261633.nupkg

I then added the Grpc.Tools package locally. It was 2.36.0-dev202102261633

I repeated the test with a proto3 proto that had optional in it. Without the argument, I got

  example.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.

It worked with

AdditionalProtocArguments="--experimental_allow_proto3_optional"

as well as with

AdditionalProtocArguments="--experimental_allow_proto3_optional;--descriptor_set_out=/tmp/protos-adhoc.bin"

which also generated the /tmp/protos-adhoc.bin file.

jtattermusch · 2021-02-26T19:08:45Z

@jtattermusch I just tested it by copying gs://grpc-testing-kokoro-prod/test_result_public/prod/grpc/core/master/linux/grpc_build_packages/15809/20210226-081551/github/grpc/artifacts/csharp_nugets_windows_dotnetcli.zip locally and then unzipped it:
$ md5sum Grpc.Tools.2.36.0-dev202102261633.nupkg 
16d80a7c4111f85cbcf2f7816388eabd  Grpc.Tools.2.36.0-dev202102261633.nupkg
I then added the Grpc.Tools package locally. It was 2.36.0-dev202102261633

I repeated the test with a proto3 proto that had optional in it. Without the argument, I got
  example.proto: This file contains proto3 optional fields, but --experimental_allow_proto3_optional was not set.
It worked with
AdditionalProtocArguments="--experimental_allow_proto3_optional"
as well as with
AdditionalProtocArguments="--experimental_allow_proto3_optional;--descriptor_set_out=/tmp/protos-adhoc.bin"
which also generated the /tmp/protos-adhoc.bin file.

Thanks a lot for testing this!

jtattermusch · 2021-02-26T19:09:42Z

I'll merge after checking the distribtest results.

jtattermusch · 2021-02-27T05:53:21Z

Distribtest results: all C# distribtests are green except the dotnet5 distribtest, but it seems that the dotnet5 distribtest has failed for unrelated resons (nuget fetch):
https://fusion2.corp.google.com/invocations/f8afd528-495b-4635-ac8e-40a5b40fdbc1/targets/github%2Fgrpc;config=default/tests;showStatuses=3,5

odalet · 2021-03-27T17:13:51Z

Hi, I've seen that AdditionalProtocArguments is described in this document and it's great as I'd like to use it to pass --csharp_opt=file_extension=.g.cs to protoc. But it seems it did not make it yet in the latest version of Grpc.Tools (v2.36.4 as I'm writing this). I suppose it'll be part of v2.37, but any idea when this could be released?

jtattermusch · 2021-04-08T09:50:46Z

Hi, I've seen that AdditionalProtocArguments is described in this document and it's great as I'd like to use it to pass --csharp_opt=file_extension=.g.cs to protoc. But it seems it did not make it yet in the latest version of Grpc.Tools (v2.36.4 as I'm writing this). I suppose it'll be part of v2.37, but any idea when this could be released?

It's in 2.37.0-pre1 (already available) and 2.37.0 (being pushed right now).

odalet · 2021-04-08T10:09:46Z

Great! Thanks!

moserware commented Feb 8, 2021

View reviewed changes

moserware mentioned this pull request Feb 8, 2021

Use Google.Protobuf protoc 3.12 in Grpc.Tools and expose --experimental_allow_proto3_optional switch #22975

Closed

jtattermusch self-assigned this Feb 9, 2021

jtattermusch added the lang/C# label Feb 9, 2021

jtattermusch self-requested a review February 9, 2021 10:53

jtattermusch added release notes: yes Indicates if PR needs to be in release notes kokoro:run labels Feb 9, 2021

grpc-kokoro removed the kokoro:run label Feb 9, 2021

jtattermusch reviewed Feb 24, 2021

View reviewed changes

moserware changed the title ~~Add support for additional protoc options in Grpc.Tools~~ Add support for additional protoc arguments in Grpc.Tools Feb 25, 2021

moserware requested a review from jtattermusch February 25, 2021 04:49

moserware commented Feb 25, 2021

View reviewed changes

jtattermusch reviewed Feb 26, 2021

View reviewed changes

jtattermusch added the kokoro:run label Feb 26, 2021

grpc-kokoro removed the kokoro:run label Feb 26, 2021

jtattermusch approved these changes Feb 26, 2021

View reviewed changes

jtattermusch mentioned this pull request Feb 26, 2021

Improve Grpc.Tools documentation #25570

Merged

jtattermusch added the kokoro:run label Feb 26, 2021

grpc-kokoro removed the kokoro:run label Feb 26, 2021

jtattermusch merged commit e46445c into grpc:master Feb 27, 2021

jtattermusch mentioned this pull request Mar 18, 2021

Grpc.Tools: Allow passing --experimental_allow_proto3_optional (protoc flag) in <protobuf/> definition #23686

Closed

jtattermusch mentioned this pull request Apr 22, 2021

Grpc.Tools is not passing OutputOptions or AdditionalProtocArguments to protoc.exe #25950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for additional protoc arguments in Grpc.Tools #25374

Add support for additional protoc arguments in Grpc.Tools #25374

moserware commented Feb 8, 2021 •

edited

linux-foundation-easycla bot commented Feb 8, 2021 •

edited

moserware Feb 8, 2021 •

edited

moserware Feb 8, 2021

jtattermusch Feb 24, 2021

jtattermusch Feb 24, 2021

moserware Feb 25, 2021

jtattermusch Feb 24, 2021

moserware Feb 25, 2021

jtattermusch Feb 24, 2021

jtattermusch Feb 24, 2021

moserware Feb 25, 2021

jtattermusch Feb 26, 2021

jtattermusch Feb 26, 2021

moserware Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch left a comment

moserware commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

moserware commented Feb 26, 2021 •

edited

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 27, 2021

odalet commented Mar 27, 2021 •

edited

jtattermusch commented Apr 8, 2021

odalet commented Apr 8, 2021

Add support for additional protoc arguments in Grpc.Tools #25374

Add support for additional protoc arguments in Grpc.Tools #25374

Conversation

moserware commented Feb 8, 2021 • edited

linux-foundation-easycla bot commented Feb 8, 2021 • edited

moserware Feb 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented Feb 26, 2021

jtattermusch left a comment

Choose a reason for hiding this comment

moserware commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

moserware commented Feb 26, 2021 • edited

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 26, 2021

jtattermusch commented Feb 27, 2021

odalet commented Mar 27, 2021 • edited

jtattermusch commented Apr 8, 2021

odalet commented Apr 8, 2021

moserware commented Feb 8, 2021 •

edited

linux-foundation-easycla bot commented Feb 8, 2021 •

edited

moserware Feb 8, 2021 •

edited

moserware commented Feb 26, 2021 •

edited

odalet commented Mar 27, 2021 •

edited