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] Add base_namespace experimental option to C# plugin #32636

Merged
merged 8 commits into from
Apr 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 20 additions & 3 deletions src/compiler/csharp_generator_helpers.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,30 @@
#include "src/compiler/config.h"
#include "src/compiler/generator_helpers.h"

using google::protobuf::compiler::csharp::GetOutputFile;
Copy link
Contributor

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:

src/compiler/csharp_generator_helpers.h:25] error: no member named 'compiler' in namespace 'google::protobuf'
using google::protobuf::compiler::csharp::GetOutputFile;

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.


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 behavior remains the same as before.
if (base_namespace.empty()) {
out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix;
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
But it's good to know this is an option for future debates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (out_file.empty()) {
return false;
}
}
return true;
}

Expand Down
11 changes: 9 additions & 2 deletions src/compiler/csharp_plugin.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "";
Copy link
Contributor

Choose a reason for hiding this comment

The 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

grpc_sh_test(

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";
Expand All @@ -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;
Expand All @@ -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(
Expand Down
31 changes: 26 additions & 5 deletions src/csharp/BUILD-INTEGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,33 @@ 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](https://protobuf.dev/reference/csharp/csharp-generated/#compiler_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.

Notes:
- `file_suffix` and `base_namespace` should not be used with `Grpc.Tools`. Using them will break the build.

- using `base_namespace` changes the algorithm for the generated file names to align it with the algorithm used by `protoc`.

This only affects files with punctuation or numbers in the name. E.g. `hello.world2d.proto` now generates file `HelloWorld2DGrpc.cs` instead of `Hello.world2dGrpc.cs`

To use these options on the command line specify them with the `--grpc_opt`
option.

Code generated by `protoc` is independent of the plugin and you may also need to specify C# options for this with `--csharp_opt`.
These are [documented here](https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options).

e.g.:

```bash
protoc --plugin=protoc-gen-grpc=grpc_csharp_plugin \
--csharp_out=OUT_DIR \
--csharp_opt=base_namespace=Example \
--grpc_out=OUT_DIR \
--grpc_opt=no_server,base_namespace=Example \
-I INCLUDE_DIR foo.proto
```
## Environment Variables
Expand Down
19 changes: 19 additions & 0 deletions test/csharp/codegen/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,22 @@ grpc_sh_test(
],
uses_polling = False,
)

grpc_sh_test(
name = "csharp_codegen_base_namespace_test",
size = "small",
srcs = ["csharp_codegen_base_namespace_test.sh"],
data = [
"basenamespace/proto/namespacetest.proto",
"//src/compiler:grpc_csharp_plugin",
"@com_google_protobuf//:protoc",
],
tags = [
"no_windows",
"noasan",
"nomsan",
"notsan",
"noubsan",
],
uses_polling = False,
)
37 changes: 37 additions & 0 deletions test/csharp/codegen/basenamespace/proto/namespacetest.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2023 gRPC authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package test.csharp.codegen.basenamespace.test;

option csharp_namespace = "Example.V1.CodegenTest";

// The greeting service definition.
service Greeter {
// Sends a greeting
rpc SayHello (HelloRequest) returns (HelloReply) {}

rpc SayHelloStreamReply (HelloRequest) returns (stream HelloReply) {}
}

// The request message containing the user's name.
message HelloRequest {
string name = 1;
}

// The response message containing the greetings
message HelloReply {
string message = 1;
}
97 changes: 97 additions & 0 deletions test/csharp/codegen/csharp_codegen_base_namespace_test.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
#!/bin/bash
# Copyright 2023 gRPC authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# Run this script via bazel test
# It expects that protoc and grpc_csharp_plugin have already been built.

# Simple test - compare generated output to expected files

set -x

TESTNAME=basenamespace

# protoc and grpc_csharp_plugin binaries are supplied as "data" in bazel
PROTOC=./external/com_google_protobuf/protoc
PLUGIN=./src/compiler/grpc_csharp_plugin

# where to find the test data
DATA_DIR=./test/csharp/codegen/${TESTNAME}

# output directory for the generated files
PROTO_OUT=./proto_out
rm -rf ${PROTO_OUT}
mkdir -p ${PROTO_OUT}

# run protoc and the plugin specifying the base_namespace options
$PROTOC \
--plugin=protoc-gen-grpc-csharp=$PLUGIN \
--csharp_out=${PROTO_OUT} \
--grpc-csharp_out=${PROTO_OUT} \
--csharp_opt=base_namespace=Example \
--grpc-csharp_opt=base_namespace=Example \
-I ${DATA_DIR}/proto \
${DATA_DIR}/proto/namespacetest.proto

# log the files generated
ls -lR ./proto_out

# Verify the output files exist in the right location.
# The base_namespace option does not change the generated code just
# the location of the files. Contents are not checked in this test.

# The C# namespace option in the proto file of "Example.V1.CodegenTest"
# combined with the command line options above should mean the generated files
# are created in the output directory "V1/CodegenTest"

# First file is generated by protoc.
[ -e ${PROTO_OUT}/V1/CodegenTest/Namespacetest.cs ] || {
echo >&2 "missing generated output, expecting V1/CodegenTest/Namespacetest.cs"
exit 1
}

# Second file is generated by the plugin.
[ -e ${PROTO_OUT}/V1/CodegenTest/NamespacetestGrpc.cs ] || {
echo >&2 "missing generated output, expecting V1/CodegenTest/NamespacetestGrpc.cs"
exit 1
}

# Run again without the base_namespace options to check the files are created
# in the root of the output directory

rm -rf ${PROTO_OUT}
mkdir -p ${PROTO_OUT}

$PROTOC \
--plugin=protoc-gen-grpc-csharp=$PLUGIN \
--csharp_out=${PROTO_OUT} \
--grpc-csharp_out=${PROTO_OUT} \
-I ${DATA_DIR}/proto \
${DATA_DIR}/proto/namespacetest.proto

ls -lR ./proto_out

[ -e ${PROTO_OUT}/Namespacetest.cs ] || {
echo >&2 "missing generated output, expecting Namespacetest.cs"
exit 1
}

[ -e ${PROTO_OUT}/NamespacetestGrpc.cs ] || {
echo >&2 "missing generated output, expecting NamespacetestGrpc.cs"
exit 1
}

# Run one extra command to clear $? before exiting the script to prevent
# failing even when tests pass.
echo "Plugin test: ${TESTNAME}: passed."