-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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] Add base_namespace experimental option to C# plugin #32636
Changes from 3 commits
6d7abc6
81765f1
24c8569
4885898
b629884
caece42
e150a65
7fe9f32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,13 +22,30 @@ | |
#include "src/compiler/config.h" | ||
#include "src/compiler/generator_helpers.h" | ||
|
||
using google::protobuf::compiler::csharp::GetOutputFile; | ||
|
||
namespace grpc_csharp_generator { | ||
|
||
inline bool ServicesFilename(const grpc::protobuf::FileDescriptor* file, | ||
const std::string& file_suffix, | ||
std::string& out_file_name_or_error) { | ||
out_file_name_or_error = | ||
grpc_generator::FileNameInUpperCamel(file, false) + file_suffix; | ||
const std::string& base_namespace, | ||
std::string& out_file, std::string* error) { | ||
// Support for base_namespace option is **experimental**. | ||
// | ||
// If base_namespace is provided then slightly different name mangling | ||
// is used to generate the service file name. This is because this | ||
// uses common code with protoc. For most file names this will not | ||
// make a difference (only files with punctuation or numbers in the | ||
// name.) | ||
// Otherwise the behavor remains the same as before. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: behavior |
||
if (base_namespace.empty()) { | ||
out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wondering why we haven't always been using protobuf's GetOutputFile implementation? It would be good to do a quick check (e.g. take a closer look at history of implementations for grpc and protobuf) to find out the reason just to make sure we didn't forget about anything. Also, looks like at this point we could probably switch to "GetOutputFile" implementation for the default case as well, after which file names produces by grpc_csharp_plugin and protoc would be 100% consistent in handling corner cases in file naming. But I think to avoid issues with backward compatibility of the Grpc.Tools msbuild stack (which needs to know the file names exactly) we'd rather not do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like GetOutputFile has been exported publicly for 7 years. Not sure why it wasn't used before. Changing the plugin to always use GetOutputFile could be a potentially breaking change for some users. I wouldn't want to do it yet, but could do in a future release of Grpc.Tools. The change would have to be documented. Also anyone using their own build of the plugin (e.g. on unsupported platforms like Alpine) may break if the Grpc.Tools logic is out of sync with their plugin. |
||
} else { | ||
out_file = GetOutputFile(file, file_suffix, true, base_namespace, error); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, will using GetOutputFile be still usable in protobuf 22.x (which we are going to upgrade to soon)? Looks like it would? |
||
if (out_file.empty()) { | ||
return false; | ||
} | ||
} | ||
return true; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -43,6 +43,8 @@ class CSharpGrpcGenerator : public grpc::protobuf::compiler::CodeGenerator { | |||
bool generate_client = true; | ||||
bool generate_server = true; | ||||
bool internal_access = false; | ||||
std::string base_namespace = ""; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we'll probably need to introduce a bazel test for the grpc_csharp_plugin, that does something similar to the C++ golden file test Line 93 in 8038d2d
at least having a golden file for C# would be a good way of making the diff in the generated code visible in PR reviews. Also, we could have different variants of the test generate code with different options. |
||||
|
||||
// the suffix that will get appended to the name generated from the name | ||||
// of the original .proto file | ||||
std::string file_suffix = "Grpc.cs"; | ||||
|
@@ -55,6 +57,11 @@ class CSharpGrpcGenerator : public grpc::protobuf::compiler::CodeGenerator { | |||
internal_access = true; | ||||
} else if (options[i].first == "file_suffix") { | ||||
file_suffix = options[i].second; | ||||
} else if (options[i].first == "base_namespace") { | ||||
// Support for base_namespace option in this plugin is experimental. | ||||
// The option may be removed or file names generated may change | ||||
// in the future. | ||||
base_namespace = options[i].second; | ||||
} else { | ||||
*error = "Unknown generator option: " + options[i].first; | ||||
return false; | ||||
|
@@ -69,8 +76,8 @@ class CSharpGrpcGenerator : public grpc::protobuf::compiler::CodeGenerator { | |||
|
||||
// Get output file name. | ||||
std::string file_name; | ||||
if (!grpc_csharp_generator::ServicesFilename(file, file_suffix, | ||||
file_name)) { | ||||
if (!grpc_csharp_generator::ServicesFilename( | ||||
file, file_suffix, base_namespace, file_name, error)) { | ||||
return false; | ||||
} | ||||
std::unique_ptr<grpc::protobuf::io::ZeroCopyOutputStream> output( | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -170,12 +170,21 @@ to perform code generation. Here is an overview of the available `grpc_csharp_pl | |
| no_client | off | Don't generate the client stub | | ||
| no_server | off | Don't generate the server-side stub | | ||
| internal_access | off | Generate classes with "internal" visibility | | ||
| file_suffix | Grpc.cs | The suffix that will get appended to the name of the generated file. **Can only be used on the command line.** | | ||
| base_namespace | none | *Experimental - may change or be removed.* Same as `base_namespace` for `protoc` C# options. **Can only be used on the command line.** | | ||
|
||
Note that the protocol buffer compiler has a special commandline syntax for plugin options. | ||
Example: | ||
``` | ||
protoc --plugin=protoc-gen-grpc=grpc_csharp_plugin --csharp_out=OUT_DIR \ | ||
--grpc_out=OUT_DIR --grpc_opt=lite_client,no_server \ | ||
To use these options with `Grpc.Tools` specify them in the __GrpcOutputOptions__ | ||
metadata in the `<Protobuf>` item. | ||
- Note: `file_suffix` and `base_namespace` should not be used with `Grpc.Tools`. Using them will break the build. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a note that when base_namespace is used, it changes the algorithm for generating file name from the name of the proto file slightly (this might be quite important for users and currently it's only mentioned in a comment in the implementation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: add link to "https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options" in "same as base_namespace for protoc C# options"? |
||
|
||
To use these options on the command line specify them using the `--grpc_opt` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless that's mentioned somewhere, also mention that the "csharp" codegen itself (for protobuf messages), uses --csharp_opt flag. |
||
command line option, e.g.: | ||
|
||
```bash | ||
protoc --plugin=protoc-gen-grpc=grpc_csharp_plugin \ | ||
--csharp_out=OUT_DIR \ | ||
--grpc_out=OUT_DIR \ | ||
--grpc_opt=no_server,base_namespace=Example \ | ||
-I INCLUDE_DIR foo.proto | ||
``` | ||
## Environment Variables | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tonydnewell we needed to revert this in #32636 because of some internal build errors, basically the following:
I have not gotten too far into the details but I'm wondering if the
google::protobuf::compiler::csharp::GetOutputFile
is a new API, only in some older versions of protobuf, or something else like that.