[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.
emaf
requested review from
chamons,
jstedfast,
kzu,
rolfbjarne and
VincentDondain
as code owners
| } | ||
| } | ||
| } 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 |
| } |
| 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? |
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.