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

--csharp_opt=file_extension= is not supported in the C# tooling #25930

Closed
odalet opened this issue Apr 8, 2021 · 8 comments
Closed

--csharp_opt=file_extension= is not supported in the C# tooling #25930

odalet opened this issue Apr 8, 2021 · 8 comments

Comments

@odalet
Copy link

odalet commented Apr 8, 2021

What version of gRPC and what language are you using?

This affects Grpc.Tools v2.37 (C#)

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

On Windows, but most probably not dependent on the OS (this is somewhere in the MSBuild tooling)

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

VS2019 16.9.1 (targeting .NET Standard 2.0)

What did you do?

In my csproj, I have something like this (taking advantage of the AdditionalProtocArguments introduced in v2.37):

<Protobuf Include="**/*.proto" GrpcServices="None" AdditionalProtocArguments="--csharp_opt=file_extension=.g.cs" />

What did you expect to see?

The project compiles successfully

What did you see instead?

The project does not compile and complains it can't find the definition of protobuf-generated code

Anything else we should know about your project / environment?

The actual behavior is kind of expected:

My csharp_opt is indeed honored by protoc and the generated files are called foo.g.cs somewhere in the obj\ directory. However, it seems the msbuild task expects the file to be named foo.cs (and by the way, a 0-sized foo.cs file gets created). This explains why the compilation subsequently fails: foo.cs is part of the files to compile, but not foo.g.cs.

The task should be clever enough to not assume the file name to be foo.cs as this can be changed with csharp_opt=file_extension...

@odalet
Copy link
Author

odalet commented Apr 8, 2021

By the way, if this is too much of a hassle to support, it's not that an issue for me (as my initial goal of preventing these generated files to be analyzed by SonarQube is already covered by the fact they live under obj/). However, maybe the documentation should be updated to warn people against modifying the generated file's name.

@jtattermusch
Copy link
Contributor

It is not feasible to track every option being passed to the protoc by attributes that allow arbitrary options (such as AdditionalProtocOptions). If you reconfigure the file extension by explicitly overriding it, the protoc integration logic won't work (as you've noticed).

What is the reason why you need to override the file extension? (there seems to be no point when you're using automatic protoc codegen integration).

Feel free to suggest an update to the documentation: https://github.com/grpc/grpc/blob/master/src/csharp/BUILD-INTEGRATION.md

@odalet
Copy link
Author

odalet commented Apr 16, 2021

Thanks for the reply.

About my initial requirement: before using Grpc.Tools, I was generating the C# source code in a more manual way. The C# files were being generated into the project's source tree. I also happen to have code coverage statistics collected and sent to a SonarQube server. In order for the code coverage to only consider files I wrote, I used to have a .g.cs extension for generated code (similarly to what happens in WPF or Winforms with either .g.cs or .Designer.cs files).

However, because the files generated by Grpc.Tools are placed in the obj subdirectory, this is no longer an issue for me (everything in here is ignored by code analyzers as far as I'm aware).

So I'm closing this and when I find time, I may indeed propose a documentation update as you suggested.

@odalet odalet closed this as completed Apr 16, 2021
@StingyJack
Copy link

I have the same need as @odalet - without one of the common conventions for identifying these as code generated, they show up as "not covered" by coverage analsys tools that use compiled type information (in my case coverlet is used by our build pipelines, the results of which feed sonarcloud).

In my case I dont even get an error. The options are not even getting passed along as arguments to protoc.exe and the file extension is always .cs

It is not feasible to track every option being passed to the protoc by attributes that allow arbitrary options (such as AdditionalProtocOptions). If you reconfigure the file extension by explicitly overriding it, the protoc integration logic won't work (as you've noticed).

@jtattermusch - what is the scope of possible options that MSBuild would be passing along? I can only find two. Two seems feasable, and otherwise necessary in the case of the OutputOptions+file_extension.

@moserware
Copy link
Contributor

As mentioned on #25950 , I think the issue might be caused by CSharpGeneratorServices.GetPossibleOutputs expecting a ".cs" extension and thus the Google.Protobuf.Tools.targets file and GetPossibleOutputs function would likely need to explicitly support a file extension option for it to work properly.

@jtattermusch
Copy link
Contributor

jtattermusch commented May 4, 2021

@moserware

As mentioned on #25950 , I think the issue might be caused by CSharpGeneratorServices.GetPossibleOutputs expecting a ".cs" extension and thus the Google.Protobuf.Tools.targets file and GetPossibleOutputs function would likely need to explicitly support a file extension option for it to work properly.

That is correct. I think GetPossibleOutputs needing to check what value of file_extension was passed to the plugin (which is something that would be required to make Grpc.Tools work with non-default values of file_extension) would be overly complex, so we are not planning to implement it.

@StingyJack you mentioned that the reason why you need this is because the generated code doesn't comply with "common conventions for identifying these as code generated". If that's all that's needed, than adding an extra header/line at the beginning of the generated files would be much easier that messing with the GetPossibleOutputs logic.
What particular convention did you have in mind? We can look into implementing it.

@jtattermusch
Copy link
Contributor

@StingyJack is this what you had in mind?

@StingyJack
Copy link

StingyJack commented May 4, 2021

@jtattermusch the Generated code attribute is the better choice over the filename, because that will be available to both source readers and compiled output readers.

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

5 participants