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.Tools is not passing OutputOptions or AdditionalProtocArguments to protoc.exe #25950

Closed
StingyJack opened this issue Apr 11, 2021 · 10 comments

Comments

@StingyJack
Copy link

What version of gRPC and what language are you using?

Grpc.Tools 2.36.4

What operating system (Linux, Windows,...) and version?

Windows 10 + Linux (current, idk)

What runtime / compiler are you using (e.g. python version or version of gcc)

Whatever Grpc.Tools uses

What did you do?

I have included 3 proto files in a VS project. Two are compiled and are simple message payloads. The third is linked from here and compiled by another proj file.

 <ItemGroup>
    <Protobuf Include="Protos\example_data_payloads.proto" GrpcServices="None" ProtoCompile="true" OutputOptions="csharp_opt=file_extension=.g.cs"/>
    <Protobuf Include="Protos\example_data_schema.proto" GrpcServices="None" ProtoCompile="true" OutputOptions="file_extension=.g.cs"/>
    <Protobuf Include="Protos\example_data_stack.proto" GrpcServices="None" ProtoCompile="false" />
  </ItemGroup>

For the two that are proto compiled in this project, I am trying to fix the file extension to note that the file has been code generated so it can be excluded from test coverage reports. OutputOptions is described here and I've tried a few variations of it but the file is still generated as a .cs file.

What did you expect to see?

the code file generated with a .g.cs extension

What did you see instead?

just the file with a .cs extension in the obj/ directory

The proto file for the first item is

syntax = "proto3";
option csharp_namespace = "Core.Schema";
package example_data_schema;

message ExampleData {
	bytes id = 1;
	string name  = 3;
	int64 code = 5;
	string description = 7;
}

the code is otherwise generated correctly. I would really prefer the exclusion mechanism to be part of the compiled output, like a [GeneratedCode] attribute, since analyzers may be working from the compiled output and not using source analysis. The current conventions used by the proto generation are not any of the common ways to designate code thats been generated.

@jtattermusch
Copy link
Contributor

related #25930

@StingyJack StingyJack changed the title Grpc.Tools is ignoring OutputOptions when compiling proto files Grpc.Tools is not passing OutputOptions or AdditionalProtocArguments to protoc.exe Apr 16, 2021
@StingyJack
Copy link
Author

Neither AdditionalProtocArguments or OutputOptions are being passed along to protoc.exe.

I updated my proj file to

    <Protobuf Include="Protos\example_data_payloads.proto" GrpcServices="None" ProtoCompile="true" AdditionalProtocArguments="--csharp_opt=file_extension=.g.cs"/>
    <Protobuf Include="Protos\example_data_schema.proto" GrpcServices="None" ProtoCompile="true" OutputOptions="csharp_opt=file_extension=.g.cs"/>

and the proj build log (set to detailed) search results for "protoc.exe" have this... (line breaks added to readability)

 C:\Users\Andrew.Stanton\.nuget\packages\grpc.tools\2.36.4\tools\windows_x86\protoc.exe 
						--csharp_out=obj\Debug\netcoreapp3.1\Protos 
						--proto_path=C:\Users\Andrew.Stanton\.nuget\packages\grpc.tools\2.36.4\build\native\include 
						--proto_path=. 
						--dependency_out=obj\Debug\netcoreapp3.1\12fbc7ffe642ca7e_example_data_payloads.protodep 
						--error_format=msvs Protos\example_data_payloads.proto

C:\Users\Andrew.Stanton\.nuget\packages\grpc.tools\2.36.4\tools\windows_x86\protoc.exe 
						--csharp_out=obj\Debug\netcoreapp3.1\Protos 
						--proto_path=C:\Users\Andrew.Stanton\.nuget\packages\grpc.tools\2.36.4\build\native\include 
						--proto_path=. 
						--dependency_out=obj\Debug\netcoreapp3.1\12fbc7ffe642ca7e_example_data_schema.protodep 
						--error_format=msvs Protos\example_data_schema.proto

@jtattermusch - these were added from this commit was it working for you then? I dont mind trying an older version but I dont know which version to pick.

@jtattermusch
Copy link
Contributor

AdditionalProtocArguments was added by #25374 and it was tested by the PR author (@moserware). Can you try with the freshest release 2.37.0? The release notes are here: https://github.com/grpc/grpc/releases/tag/v1.37.0

@StingyJack
Copy link
Author

Using AdditionalProtocArguments with a value of AdditionalProtocArguments="--csharp_opt=file_extension=.g.cs" for 2.37.0 does make a file with the .g.cs extension that has what looks to be the expected content, but its also created an empty file. The .g.cs file doesn't look like it is associated with the project and shows as "Miscellaneous files" in the namespace DDL. The empty file looks like it is associated with the project.
image
image

I was really expecting to use the OutputOptions property. Updating to 2.37.0 didnt change anything wrt its behavior

@jtattermusch
Copy link
Contributor

CC @moserware

@moserware
Copy link
Contributor

moserware commented Apr 23, 2021

I took a quick look into this and there seems to be two possible issues:

  1. I don't see user-provided OutputOptions actually being sent via MSBuild to protoc. The build target ultimately passes OutputOptions="%(_Protobuf_OutOfDateProto._OutputOptions)" (note the _ prefix on _OutputOptions). I believe this prefix was used so that the Access= argument could append to it. Indeed, if you add the Access="internal" argument to your <Protobuf , you'll see that --csharp_opt=internal_access is added to the protoc arguments. The issue is that it seems to append to Protobuf_Compile._OutputOptions, but doesn't seem to initialize Protobuf_Compile._OutputOptions from Protobuf_Compile.OutputOptions (note the lack of a _ prefix) and thus ignores any user-provided OutputOptions specified via a .csproj/MSBuild file. As a test, if you manually update your Google.Protobuf.Tools.targets from:
OutputOptions="%(_Protobuf_OutOfDateProto._OutputOptions)"

to:

OutputOptions="%(_Protobuf_OutOfDateProto.OutputOptions);%(_Protobuf_OutOfDateProto._OutputOptions)"

Then you'll see that user-provided MSBuild OutputOptions does begin to work. Although, it's probably better to fix it by ensuring that _OutputOptions is always initialized with OutputOptions.

  1. Regardless of what is specified, the CSharpGeneratorServices.GetPossibleOutputs currently seems to expect a ".cs" extension. I think this is the root cause of the follow-up issue and the cause of --csharp_opt=file_extension= is not supported in the C# tooling #25930 . For example, if you use the manually patched build file patch that I suggest above and then do:
<Protobuf Include="example.proto" OutputOptions="file_extension=.g.cs" />

You'll see that protoc gets the right arguments:

/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/tools/linux_x64/protoc --csharp_out=obj/Debug/net5.0 --csharp_opt=file_extension=.g.cs --plugin=protoc-gen-grpc=/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/tools/linux_x64/grpc_csharp_plugin --grpc_out=obj/Debug/net5.0 --proto_path=/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/build/native/include --proto_path=. --dependency_out=obj/Debug/net5.0/da39a3ee5e6b4b0d_example.protodep --error_format=msvs example.proto

Alternatively, you could use the new AdditionalProtocArguments option:

<Protobuf Include="example.proto" AdditionalProtocArguments="--csharp_opt=file_extension=.g.cs" />

and protoc would still get the right arguments, but just a bit later:

/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/tools/linux_x64/protoc --csharp_out=obj/Debug/net5.0 --plugin=protoc-gen-grpc=/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/tools/linux_x64/grpc_csharp_plugin --grpc_out=obj/Debug/net5.0 --proto_path=/home/jeffmoser/.nuget/packages/grpc.tools/2.37.0/build/native/include --proto_path=. --dependency_out=obj/Debug/net5.0/da39a3ee5e6b4b0d_example.protodep --error_format=msvs --csharp_opt=file_extension=.g.cs example.proto

In either case, protoc indeed produces the Example.g.cs file (and nothing else). However, I think the GetPossibleOutputs issue might be causing the Example.cs file to be expected and thus created as a side-effect.

Thus, to the best of my knowledge, this issue isn't caused by the new AdditionalProtocArguments option but rather due to the issues mentioned above. The first seems to be a bug and the second seems like a feature request to explicitly support alternate file extensions in Google.Protobuf.Tools.targets (similar to explicit support for Access) and also in GetPossibleOutputs.

@josephdrake-stahls
Copy link

josephdrake-stahls commented May 31, 2022

I have been tinkering with a workaround because of a separate issue (poor code coverage exclusion) where I tried to get the files to contain a ".g.cs" extension so that I can globally exclude the ".g.cs" files from code coverage.

In general, the MSBuild targets can be extended (read hacked) to correct the expected outputs:

  <ItemDefinitionGroup>
    <Protobuf_Compile>
      <_OutputOptions>%(Protobuf_Compile._OutputOptions);file_extension=.g.cs</_OutputOptions>
    </Protobuf_Compile>
  </ItemDefinitionGroup>
  <Target Name="_test" AfterTargets="Protobuf_PrepareCompile;Protobuf_PrepareClean">
    <ItemGroup>
      <New_Protobuf_ExpectedOutputs Include="@(Protobuf_ExpectedOutputs -> '%(RootDir)%(Directory)%(Filename).g.cs')" />
      <Protobuf_ExpectedOutputs Remove="@(Protobuf_ExpectedOutputs)" />
      <Protobuf_ExpectedOutputs Include="@(New_Protobuf_ExpectedOutputs)" />
    </ItemGroup>
  </Target>

HOWEVER a bigger problem was found. The grpc csharp plguin does not respect the file_extension option in regards to the generated services. If you run protoc manually with --grpc_out and --csharp_opt=file_extension, the services generated will still be *Grpc.cs. Looking through the code, you can see that there is a magic string used for the service file suffix:

std::string file_suffix = "Grpc.cs";

In the lines following the command line parameters are interpreted and can easily (probably, unless I missed something) look for these options similar to how the standard protobuf csharp compiler operates:
https://github.com/protocolbuffers/protobuf/blob/520c601c99012101c816b6ccc89e8d6fc28fdbb8/src/google/protobuf/compiler/csharp/csharp_generator.cc#L74

edit: included additional msbuild hacks

@tonydnewell
Copy link
Contributor

Summary of above for my benefit:

  • User wanted to change the file extensions for generated code (.g.cs) so that these can be excluded in coverage tools
  • Trying to do this via OutputOptions on Protobuf - but OutputOptions not being passed to the code generator (this is still a bug)
  • Request for changing the file extension won’t be fixed (too much impact on Grpc.Tools which has to try and guess the outputs from protoc)
  • There is now [GeneratedCode] attribute added which solves the original problem (fixed in Add an option to decorate code with GeneratedCodeAttribute (C#) #24594)
  • AdditionalProtocArguments was added in Add support for additional protoc arguments in Grpc.Tools #25374
  • OutputOptions is not passed (can use _OutputOptions - note underscore) - still need to fix

@tonydnewell
Copy link
Contributor

Created a fix for including the OutputOptions and GrpcOutputOptions - #30410

@jtattermusch
Copy link
Contributor

#30410 is now merged and some useful insights are in #25950 (comment) and #25950 (comment).
I believe this can be closed.

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