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] reintroduce base_namespace experimental option to C# (with a patch) #33535

Merged
merged 2 commits into from Jun 26, 2023

Conversation

jtattermusch
Copy link
Contributor

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

@jtattermusch jtattermusch changed the title Grpc base namespace retry [csharp] reintroduce base_namespace experimental option to C# (with a patch) Jun 23, 2023
@jtattermusch jtattermusch added the release notes: yes Indicates if PR needs to be in release notes label Jun 23, 2023
@jtattermusch
Copy link
Contributor Author

@tonydnewell

@jtattermusch
Copy link
Contributor Author

The internal build is passing, so I'm going the assume that my fix worked and this PR is ready to merge.

@jtattermusch jtattermusch merged commit 1002319 into grpc:master Jun 26, 2023
63 of 66 checks passed
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Jun 26, 2023
mario-vimal pushed a commit to mario-vimal/grpc that referenced this pull request Jul 13, 2023
… 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>
Comment on lines +39 to +47
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;
}
}
Copy link

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Copy link

Choose a reason for hiding this comment

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

I've created a new issue for this: #34113

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/C# per-call-memory/neutral per-channel-memory/neutral release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants