[csharp] reintroduce base_namespace experimental option to C# (with a patch)#33535
Conversation
fd451fb to
c7a9f0b
Compare
… plugin" (grpc#32957)" This reverts commit d63f8d4.
c7a9f0b to
e33eab6
Compare
|
The internal build is passing, so I'm going the assume that my fix worked and this PR is ready to merge. |
… patch) (grpc#33535) Reintroduces patched version of grpc#32636 (which was reverted in grpc#32957). Together with cl/542843305, the internal build should work fine. Supersedes grpc#33310 (since a patch is also needed). --------- Co-authored-by: tony <tony.newell@pobox.com>
| if (base_namespace.empty()) { | ||
| out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix; | ||
| } else { | ||
| out_file = GRPC_CUSTOM_CSHARP_GETOUTPUTFILE(file, file_suffix, true, | ||
| base_namespace, error); | ||
| if (out_file.empty()) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not entirely sure what happens here, but with protoc's base_namespace option I can specify base_namespace= (yes, without a value after the =) and it'll generate the code into a folder structure instead of putting everything into the root directory.
I expected the same behavior for grpc, but it still tries to put all files into the root directory, which in my case causes issues with duplicate file names.
Since GRPC_CUSTOM_CSHARP_GETOUTPUTFILE() already handles the base_namespace.empty() case perhaps the check in grpc just needs to be removed?:
https://github.com/protocolbuffers/protobuf/blob/1bfcedaf1280923f36e52a6fa0a5645ab63568f0/src/google/protobuf/compiler/csharp/names.cc#L137-L153
There was a problem hiding this comment.
Just found the empty string behavior in protoc documented here:
https://github.com/protocolbuffers/protobuf/blob/1bfcedaf1280923f36e52a6fa0a5645ab63568f0/src/google/protobuf/compiler/csharp/csharp_options.h#L63-L67
So, essentially when base_namespace is explicitly set via the CLI the variable base_namespace_specified is set to true which in turn causes GetOutputFile() to be invoked with generate_directories set to true.
To match the behavior, grpc should also check if base_namespace was explicitly set and use that in the conditional, e.g.:
| if (base_namespace.empty()) { | |
| out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix; | |
| } else { | |
| out_file = GRPC_CUSTOM_CSHARP_GETOUTPUTFILE(file, file_suffix, true, | |
| base_namespace, error); | |
| if (out_file.empty()) { | |
| return false; | |
| } | |
| } | |
| if (!base_namespace_specified) { | |
| out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix; | |
| } else { | |
| out_file = GRPC_CUSTOM_CSHARP_GETOUTPUTFILE(file, file_suffix, true, | |
| base_namespace, error); | |
| if (out_file.empty()) { | |
| return false; | |
| } | |
| } |
No need to removing this check entirely to retain backwards compatibility. See https://github.com/grpc/grpc/pull/32636/files#r1148360058 for some notes on that.
There was a problem hiding this comment.
I've created a new issue for this: #34113
Reintroduces patched version of #32636 (which was reverted in #32957).
Together with cl/542843305, the internal build should work fine.
Supersedes #33310 (since a patch is also needed).