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
Grpc.Tools: Parse warnings from libprotobuf (fix #27502) #30371
Conversation
The warnings and errors logged by some of the pluggins such as the csharp plugin are in a different format from other warnings etc logged by the protobuf compiler (the plugins use GOOGLE_LOG to log message). The parsing code in Grpc.Tools has been modified to correctly parse these so that warnings are no longer handled as errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not massively familiar with this code, but assuming the new tests failed before and pass now, this LGTM.
// The filename and line logged by the plugins may not be useful to the | ||
// end user as they are not the location in the proto file but rather | ||
// in the source code for the plugin. Log them anyway as they may help in | ||
// diagnositics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: diagnositics => diagnostics, here and below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed typos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Fix for #27502
The warnings and errors logged by some of the plugins such as the csharp plugin are in a different format from other warnings etc logged by the protobuf compiler (the plugins use GOOGLE_LOG to log message).
The parsing code in Grpc.Tools has been modified to correctly parse these so that warnings are no longer handled as errors.
The unit tests have been extended and fixed (they did not notice if a test case did not match the expected log level).