From 9f2106ea3a7a7913078ad2d113aced7f491bd2d8 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 17:03:42 -0700 Subject: [PATCH 1/8] Change `$ErrorActionPreference` to not affect stderr output --- .../engine/MshCommandRuntime.cs | 53 ++++++++++--------- .../NativeCommandProcessor.Tests.ps1 | 4 ++ test/tools/TestExe/TestExe.cs | 3 ++ 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/System.Management.Automation/engine/MshCommandRuntime.cs b/src/System.Management.Automation/engine/MshCommandRuntime.cs index af4716f3309..625d1cbd400 100644 --- a/src/System.Management.Automation/engine/MshCommandRuntime.cs +++ b/src/System.Management.Automation/engine/MshCommandRuntime.cs @@ -2856,36 +2856,37 @@ internal void _WriteErrorSkipAllowCheck(ErrorRecord errorRecord, ActionPreferenc preference = ActionPreference.Continue; } - switch (preference) + if (!isNativeError) { - case ActionPreference.Stop: - ActionPreferenceStopException e = - new ActionPreferenceStopException( - MyInvocation, - errorRecord, - StringUtil.Format(CommandBaseStrings.ErrorPreferenceStop, - "ErrorActionPreference", - errorRecord.ToString())); - throw ManageException(e); + switch (preference) + { + case ActionPreference.Stop: + ActionPreferenceStopException e = + new ActionPreferenceStopException( + MyInvocation, + errorRecord, + StringUtil.Format(CommandBaseStrings.ErrorPreferenceStop, + "ErrorActionPreference", + errorRecord.ToString())); + throw ManageException(e); + + case ActionPreference.Inquire: + // ignore return value + // this will throw if the user chooses not to continue + lastErrorContinueStatus = InquireHelper( + RuntimeException.RetrieveMessage(errorRecord), + null, + true, // allowYesToAll + false, // allowNoToAll + true, // replaceNoWithHalt + false // hasSecurityImpact + ); + break; + } - case ActionPreference.Inquire: - // ignore return value - // this will throw if the user chooses not to continue - lastErrorContinueStatus = InquireHelper( - RuntimeException.RetrieveMessage(errorRecord), - null, - true, // allowYesToAll - false, // allowNoToAll - true, // replaceNoWithHalt - false // hasSecurityImpact - ); - break; + AppendErrorToVariables(errorRecord); } - // 2005/01/20 Do not write the object to $error if - // ManageException has already done so - AppendErrorToVariables(errorRecord); - // Add this note property and set its value to true for F&O // to decide whether to call WriteErrorLine or WriteLine. // We want errors to print in red in both cases. diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 12b1349f716..648f6079c66 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -140,6 +140,10 @@ Describe "Native Command Processor" -tags "Feature" { [Console]::OutputEncoding = $originalOutputEncoding } } + + It '$ErrorActionPreference does not apply to redirected stderr output' { + pwsh -noprofile -command "`$ErrorActionPreference = 'Stop'; testexe -stderr stop 2>`$null; 'hello'" | Should -BeExactly 'hello' + } } Describe "Open a text file with NativeCommandProcessor" -tags @("Feature", "RequireAdminOnWindows") { diff --git a/test/tools/TestExe/TestExe.cs b/test/tools/TestExe/TestExe.cs index 09606f43e9d..c3c936521b0 100644 --- a/test/tools/TestExe/TestExe.cs +++ b/test/tools/TestExe/TestExe.cs @@ -24,6 +24,9 @@ static int Main(string[] args) // Used to test functionality depending on $LASTEXITCODE, like &&/|| operators Console.WriteLine(args[1]); return int.Parse(args[1]); + case "-stderr": + Console.Error.WriteLine(args[1]); + break; default: Console.WriteLine("Unknown test {0}", args[0]); break; From 5d5e9fec2cbdc77da9250b7ed79c07d097ad71af Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 17:18:33 -0700 Subject: [PATCH 2/8] wrap as ExperimentalFeature --- .../engine/ExperimentalFeature/ExperimentalFeature.cs | 3 +++ src/System.Management.Automation/engine/MshCommandRuntime.cs | 2 +- .../Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs index f6993a03a0a..e44ea07962c 100644 --- a/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs +++ b/src/System.Management.Automation/engine/ExperimentalFeature/ExperimentalFeature.cs @@ -123,6 +123,9 @@ static ExperimentalFeature() new ExperimentalFeature( name: "PSNativePSPathResolution", description: "Convert PSPath to filesystem path, if possible, for native commands"), + new ExperimentalFeature( + name: "PSNotApplyErrorActionToStderr", + description: "Don't have $ErrorActionPreference affect stderr output"), }; EngineExperimentalFeatures = new ReadOnlyCollection(engineFeatures); diff --git a/src/System.Management.Automation/engine/MshCommandRuntime.cs b/src/System.Management.Automation/engine/MshCommandRuntime.cs index 625d1cbd400..88ce2423b7d 100644 --- a/src/System.Management.Automation/engine/MshCommandRuntime.cs +++ b/src/System.Management.Automation/engine/MshCommandRuntime.cs @@ -2856,7 +2856,7 @@ internal void _WriteErrorSkipAllowCheck(ErrorRecord errorRecord, ActionPreferenc preference = ActionPreference.Continue; } - if (!isNativeError) + if (!(ExperimentalFeature.IsEnabled("PSNotApplyErrorActionToStderr") && isNativeError)) { switch (preference) { diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 648f6079c66..38baa3ae7f2 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -141,7 +141,7 @@ Describe "Native Command Processor" -tags "Feature" { } } - It '$ErrorActionPreference does not apply to redirected stderr output' { + It '$ErrorActionPreference does not apply to redirected stderr output' -Skip:(!$EnabledExperimentalFeatures.Contains('PSNotApplyErrorActionToStderr')) { pwsh -noprofile -command "`$ErrorActionPreference = 'Stop'; testexe -stderr stop 2>`$null; 'hello'" | Should -BeExactly 'hello' } } From e487b77a4a3258accde7bd280d9391b73f8a05eb Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 17:21:01 -0700 Subject: [PATCH 3/8] address Joel's feedback --- .../Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 38baa3ae7f2..13b03928a93 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -142,7 +142,7 @@ Describe "Native Command Processor" -tags "Feature" { } It '$ErrorActionPreference does not apply to redirected stderr output' -Skip:(!$EnabledExperimentalFeatures.Contains('PSNotApplyErrorActionToStderr')) { - pwsh -noprofile -command "`$ErrorActionPreference = 'Stop'; testexe -stderr stop 2>`$null; 'hello'" | Should -BeExactly 'hello' + pwsh -noprofile -command '$ErrorActionPreference = "Stop"; testexe -stderr stop 2>$null; "hello"' | Should -BeExactly 'hello' } } From 7a52d08287b622ef7edc1874bfa61f6c67a9586e Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 18:03:04 -0700 Subject: [PATCH 4/8] remove test that is no longer valid as it relied on stderr resulting in an exception --- test/powershell/Host/ConsoleHost.Tests.ps1 | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/powershell/Host/ConsoleHost.Tests.ps1 b/test/powershell/Host/ConsoleHost.Tests.ps1 index d8eec255184..390b461fb90 100644 --- a/test/powershell/Host/ConsoleHost.Tests.ps1 +++ b/test/powershell/Host/ConsoleHost.Tests.ps1 @@ -100,19 +100,6 @@ Describe "ConsoleHost unit tests" -tags "Feature" { { & $powershell -outp blah -comm { $input } } | Should -Throw -ErrorId "IncorrectValueForFormatParameter" } - It "Verify Validate Dollar Error Populated should throw exception" { - $origEA = $ErrorActionPreference - $ErrorActionPreference = "Stop" - $a = 1,2,3 - $e = { - $a | & $powershell -noprofile -command { wgwg-wrwrhqwrhrh35h3h3} - } | Should -Throw -ErrorId "CommandNotFoundException" -PassThru - - $e.ToString() | Should -Match "wgwg-wrwrhqwrhrh35h3h3" - - $ErrorActionPreference = $origEA - } - It "Verify Validate Output Format As Text Explicitly Child Single Shell does not throw" { { "blahblah" | & $powershell -noprofile -out text -com { $input } From 2d4baa92140ca4cf9362a1f86738901ad78e493c Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 19:17:02 -0700 Subject: [PATCH 5/8] Update test to not expect stderr in $error --- .../Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 index 2970f7ae7a9..59702938375 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeStreams.Tests.ps1 @@ -21,8 +21,7 @@ Describe "Native streams behavior with PowerShell" -Tags 'CI' { # this check should be the first one, because $error is a global shared variable It 'should not add records to $error variable' { - # we are keeping existing Windows PS v5.1 behavior for $error variable - $error.Count | Should -Be 9 + $error.Count | Should -Be 0 } It 'uses ErrorRecord object to return stderr output' { From 77d160f613e84005435b3753e5db546f7d6edb2f Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Wed, 5 Aug 2020 19:48:04 -0700 Subject: [PATCH 6/8] Fix test where the double quotes are getting stripped passed to the native command --- .../Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index 13b03928a93..af20a919e36 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -142,7 +142,7 @@ Describe "Native Command Processor" -tags "Feature" { } It '$ErrorActionPreference does not apply to redirected stderr output' -Skip:(!$EnabledExperimentalFeatures.Contains('PSNotApplyErrorActionToStderr')) { - pwsh -noprofile -command '$ErrorActionPreference = "Stop"; testexe -stderr stop 2>$null; "hello"' | Should -BeExactly 'hello' + pwsh -noprofile -command '$ErrorActionPreference = ''Stop''; testexe -stderr stop 2>$null; ''hello''' | Should -BeExactly 'hello' } } From 2bc6a746681da3e6085138141217e2d8477b3b5a Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Thu, 6 Aug 2020 07:59:10 -0700 Subject: [PATCH 7/8] Update test to dump $error making sure it's empty --- .../Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 index af20a919e36..e582f55a044 100644 --- a/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 +++ b/test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1 @@ -142,7 +142,7 @@ Describe "Native Command Processor" -tags "Feature" { } It '$ErrorActionPreference does not apply to redirected stderr output' -Skip:(!$EnabledExperimentalFeatures.Contains('PSNotApplyErrorActionToStderr')) { - pwsh -noprofile -command '$ErrorActionPreference = ''Stop''; testexe -stderr stop 2>$null; ''hello''' | Should -BeExactly 'hello' + pwsh -noprofile -command '$ErrorActionPreference = ''Stop''; testexe -stderr stop 2>$null; ''hello''; $error' | Should -BeExactly 'hello' } } From d8bdfaa281dc1dc1737a717d0845988f54162cc9 Mon Sep 17 00:00:00 2001 From: Steve Lee Date: Thu, 6 Aug 2020 10:53:13 -0700 Subject: [PATCH 8/8] address Dongbo's feedback --- .../engine/MshCommandRuntime.cs | 44 +++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/System.Management.Automation/engine/MshCommandRuntime.cs b/src/System.Management.Automation/engine/MshCommandRuntime.cs index 88ce2423b7d..eea82e1ab65 100644 --- a/src/System.Management.Automation/engine/MshCommandRuntime.cs +++ b/src/System.Management.Automation/engine/MshCommandRuntime.cs @@ -2831,33 +2831,33 @@ internal void _WriteErrorSkipAllowCheck(ErrorRecord errorRecord, ActionPreferenc this.PipelineProcessor.LogExecutionError(_thisCommand.MyInvocation, errorRecord); } - ActionPreference preference = ErrorAction; - if (actionPreference.HasValue) + if (!(ExperimentalFeature.IsEnabled("PSNotApplyErrorActionToStderr") && isNativeError)) { - preference = actionPreference.Value; - } + ActionPreference preference = ErrorAction; + if (actionPreference.HasValue) + { + preference = actionPreference.Value; + } - // No trace of the error in the 'Ignore' case - if (ActionPreference.Ignore == preference) - { - return; // do not write or record to output pipe - } + // No trace of the error in the 'Ignore' case + if (ActionPreference.Ignore == preference) + { + return; // do not write or record to output pipe + } - // 2004/05/26-JonN - // The object is not written in the SilentlyContinue case - if (ActionPreference.SilentlyContinue == preference) - { - AppendErrorToVariables(errorRecord); - return; // do not write to output pipe - } + // 2004/05/26-JonN + // The object is not written in the SilentlyContinue case + if (ActionPreference.SilentlyContinue == preference) + { + AppendErrorToVariables(errorRecord); + return; // do not write to output pipe + } - if (ContinueStatus.YesToAll == lastErrorContinueStatus) - { - preference = ActionPreference.Continue; - } + if (ContinueStatus.YesToAll == lastErrorContinueStatus) + { + preference = ActionPreference.Continue; + } - if (!(ExperimentalFeature.IsEnabled("PSNotApplyErrorActionToStderr") && isNativeError)) - { switch (preference) { case ActionPreference.Stop: