Skip to content

Commit

Permalink
Grpc.Tools: Parse warnings from libprotobuf (fix #27502) (#30371)
Browse files Browse the repository at this point in the history 
* issue 27502 - parse warning from libprotobuf

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.

* Fix typos in comments
  • Loading branch information
@tonydnewell
tonydnewell committed Sep 5, 2022
1 parent f0948a7 commit e313ac8
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 0 deletions.
33 changes: 33 additions & 0 deletions src/csharp/Grpc.Tools.Tests/ProtoCompileCommandLineGeneratorTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,13 +199,21 @@ public void DirectorySlashTrimmingCases(string given, string expect)
"Import google/protobuf/empty.proto but not used.")]
[TestCase("../Protos/greet.proto(14) : error in column=10: \"name\" is already defined in \"Greet.HelloRequest\".", null, 0, 0, null)]
[TestCase("../Protos/greet.proto: Import \"google / protobuf / empty.proto\" was listed twice.", null, 0, 0, null)]
[TestCase("[libprotobuf WARNING T:\\altsrc\\github\\...\\csharp\\csharp_enum.cc:74] Duplicate enum value Work (originally Work) in PhoneType; adding underscore to distinguish",
"T:\\altsrc\\github\\...\\csharp\\csharp_enum.cc",
74, 0,
"Duplicate enum value Work (originally Work) in PhoneType; adding underscore to distinguish")]
[TestCase("[libprotobuf ERROR T:\\path\\...\\filename:23] Some message", null, 0, 0, null)]
[TestCase("[libprotobuf FATAL T:\\path\\...\\filename:23] Some message", null, 0, 0, null)]
public void WarningsParsed(string stderr, string file, int line, int col, string message)
{
_task.StdErrMessages.Add(stderr);

bool matched = false;
_mockEngine
.Setup(me => me.LogWarningEvent(It.IsAny<BuildWarningEventArgs>()))
.Callback((BuildWarningEventArgs e) => {
matched = true;
if (file != null)
{
Assert.AreEqual(file, e.File);
Expand All @@ -221,6 +229,14 @@ public void WarningsParsed(string stderr, string file, int line, int col, string

bool result = _task.Execute();
Assert.IsFalse(result);

// To get here in the test then either the event fired and the values matched
// or the event did not fire (input did not parse as a warning).
// If it did not parse as a warning then check that the expected message is null
if (!matched && message != null)
{
Assert.Fail($"Expected match: {message}");
}
}

[TestCase(
Expand All @@ -237,13 +253,21 @@ public void WarningsParsed(string stderr, string file, int line, int col, string
"Import \"google / protobuf / empty.proto\" was listed twice.")]
[TestCase("../Protos/greet.proto(19) : warning in column=5 : warning : When enum name is stripped and label is PascalCased (Zero) this value label conflicts with Zero.", null, 0, 0, null)]
[TestCase("../Protos/greet.proto: warning: Import google/protobuf/empty.proto but not used.", null, 0, 0, null)]
[TestCase("[libprotobuf WARNING T:\\altsrc\\github\\...\\csharp\\csharp_enum.cc:74] Duplicate enum value Work (originally Work) in PhoneType; adding underscore to distinguish",
null, 0, 0, null)]
[TestCase("[libprotobuf ERROR T:\\path\\...\\filename:23] Some message",
"T:\\path\\...\\filename", 23, 0, "ERROR Some message")]
[TestCase("[libprotobuf FATAL T:\\path\\...\\filename:23] Some message",
"T:\\path\\...\\filename", 23, 0, "FATAL Some message")]
public void ErrorsParsed(string stderr, string file, int line, int col, string message)
{
_task.StdErrMessages.Add(stderr);

bool matched = false;
_mockEngine
.Setup(me => me.LogErrorEvent(It.IsAny<BuildErrorEventArgs>()))
.Callback((BuildErrorEventArgs e) => {
matched = true;
if (file != null)
{
Assert.AreEqual(file, e.File);
Expand All @@ -262,8 +286,17 @@ public void ErrorsParsed(string stderr, string file, int line, int col, string m
}
});


bool result = _task.Execute();
Assert.IsFalse(result);

// To get here in the test then either the event fired and the values matched
// or the event did not fire (input did not parse as an error).
// If it did not parse as an error then check that the expected message is null
if (!matched && message != null)
{
Assert.Fail($"Expected match: {message}");
}
}
};
}
57 changes: 57 additions & 0 deletions src/csharp/Grpc.Tools/ProtoCompile.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,63 @@ public class ProtoCompile : ToolTask
}
},

// Example warning from plugins that use GOOGLE_LOG
// [libprotobuf WARNING T:\altsrc\..\csharp_enum.cc:74] Duplicate enum value Work (originally Work) in PhoneType; adding underscore to distinguish
new ErrorListFilter
{
Pattern = new Regex(
pattern: "^\\[.+? WARNING (?'FILENAME'.+?):(?'LINE'\\d+?)\\] ?(?'TEXT'.*)",
options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
matchTimeout: s_regexTimeout),
LogAction = (log, match) =>
{
// 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
// diagnostics.
int.TryParse(match.Groups["LINE"].Value, out var line);
log.LogWarning(
subcategory: null,
warningCode: null,
helpKeyword: null,
file: match.Groups["FILENAME"].Value,
lineNumber: line,
columnNumber: 0,
endLineNumber: 0,
endColumnNumber: 0,
message: match.Groups["TEXT"].Value);
}
},

// Example error from plugins that use GOOGLE_LOG
// [libprotobuf ERROR T:\path\...\filename:23] Some message
// [libprotobuf FATAL T:\path\...\filename:23] Some message
new ErrorListFilter
{
Pattern = new Regex(
pattern: "^\\[.+? (?'LEVEL'ERROR|FATAL) (?'FILENAME'.+?):(?'LINE'\\d+?)\\] ?(?'TEXT'.*)",
options: RegexOptions.Compiled | RegexOptions.IgnoreCase,
matchTimeout: s_regexTimeout),
LogAction = (log, match) =>
{
// 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
// diagnostics.
int.TryParse(match.Groups["LINE"].Value, out var line);
log.LogError(
subcategory: null,
errorCode: null,
helpKeyword: null,
file: match.Groups["FILENAME"].Value,
lineNumber: line,
columnNumber: 0,
endLineNumber: 0,
endColumnNumber: 0,
message: match.Groups["LEVEL"].Value + " " + match.Groups["TEXT"].Value);
}
},

// Example error without location
//../Protos/greet.proto: Import "google/protobuf/empty.proto" was listed twice.
new ErrorListFilter
Expand Down

0 comments on commit e313ac8

Please sign in to comment.