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

C#: Suppress CS8981 in generated source #9981

Merged
merged 2 commits into from
May 18, 2022
Merged

Conversation

JamesNK
Copy link
Contributor

@JamesNK JamesNK commented May 17, 2022

Fixes grpc/grpc#29672

This is option 2 (see linked issue). It suppresses the new warning in the generated source code. It's the simplest change and I think it's unlikely that any of the namespace aliases would turn into keywords.

@jtattermusch @jskeet

@JamesNK JamesNK changed the title Suppress CS8981 in generated source C#: Suppress CS8981 in generated source May 17, 2022
@jskeet jskeet self-assigned this May 17, 2022
@jskeet jskeet requested review from jskeet and jtattermusch May 17, 2022 05:52
@jskeet
Copy link
Contributor

jskeet commented May 17, 2022

This seems reasonable to me. It's an unfortunate warning to be on by default IMO (and particularly as it refers to the alias as a type name, when it's not) but suppressing it seems reasonable. Will wait for @jtattermusch's input.

We should regenerate all the code in the csharp directory as well - I'm happy to do that after this PR is in, or it could be a second commit in this PR.

@JamesNK
Copy link
Contributor Author

JamesNK commented May 17, 2022

We should regenerate all the code in the csharp directory as well - I'm happy to do that after this PR is in, or it could be a second commit in this PR.

A follow-up PR would be great. I've rebuilt my machine since I last updated the C++ bits in this repo, and I don't have it setup anymore.

I see an RC for the next release has recently been cut - https://github.com/protocolbuffers/protobuf/releases/tag/v21.0-rc1. What do you think about putting this change into 21? Either RC2, or a patch release for it?

This fix isn't urgent yet because .NET 7 is only in preview, but anyone who downloads it and compiles generated Protobuf or gRPC code will get compiler warnings, and that'll break those with WarningsAsErrors enable. They'll need to update their csproj to suppress this error.

@jskeet
Copy link
Contributor

jskeet commented May 17, 2022

I see an RC for the next release has recently been cut - https://github.com/protocolbuffers/protobuf/releases/tag/v21.0-rc1. What do you think about putting this change into 21? Either RC2, or a patch release for it?

I don't know enough about the protobuf release process to know whether that's feasible/desirable - I'll follow up internally.

Fundamentally, I think it's pretty reasonable for folks using preview builds to have to take some action when a new warning is introduced.

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM, the proposed solution looks good.

I'd say let's add a commit to this PR so that it has both the generator code and the regenerated sources.
Note that other maintainers can push commits to this PR.

@jskeet do you want to push the regenerated protos to this PR?

@jskeet
Copy link
Contributor

jskeet commented May 18, 2022

@jtattermusch: I'm fine with that as a plan - but I can't do it myself as I currently don't have a working Google machine. (Long story.) Are you happy to pull, build, regenerate and add the commit yourself?

@jtattermusch
Copy link
Contributor

@jtattermusch: I'm fine with that as a plan - but I can't do it myself as I currently don't have a working Google machine. (Long story.) Are you happy to pull, build, regenerate and add the commit yourself?

Regenerated C# protos pushed in a separate commit.

@jskeet
Copy link
Contributor

jskeet commented May 18, 2022

Thanks @jtattermusch - will merge when the Kokoro build has completed.

@jskeet
Copy link
Contributor

jskeet commented May 18, 2022

The MacOS Ruby Kokoro build seems to have actually completed successfully (just not reported back) so I'll merge now.

@jskeet jskeet merged commit d2292fc into protocolbuffers:main May 18, 2022
@jtattermusch
Copy link
Contributor

For the gRPC codegen plugin update, also see grpc/grpc#29708

@jtattermusch
Copy link
Contributor

Should be backport this to one of the existing release branches?
(see grpc/grpc#29708 (comment))
I don't think this change by itself requires a protobuf patch release, but if we're already planning a patch release for protobuf, we might as well include this change.
I'm unfortunately not fully up to date on the recent protobuf versioning change, so I'm not sure whether we would backport to https://github.com/protocolbuffers/protobuf/releases/tag/v21.0-rc1 or to https://github.com/protocolbuffers/protobuf/releases/tag/v3.20.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grpc.Tools: Warnings from generated code in .NET 7
5 participants