Standardize our Powershell code (#72423)

* Standardize our Powershell code * Apply suggestions from code review Co-authored-by: Jan Jones <jan.jones.cz@gmail.com> --------- Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>
dotnet · Mar 8, 2024 · c625ed1 · c625ed1
1 parent c177e34
commit c625ed1
Show file tree

Hide file tree

Showing 14 changed files with 103 additions and 159 deletions.
diff --git a/azure-pipelines.yml b/azure-pipelines.yml
@@ -385,33 +385,22 @@ stages:
     steps:
       - template: eng/pipelines/checkout-windows-task.yml
 
-      - task: PowerShell@2
+      - powershell: eng/build.ps1 -configuration Release -prepareMachine -ci -restore -binaryLogName Restore.binlog 
         displayName: Restore
-        inputs:
-          pwsh: true
-          filePath: eng/build.ps1
-          arguments: -configuration Release -prepareMachine -ci -restore -binaryLogName Restore.binlog 
 
-      - task: PowerShell@2
+      - powershell: eng/build.ps1 -configuration Release -prepareMachine -ci -build -pack -publish -sign -binaryLogName Build.binlog
         displayName: Build
-        inputs:
-          pwsh: true
-          filePath: eng/build.ps1
-          arguments: -configuration Release -prepareMachine -ci -build -pack -publish -sign -binaryLogName Build.binlog
 
       - script: $(Build.SourcesDirectory)\artifacts\bin\BuildBoss\Release\net472\BuildBoss.exe  -r "$(Build.SourcesDirectory)/" -c Release -p Roslyn.sln
         displayName: Validate Build Artifacts
 
-      - pwsh: |
-          ./eng/validate-rules-missing-documentation.ps1 -ci
+      - powershell: eng/validate-rules-missing-documentation.ps1 -ci
         displayName: Validate rules missing documentation
 
-      - pwsh: |
-          ./eng/generate-compiler-code.ps1 -test -configuration Release
+      - powershell: eng/generate-compiler-code.ps1 -test -configuration Release
         displayName: Generate Syntax Files
 
-      - pwsh: |
-          ./eng/validate-code-formatting.ps1 -ci -rootDirectory $(Build.SourcesDirectory)\src -includeDirectories Compilers\CSharp\Portable\Generated\, Compilers\VisualBasic\Portable\Generated\, ExpressionEvaluator\VisualBasic\Source\ResultProvider\Generated\ 
+      - powershell: eng/validate-code-formatting.ps1 -ci -rootDirectory $(Build.SourcesDirectory)\src -includeDirectories Compilers\CSharp\Portable\Generated\, Compilers\VisualBasic\Portable\Generated\, ExpressionEvaluator\VisualBasic\Source\ResultProvider\Generated\ 
         displayName: Validate Generated Syntax Files
         condition: or(ne(variables['Build.Reason'], 'PullRequest'), eq(variables['compilerChange'], 'true'))
 
@@ -434,10 +423,10 @@ stages:
     steps:
       - template: eng/pipelines/checkout-windows-task.yml
 
-      - pwsh: eng/make-bootstrap.ps1 -output $(bootstrapDir)
+      - powershell: eng/make-bootstrap.ps1 -output $(bootstrapDir)
         displayName: Build Bootstrap Compiler
 
-      - pwsh: eng/test-determinism.ps1 -configuration Debug -bootstrapDir $(bootstrapDir)
+      - powershell: eng/test-determinism.ps1 -configuration Debug -bootstrapDir $(bootstrapDir) -ci
         displayName: Build - Validate determinism
 
       - template: eng/pipelines/publish-logs.yml
@@ -479,7 +468,7 @@ stages:
     steps:
       - template: eng/pipelines/checkout-unix-task.yml
 
-      - pwsh: eng/todo-check.ps1
+      - powershell: eng/todo-check.ps1
         displayName: Validate TODO/PROTOTYPE comments are not present
 
   - job: Correctness_Rebuild

diff --git a/docs/contributing/Powershell Guidelines.md b/docs/contributing/Powershell Guidelines.md
@@ -1,123 +1,95 @@
-Powershell Guidelines
-==============
+# Powershell Guidelines
 
-Powershell is primarily used in this repo as the backbone of our infrastructure.  As such 
-Powershell scripts need to be portable, reliable and take advantage of stop on error 
+Powershell is primarily used in this repo as the backbone of our infrastructure.  As such
+Powershell scripts need to be portable, reliable and take advantage of stop on error
 approaches.  The guidelines below are meant to push scripts to this mindset of execution.
 
-# Coding Style
+## Coding Style
 
 1. Opening braces always go at the end of an expression / statement. 
 1. Closing braces always occur on an otherwise empty line.
-1. We use four space indentation.
+1. We use two space indentation.
 1. Use Pascal casing for functions and where possible follow the Verb-Name convention.
 1. Use Camel casing for all other identifier.
-1. Use full command names instead of aliass.  For example `Get-Content` vs. `gc`.  Aliases can be 
-overridden by the environment and hence are not considered portable. 
+1. Use full command names instead of aliases.  For example `Get-Content` vs. `gc`.  Aliases can be
+overridden by the environment and hence are not considered portable.
 
-# General Guidelines
+## General Guidelines
 
-## Body 
+### Script Template
 
-All scripts shall include the following two statements after parameter declarations:
+All Powershell scripts that execute in CI need to conform to the following template.
+
+```powershell
+[CmdletBinding(PositionalBinding=$false)]
+param (
+    [switch]$ci = $false,
+    [string]$configuration = "Debug")
 
-``` powershell
 Set-StrictMode -version 2.0
 $ErrorActionPreference="Stop"
-```
 
-This both forces Powershell into a more strict mode of interepretation and swaps out the 
-"On Error Resume Next" approach for an "On Error Stop" model.  Both of these help with our
-goals of reliability as it makes errors hard stops (usless specifically stated otherwise)
+try { 
+  . (Join-Path $PSScriptRoot "build-utils.ps1")
+  $prepareMachine = $ci
 
-The body of a Powershell script shall be wrapped inside the following try / catch template:
+  # Body of Powershell script goes here
 
-``` powershell
-try { 
-    # Body of Powershell script goes here
+  ExitWithExitCode 0
 }
 catch {
-    Write-Host $_
-    Write-Host $_.Exception
-    exit 1
+  Write-Host $_
+  Write-Host $_.Exception
+  Write-Host $_.ScriptStackTrace
+  ExitWithExitCode 1
 }
 ```
 
-This will force scripts to exit with an error code when an unhandled exception occurs. 
-
-## Parameters
-
-The parameter block shall occur at the top of the script.  Authors should consider disabling
-positional binding:
-
-``` powershell
-[CmdletBinding(PositionalBinding=$false)]
-param (
-    [switch]$test64 = $false,
-    [switch]$testDeterminism = $false)
-```
-
-This helps alert callers to casual typos, which would otherwise go unnoticed, by making it an 
-error. 
+The rationale for these parts are:
 
-If the script is complicated enough that it contains a usage / help display, then the following 
-pattern can be used:
+- `Set-StrictMode`: forces Powershell into a stricter mode of interpretation and
+swaps out the "On Error Resume Next" approach for an "On Error Stop" model. Both of these 
+help with our goals of reliability as it makes errors hard stops (unless specifically stated otherwise)
+- `$prepareMachine = $ci`: is necessary to ensure `ExitWithExitCode` properly exits all
+build processes in CI environments (conforming to arcade guidelines).
+- `ExitWithExitCode`: arcade standard for exiting a script that deals with process management
+- `PositionalBinding=$false`: alerts callers when they have incorrect parameters invoking a
+script
 
-``` powershell
-[CmdletBinding(PositionalBinding=$false)]
-param (
-    [switch]$build = $false, 
-    [string]$msbuildDir = "",
-    [parameter(ValueFromRemainingArguments=$true)] $badArgs)
-
-try {
-    if ($badArgs -ne $null) {
-        Print-Usage 
-        exit 1
-    }
-}
+### Coding Guidelines
 
-```
-
-## Executing windows commands
+Use `Exec-*` functions to execute programs or `dotnet` commands. This adds automatic
+error detection on invocation failure, incorrect parameters, etc ... 
 
-Invoking windows commands should be done via the Exec function.  This adds automatic error detection 
-to the invocation and removes the need for error prone if checking after every command.
-
-``` powershell
+```powershell
 # DO NOT
 & msbuild /v:m /m Roslyn.sln
-# DO 
-Exec-Block { & msbuild /v:m /m Roslyn.sln }
-```
+& dotnet build Roslyn.sln
 
-Note this will not work for the rare Windows commands which use 0 as an exit code on failure.  For 
-example robocopy and corflags.
+# DO
+Exec-Command "msbuild" "/v:m /m Roslyn.sln"
+Exec-DotNet "build Roslyn.sln"
+```
 
-In some cases windows commands need to have their argument list built up dynamically.  When that 
-happens do not use Invoke-Expression to execute the command, instead use Exec-Command.  The former
-does not fail when the windows command fails, can invoke powershell argument parsing and doesn't 
-have a mechanism for echoing output to console.  The Exec-Command uses Process directly and can support
-the major functionality needed.
+Scripts that have many executions of `dotnet` commands can store the `dotnet` command in a variable
+and use `Exec-Command` instead.
 
+Call `Test-LastExitCode` after invoking a powershell script to make sure failure is not ignored.
 
-``` powershell
-$command = "C:\Program Files (x86)\Microsoft Visual Studio\Preview\Dogfood\MSBuild\15.0\Bin\MSBuild.exe"
-$args = "/v:m Roslyn.sln"
-if (...) { 
-    $args += " /fl /flp:v=diag"
-}
+```powershell
 # DO NOT
-Invoke-Expression "& $command $args"
+& eng/make-bootstrap.ps1
+Write-Host "Done with Bootstrap"
+
 # DO
-Exec-Command $command $args
+& eng/make-bootstrap.ps1
+Test-LastExitCode
+Write-Host "Done with Bootstrap"
 ```
 
-## Comarisons with null
 Whenever comparing with `$null` always make sure to put `$null` on the left hand side of the 
 operator. For non-collection types this doesn't really affect behavior. For collection types though
-having a collection on the left hand side changes the meaning of `-ne` and `-eq`. Instead of checking
-for `$null` it will instead compare collection contents.
+having a collection on the left hand side changes the meaning of `-ne` and `-eq`. Instead of checking for `$null` it will instead compare collection contents.
 
 ``` powershell
 # DO NOT
@@ -126,3 +98,11 @@ if ($e -ne $null) { ... }
 if ($null -ne $e) { ... }
 ```
 
+### Powershell vs. pwsh
+
+The Roslyn infrastructure should use `powershell` for execution not `pwsh`. The general .NET infra
+still uses `Powershell` and calls into our scripts. Moving to `pwsh` in our scripts creates errors
+in source and unified build. Until that moves to `pwsh` our scripts need to stay on `Powershell`.
+
+The exception is that our VS Code helper scripts should use `pwsh`. That is not a part of our 
+infrastructure and needs to run cross platform hence `pwsh` is appropriate.
diff --git a/eng/build-utils.ps1 b/eng/build-utils.ps1
@@ -67,26 +67,6 @@ function GetReleasePublishData([string]$releaseName) {
   }
 }
 
-# Handy function for executing a command in powershell and throwing if it 
-# fails.
-#
-# Use this when the full command is known at script authoring time and 
-# doesn't require any dynamic argument build up.  Example:
-#
-#   Exec-Block { & $msbuild Test.proj }
-# 
-# Original sample came from: http://jameskovacs.com/2010/02/25/the-exec-problem/
-function Exec-Block([scriptblock]$cmd) {
-  & $cmd
-
-  # Need to check both of these cases for errors as they represent different items
-  # - $?: did the powershell script block throw an error
-  # - $lastexitcode: did a windows command executed by the script block end in error
-  if ((-not $?) -or ($lastexitcode -ne 0)) {
-    throw "Command failed to execute: $cmd"
-  }
-}
-
 function Exec-CommandCore([string]$command, [string]$commandArgs, [switch]$useConsole = $true, [switch]$echoCommand = $true) {
   if ($echoCommand) {
     Write-Host "$command $commandArgs"
@@ -162,19 +142,6 @@ function Exec-Command([string]$command, [string]$commandArgs, [switch]$useConsol
   Exec-CommandCore -command $command -commandArgs $commandArgs -useConsole:$useConsole -echoCommand:$echoCommand
 }
 
-# Handy function for executing a powershell script in a clean environment with 
-# arguments.  Prefer this over & sourcing a script as it will both use a clean
-# environment and do proper error checking
-# 
-# The -useConsole argument controls if the process should re-use the current
-# console for output or return output as a string
-function Exec-Script([string]$script, [string]$scriptArgs = "", [switch]$useConsole = $true, [switch]$echoCommand = $true) {
-  if ($args -ne "") {
-    throw "Extra arguments passed to Exec-Script: $args"
-  }
-  Exec-CommandCore -command "pwsh" -commandArgs "-noprofile -executionPolicy RemoteSigned -file `"$script`" $scriptArgs" -useConsole:$useConsole -echoCommand:$echoCommand
-}
-
 # Handy function for executing a dotnet command without having to track down the 
 # proper dotnet executable or ensure it's on the path.
 function Exec-DotNet([string]$commandArgs = "", [switch]$useConsole = $true, [switch]$echoCommand = $true) {
@@ -202,6 +169,12 @@ function Ensure-DotnetSdk() {
   throw "Could not find dotnet executable in $dotnetInstallDir"
 }
 
+function Test-LastExitCode() {
+  if ($LASTEXITCODE -ne 0) {
+    throw "Last command failed with exit code $LASTEXITCODE"
+  }
+}
+
 # Walks up the source tree, starting at the given file's directory, and returns a FileInfo object for the first .csproj file it finds, if any.
 function Get-ProjectFile([object]$fileInfo) {
   Push-Location

diff --git a/eng/build.ps1 b/eng/build.ps1
@@ -752,7 +752,8 @@ try {
   if ($bootstrap -and $bootstrapDir -eq "") {
     Write-Host "Building bootstrap Compiler"
     $bootstrapDir = Join-Path (Join-Path $ArtifactsDir "bootstrap") "build"
-    Exec-Script (Join-Path $PSScriptRoot "make-bootstrap.ps1") "-output $bootstrapDir -force -ci:$ci"
+    & eng/make-bootstrap.ps1 -output $bootstrapDir -force -ci:$ci
+    Test-LastExitCode
   }
 
   if ($restore -or $build -or $rebuild -or $pack -or $sign -or $publish) {

diff --git a/eng/generate-compiler-code.cmd b/eng/generate-compiler-code.cmd
@@ -1,3 +1,3 @@
 @echo off
-pwsh -noprofile -executionPolicy RemoteSigned -file "%~dp0\generate-compiler-code.ps1" %* 
+powershell -noprofile -executionPolicy RemoteSigned -file "%~dp0\generate-compiler-code.ps1" %* 
 
diff --git a/eng/make-bootstrap.cmd b/eng/make-bootstrap.cmd
@@ -1,2 +1,2 @@
 @echo off
-pwsh -noprofile -file "%~dp0\make-bootstrap.ps1" %* 
+powershell -noprofile -file "%~dp0\make-bootstrap.ps1" %* 
diff --git a/eng/pipelines/build-bootstrap.yml b/eng/pipelines/build-bootstrap.yml
@@ -7,14 +7,11 @@ parameters:
 steps:
     - template: checkout-windows-task.yml
 
-    - pwsh: |
-        ./eng/make-bootstrap.ps1 -ci -toolset ${{parameters.toolset}} -output '$(Build.SourcesDirectory)/artifacts/bootstrap/ci-bootstrap'
-
+    - powershell: eng/make-bootstrap.ps1 -ci -toolset ${{parameters.toolset}} -output '$(Build.SourcesDirectory)/artifacts/bootstrap/ci-bootstrap'
       displayName: Build Bootstrap Compiler
 
-    - pwsh: |
-        ./eng/test-build-correctness.ps1 -ci -configuration Release -enableDumps -bootstrapDir '$(Build.SourcesDirectory)/artifacts/bootstrap/ci-bootstrap'
-      displayName: Build - Validate correctness
+    - powershell: eng/test-build-correctness.ps1 -ci -configuration Release -enableDumps -bootstrapDir '$(Build.SourcesDirectory)/artifacts/bootstrap/ci-bootstrap'
+      displayName: Build - Validate Correctness
 
     - template: publish-logs.yml
       parameters:

diff --git a/eng/test-build-correctness.cmd b/eng/test-build-correctness.cmd
@@ -1,2 +1,2 @@
 @echo off
-pwsh -noprofile -file "%~dp0\test-build-correctness.ps1" %*
+powershell -noprofile -file "%~dp0\test-build-correctness.ps1" %*
diff --git a/eng/test-build-correctness.ps1 b/eng/test-build-correctness.ps1
@@ -45,12 +45,14 @@ try {
 
   if ($bootstrapDir -eq "") {
     Write-Host "Building bootstrap compiler"
-    $bootstrapDir = Join-Path $ArtifactsDir "bootstrap" "correctness"
-    Exec-Script (Join-Path $PSScriptRoot "make-bootstrap.ps1") "-output $bootstrapDir -ci:$ci"
+    $bootstrapDir = Join-Path $ArtifactsDir (Join-Path "bootstrap" "correctness")
+    & eng/make-bootstrap.ps1 -output $bootstrapDir -ci:$ci
+    Test-LastExitCode
   }
 
   Write-Host "Building Roslyn"
-  Exec-Script (Join-Path $PSScriptRoot "build.ps1") "-restore -build -bootstrapDir:$bootstrapDir -ci:$true -prepareMachine:$true -runAnalyzers:$true -configuration:$configuration -pack -binaryLog -useGlobalNuGetCache:$false -warnAsError:$true -properties `"/p:RoslynEnforceCodeStyle=true`""
+  & eng/build.ps1 -restore -build -bootstrapDir:$bootstrapDir -ci:$ci -prepareMachine:$prepareMachine -runAnalyzers:$true -configuration:$configuration -pack -binaryLog -useGlobalNuGetCache:$false -warnAsError:$true -properties:"/p:RoslynEnforceCodeStyle=true"
+  Test-LastExitCode
 
   Subst-TempDir
 
@@ -62,7 +64,8 @@ try {
 
   # Verify the state of our generated syntax files
   Write-Host "Checking generated compiler files"
-  Exec-Script (Join-Path $PSScriptRoot "generate-compiler-code.ps1") "-test -configuration:$configuration"
+  & eng/generate-compiler-code.ps1 -test -configuration:$configuration
+  Test-LastExitCode
   Exec-DotNet "tool run dotnet-format whitespace . --folder --include-generated --include src/Compilers/CSharp/Portable/Generated/ src/Compilers/VisualBasic/Portable/Generated/ src/ExpressionEvaluator/VisualBasic/Source/ResultProvider/Generated/ --verify-no-changes"
   Write-Host ""
 

diff --git a/eng/test-determinism.cmd b/eng/test-determinism.cmd
@@ -1,2 +1,2 @@
 @echo off
-pwsh -noprofile -file "%~dp0\test-determinism.ps1" %*
+powershell -noprofile -file "%~dp0\test-determinism.ps1" %*