-
Notifications
You must be signed in to change notification settings - Fork 323
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
Check exitcodes in build script #3236
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
# Copyright (c) Microsoft. All rights reserved. | ||
|
||
$ErrorActionPreference = "Stop" | ||
$script:ScriptFailedCommands = @() | ||
$script:ScriptFailed = $false | ||
|
||
# | ||
# Git Branch | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😁 |
||
|
||
function Write-Log ([string] $message) | ||
{ | ||
function Write-Log { | ||
param ( | ||
[string] $message, | ||
[ValidateSet("Success", "Error")] | ||
[string] | ||
$Level = "Success" | ||
) | ||
|
||
$currentColor = $Host.UI.RawUI.ForegroundColor | ||
$Host.UI.RawUI.ForegroundColor = "Green" | ||
if ($message) | ||
{ | ||
Write-Output "... $message" | ||
try { | ||
$Host.UI.RawUI.ForegroundColor = if ("Success" -eq $Level) { "Green" } else { "Red" } | ||
if ($message) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we maybe do some kind of early return here and basically do nothing, not even change the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might, but it won't save any time. Thanks for the feedback, but I won't do it now. |
||
{ | ||
Write-Output "... $message" | ||
} | ||
} | ||
finally { | ||
$Host.UI.RawUI.ForegroundColor = $currentColor | ||
} | ||
$Host.UI.RawUI.ForegroundColor = $currentColor | ||
} | ||
|
||
function Write-VerboseLog([string] $message) | ||
|
@@ -124,12 +136,12 @@ function Install-DotNetCli | |
Get-ChildItem "Env:\dotnet_*" | ||
|
||
"`n`n---- x64 dotnet" | ||
& "$env:DOTNET_ROOT\dotnet.exe" --info | ||
Invoke-Exe "$env:DOTNET_ROOT\dotnet.exe" "--info" | ||
|
||
"`n`n---- x86 dotnet" | ||
# avoid erroring out because we don't have the sdk for x86 that global.json requires | ||
try { | ||
& "${env:DOTNET_ROOT(x86)}\dotnet.exe" --info 2> $null | ||
Invoke-Exe "${env:DOTNET_ROOT(x86)}\dotnet.exe" "--info" 2> $null | ||
} catch {} | ||
Write-Log "Install-DotNetCli: Complete. {$(Get-ElapsedTime($timer))}" | ||
} | ||
|
@@ -151,11 +163,8 @@ function Restore-Package | |
$dotnetExe = Get-DotNetPath | ||
|
||
Write-Log ".. .. Restore-Package: Source: $env:TP_ROOT_DIR\src\package\external\external.csproj" | ||
& $dotnetExe restore $env:TP_ROOT_DIR\src\package\external\external.csproj --packages $env:TP_PACKAGES_DIR -v:minimal -warnaserror -p:Version=$TPB_Version -bl:"$env:TP_OUT_DIR\log\$Configuration\external.binlog" | ||
Invoke-Exe $dotnetExe "restore $env:TP_ROOT_DIR\src\package\external\external.csproj --packages $env:TP_PACKAGES_DIR -v:minimal -warnaserror -p:Version=$TPB_Version -bl:""$env:TP_OUT_DIR\log\$Configuration\external.binlog""" | ||
Write-Log ".. .. Restore-Package: Complete." | ||
|
||
Set-ScriptFailedOnError | ||
|
||
Write-Log "Restore-Package: Complete. {$(Get-ElapsedTime($timer))}" | ||
} | ||
|
||
|
@@ -186,14 +195,16 @@ function Get-ElapsedTime([System.Diagnostics.Stopwatch] $timer) | |
|
||
function Set-ScriptFailedOnError | ||
{ | ||
param ($Command, $Arguments) | ||
if ($lastExitCode -eq 0) { | ||
return | ||
} | ||
|
||
if ($FailFast -eq $true) { | ||
Write-Error "Build failed. Stopping as fail fast is set." | ||
Write-Error "Build failed. Stopping as fail fast is set.`nFailed command: $Command $Arguments`nExit code: $LASTEXITCODE" | ||
} | ||
|
||
$script:ScriptFailedCommands += "$Command $Arguments" | ||
$Script:ScriptFailed = $true | ||
} | ||
|
||
|
@@ -203,4 +214,18 @@ function PrintAndExit-OnError([System.String] $output) | |
Write-Error $output | ||
Exit 1 | ||
} | ||
} | ||
|
||
function Invoke-Exe { | ||
param ( | ||
[Parameter(Mandatory)] | ||
[string] $Command, | ||
[string] $Arguments, | ||
[int[]] $IgnoreExitCode | ||
) | ||
Write-Verbose "Invoking: $Command $Arguments" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Provided that my understanding is correct, the only mandatory argument of this function is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. $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. |
||
& $Command ($Arguments -split ' ') | ||
if ($IgnoreExitCode -notcontains $LASTEXITCODE) { | ||
Set-ScriptFailedOnError -Command $Command -Arguments $Arguments | ||
} | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.