[CSHARP] Add base_namespace experimental option to C# plugin by tonydnewell · Pull Request #32636 · grpc/grpc

tonydnewell · 2023-03-16T10:39:52Z

Added base_namespace experimental option to grpc_csharp_plugin as this has been requested several times by
people not using Grpc.Tools to generate their code - see #28663

Notes:

it should not be used with Grpc.Tools. That has a different way of handling duplicate proto file names in different directories. Using this option will break those builds. It can only be used on the protoc command line.
it uses common code with the base_namespace option for C# in protoc, which unfortunately has a slightly different name mangling algorithm for converting proto file names to C# camel case names. This only affects files with punctation or numbers in the name. This should not matter unless you are expecting specific file names
See https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options for an explanation of this option

tonydnewell · 2023-03-17T10:32:52Z

@jtattermusch for review

jtattermusch

Overall this looks good but opens up some discussion. I'm also wondering whether we should add bazel tests for codegen first, so that we can add this feature in a more controlled way.

jtattermusch · 2023-03-25T11:38:08Z

src/compiler/csharp_generator_helpers.h

+  // 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.


typo: behavior

jtattermusch · 2023-03-25T12:13:43Z

src/compiler/csharp_generator_helpers.h

+  if (base_namespace.empty()) {
+    out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix;
+  } else {
+    out_file = GetOutputFile(file, file_suffix, true, base_namespace, error);


note to self: https://github.com/protocolbuffers/protobuf/blob/5184022a17031c62d3c2e0bd44f2324752812485/src/google/protobuf/compiler/csharp/names.cc#L125

Also, will using GetOutputFile be still usable in protobuf 22.x (which we are going to upgrade to soon)?

Looks like it would?
https://github.com/protocolbuffers/protobuf/blob/3a871acca05ca4cdf79fc14ea9b701be6fe745f1/src/google/protobuf/compiler/csharp/names.h#L121
https://github.com/protocolbuffers/protobuf/blob/3a871acca05ca4cdf79fc14ea9b701be6fe745f1/src/google/protobuf/compiler/csharp/BUILD.bazel#L15

jtattermusch · 2023-03-25T12:15:57Z

src/compiler/csharp_generator_helpers.h

+  // name.)
+  // Otherwise the behavor remains the same as before.
+  if (base_namespace.empty()) {
+    out_file = grpc_generator::FileNameInUpperCamel(file, false) + file_suffix;


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.

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.

jtattermusch · 2023-03-25T12:17:43Z

src/compiler/csharp_plugin.cc

    bool generate_client = true;
    bool generate_server = true;
    bool internal_access = false;
+    std::string base_namespace = "";


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/test/cpp/codegen/BUILD

Line 93 in 8038d2d

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.

jtattermusch · 2023-03-27T07:11:47Z

src/csharp/BUILD-INTEGRATION.md

-    --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.


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.

nit: add link to "https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options" in "same as base_namespace for protoc C# options"?

jtattermusch · 2023-03-27T07:15:00Z

src/csharp/BUILD-INTEGRATION.md

+metadata in the `<Protobuf>` item.
+- Note: `file_suffix` and `base_namespace` should not be used with `Grpc.Tools`. Using them will break the build.
+
+To use these options on the command line specify them using the `--grpc_opt`


Unless that's mentioned somewhere, also mention that the "csharp" codegen itself (for protobuf messages), uses --csharp_opt flag.
Also, probably add --csharp_opt to the example, since it's more realistic that actual users will want to set base_namespace for both --grpc_opt and --csharp_opt shall they use the base_namespace option.

jtattermusch

I think this looks good, but I'd prefer to hold until we have the bazel codegen tests in place.

jtattermusch · 2023-04-19T11:23:34Z

I just merged #32734, so I think you should rebase and provide a codegen test for when the base_namespace option is enabled? (something simple is enough, e.g. run grep to check that the important parts of the generated file contain the expected stuff there when the option is set and aren't there if it's not set)

tonydnewell · 2023-04-26T12:35:19Z

@jtattermusch
Added bazel tests.
This should probably be merged after #32414
Both modify the BUILD bazel file so some merging/editing may need to be done before this can be merged.

…32636)" This reverts commit d534b4a.

apolcyn · 2023-04-27T19:49:05Z

src/compiler/csharp_generator_helpers.h

 #include "src/compiler/config.h"
 #include "src/compiler/generator_helpers.h"

+using google::protobuf::compiler::csharp::GetOutputFile;


@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.

…#32957) Reverts #32636 ``` src/compiler/csharp_generator_helpers.h:25:7: error: no member named 'compiler' in namespace ... src/compiler/csharp_generator_helpers.h:25:25: error: no member named 'csharp' in namespace 'compiler' ... ```

) Added `base_namespace` experimental option to `grpc_csharp_plugin` as this has been requested several times by people not using `Grpc.Tools` to generate their code - see grpc#28663 Notes: - it should not be used with `Grpc.Tools`. That has a different way of handling duplicate proto file names in different directories. Using this option will break those builds. It can only be used on the `protoc` command line. - it uses common code with the `base_namespace` option for C# in `protoc`, which unfortunately has a slightly different name mangling algorithm for converting proto file names to C# camel case names. This only affects files with punctation or numbers in the name. This should not matter unless you are expecting specific file names - See https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options for an explanation of this option

…grpc#32957) Reverts grpc#32636 ``` src/compiler/csharp_generator_helpers.h:25:7: error: no member named 'compiler' in namespace ... src/compiler/csharp_generator_helpers.h:25:25: error: no member named 'csharp' in namespace 'compiler' ... ```

) Added `base_namespace` experimental option to `grpc_csharp_plugin` as this has been requested several times by people not using `Grpc.Tools` to generate their code - see grpc#28663 Notes: - it should not be used with `Grpc.Tools`. That has a different way of handling duplicate proto file names in different directories. Using this option will break those builds. It can only be used on the `protoc` command line. - it uses common code with the `base_namespace` option for C# in `protoc`, which unfortunately has a slightly different name mangling algorithm for converting proto file names to C# camel case names. This only affects files with punctation or numbers in the name. This should not matter unless you are expecting specific file names - See https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options for an explanation of this option

…grpc#32957) Reverts grpc#32636 ``` src/compiler/csharp_generator_helpers.h:25:7: error: no member named 'compiler' in namespace ... src/compiler/csharp_generator_helpers.h:25:25: error: no member named 'csharp' in namespace 'compiler' ... ```

Added `base_namespace` experimental option to `grpc_csharp_plugin` as this has been requested several times by people not using `Grpc.Tools` to generate their code - see #28663 Notes: - it should not be used with `Grpc.Tools`. That has a different way of handling duplicate proto file names in different directories. Using this option will break those builds. It can only be used on the `protoc` command line. - it uses common code with the `base_namespace` option for C# in `protoc`, which unfortunately has a slightly different name mangling algorithm for converting proto file names to C# camel case names. This only affects files with punctation or numbers in the name. This should not matter unless you are expecting specific file names - See https://protobuf.dev/reference/csharp/csharp-generated/#compiler_options for an explanation of this option

…#32957) Reverts #32636 ``` src/compiler/csharp_generator_helpers.h:25:7: error: no member named 'compiler' in namespace ... src/compiler/csharp_generator_helpers.h:25:25: error: no member named 'csharp' in namespace 'compiler' ... ```

… patch) (#33535) 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). --------- Co-authored-by: tony <tony.newell@pobox.com>

… 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>

Add base_namespace to C# plugin

6d7abc6

github-actions bot added the lang/C# label Mar 16, 2023

tonydnewell mentioned this pull request Mar 16, 2023

C# protoc codegen plugin: Add base_namespace option to grpc_csharp_plugin #28663

Closed

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels Mar 16, 2023

tonydnewell added 2 commits March 16, 2023 18:55

Fix code formatting

81765f1

Fix C++ formatting

24c8569

jtattermusch added the release notes: yes Indicates if PR needs to be in release notes label Mar 27, 2023

jtattermusch reviewed Mar 27, 2023

View reviewed changes

Updates from review

4885898

jtattermusch approved these changes Mar 31, 2023

View reviewed changes

jtattermusch changed the title ~~Add base_namespace experimental option to C# plugin~~ [csharp] Add base_namespace experimental option to C# plugin Mar 31, 2023

tonydnewell and others added 2 commits April 26, 2023 11:09

Merge branch 'grpc:master' into grpc_base_namespace

b629884

Added bazel tests

caece42

tonydnewell added 2 commits April 27, 2023 10:04

Merging from master

e150a65

Fix format of BUILD file

7fe9f32

apolcyn changed the title ~~[csharp] Add base_namespace experimental option to C# plugin~~ [CSHARP] Add base_namespace experimental option to C# plugin Apr 27, 2023

apolcyn merged commit d534b4a into grpc:master Apr 27, 2023

drfloob added a commit that referenced this pull request Apr 27, 2023

Revert "[CSHARP] Add base_namespace experimental option to C# plugin (#…

73ca9d2

…32636)" This reverts commit d534b4a.

drfloob mentioned this pull request Apr 27, 2023

Revert "[CSHARP] Add base_namespace experimental option to C# plugin" #32957

Merged

apolcyn reviewed Apr 27, 2023

View reviewed changes

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Apr 28, 2023

tonydnewell mentioned this pull request Jun 1, 2023

Revert "Revert "[CSHARP] Add base_namespace experimental option to C#… #33310

Closed

jtattermusch mentioned this pull request Jun 23, 2023

[csharp] reintroduce base_namespace experimental option to C# (with a patch) #33535

Merged

Conversation

tonydnewell commented Mar 16, 2023

Uh oh!

tonydnewell commented Mar 17, 2023

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonydnewell Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

jtattermusch commented Apr 19, 2023

Uh oh!

tonydnewell commented Apr 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tonydnewell Mar 30, 2023 •

edited

Loading