[msbuild] Improve altool task by logging execution errors - #6815
Conversation
The altool task was just logging the XML output produced by the tool execution but was not logging any build error neither failing in that case.
| } | ||
| } | ||
| } catch (Exception ex) { | ||
| Log.LogWarning ("Failed to parse altool output: {0}", ex.Message); |
There was a problem hiding this comment.
Should you include output in the exception log ?
That way you can get a clue of what went wrong
There was a problem hiding this comment.
also $"Failed to parse altool output: {ex.Message}" looks nicer :)
| void LogErrorsFromOutput (string output) | ||
| { | ||
| try { | ||
| if (string.IsNullOrEmpty(output)) |
| foreach (var error in errors) { | ||
| var dict = error as PDictionary; | ||
| if (dict?.TryGetValue ("message", out message) == true) { | ||
| Log.LogError (ToolName, null, null, null, 0, 0, 0, 0, "{0}", message.Value); |
There was a problem hiding this comment.
not sure if the API (or some overloads) allows it but "{0}" seems to require a call to String.Format, can't you just provide message.Value ?
There was a problem hiding this comment.
@spouliot What if message.Value contains something that can be a format string? Say "{0}" for instance, then things might fail (depending on LogError's implementation).
There was a problem hiding this comment.
yeah, bring us back to not sure if the API :)
There was a problem hiding this comment.
yes, I don't really like it (using "{0}") but it seems to be safer, and that's how most of the XI tasks log errors that come from tool outputs or text we don't control.
|
Build success |
| } | ||
| } | ||
| } No newline at end of file | ||
| } No newline at end of file |
| foreach (var error in errors) { | ||
| var dict = error as PDictionary; | ||
| if (dict?.TryGetValue ("message", out message) == true) { | ||
| Log.LogError (ToolName, null, null, null, 0, 0, 0, 0, "{0}", message.Value); |
There was a problem hiding this comment.
@spouliot What if message.Value contains something that can be a format string? Say "{0}" for instance, then things might fail (depending on LogError's implementation).
|
Build failure 🔥 Build failed 🔥 |
|
build |
1 similar comment
|
build |
|
Build success |
|
@rolfbjarne could you review the latest changes? Is there anything else needed to get this PR merged? |
6 participants
The altool task was just logging the XML output produced by the tool execution but was not logging any build error neither failing in that case.