Check exitcodes in build script #3236
Conversation
| [string] $Arguments, | ||
| [int[]] $IgnoreExitCode | ||
| ) | ||
| Write-Verbose "Invoking: $Command $Arguments" |
There was a problem hiding this comment.
Provided that my understanding is correct, the only mandatory argument of this function is $Command, in which case how would Write-Verbose work if we provide no arguments to the command ? Will the $Arguments be empty or null ?
There was a problem hiding this comment.
$Arguments will be defined and will be $null, so it will make no difference to the output, and won't fail even if we were running in strict mode.
| @@ -51,15 +53,25 @@ $env:DOTNET_CLI_VERSION = $GlobalJson.tools.dotnet | |||
| $env:VSWHERE_VERSION = "2.0.2" | |||
| $env:MSBUILD_VERSION = "15.0" | |||
There was a problem hiding this comment.
Should this MSBUILD_VERSION be updated too ?
There was a problem hiding this comment.
IDK I updated the build tools because they were using parameter that is not present in the referenced version, and because the exit code was not checked it just "silently" failed. I don't see that environment variable used anywhere, and if it has significance to msbuild, then it is not breaking, and I don't want to fiddle with this any more than I need to 😁
| Write-Output "... $message" | ||
| try { | ||
| $Host.UI.RawUI.ForegroundColor = if ("Success" -eq $Level) { "Green" } else { "Red" } | ||
| if ($message) |
There was a problem hiding this comment.
Could we maybe do some kind of early return here and basically do nothing, not even change the ForegroundColor if the $message variable is empty ? Something along the lines of:
function Write-Log {
param (...)
if (!$message)
{
return
}
try {
...
}
finally {
...
}
There was a problem hiding this comment.
We might, but it won't save any time. Thanks for the feedback, but I won't do it now.
| @@ -54,6 +54,8 @@ Param( | |||
| [String[]] $Steps = @("InstallDotnet", "Restore", "UpdateLocalization", "Build", "Publish", "PrepareAcceptanceTests") | |||
| ) | |||
|
|
|||
| $ErrorActionPreference = 'Stop' | |||
There was a problem hiding this comment.
Would there be any use case in which we'd like to have this error action preference configurable ?
There was a problem hiding this comment.
Nope. This is a best practice. The common.lib.ps1 file actually sets this as well. Setting this to Stop, and optionally overriding it to Continue on commands that are allowed to fail is how this is done.
Description
Check exit codes of executables we call and write the actual call into verbose rather than a copy of the command that is sometimes not updated.
Related issue
Kindly link any related issues. E.g. Fixes #xyz.