SharpeRAD / Cake.Powershell

Powershell addin for Cake
http://cakebuild.net
MIT License
83 stars 36 forks source link

Powershell Commands that fail do not fail Cake Task #37

Closed tcables closed 6 years ago

tcables commented 6 years ago

Hi, thanks for making such a useful addon for Cake!

If a simple command is run, and it fails, the error is not recognized as an error by the Cake Task:

eg, call "Import-Module" with no parameters will throw an exception in powershell:

C:\> Import-Module ""
Import-Module : Cannot bind argument to parameter 'Name' because it is an empty string.
At line:1 char:15
+ Import-Module ""
+               ~~
    + CategoryInfo          : InvalidData: (:) [Import-Module], ParameterBindingValidationException
    + FullyQualifiedErrorId : ParameterArgumentValidationErrorEmptyStringNotAllowed,Microsoft.PowerShell.Commands.ImportModuleCommand

Cake setup:

Task("continue-after-test123")
    .IsDependentOn("test123")
    .Does(() => 
    {
        Verbose("I should not be here. previous method failed.")
    });

Task("test123")
    .Does(() => 
    {
        StartPowershellScript("Import-Module",
            new PowershellSettings()
            {
                FormatOutput = true,
                LogOutput = true
            }.WithArguments(args =>
            {
                args.Append("ErrorAction", "Stop");
                args.Append("-Verbose");
            }));
    });

Output:

C:\> & ".\build.ps1" -Target continue-after-test123
Preparing to run build script...
Running build script...
Analyzing build script...
Processing build script...
Installing addins...
Compiling build script...

========================================
test123
========================================
Executing task: test123
Executing: Import-Module
Import-Module : Cannot process command because of one or more missing mandatory parameters: Name.
At line:1 char:1
+ Import-Module
+ ~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Import-Module], ParameterBindingException
    + FullyQualifiedErrorId : MissingMandatoryParameter,Microsoft.PowerShell.Commands.ImportModuleCommand

Finished executing task: test123

========================================
continue-after-test123
========================================
Executing task: continue-after-test123
I should not be here. previous method failed.
Finished executing task: continue-after-test123

Task                          Duration
--------------------------------------------------
test123                       00:00:00.6414215
continue-after-test123        00:00:00.0027986
--------------------------------------------------
Total:                        00:00:00.6442201

Even adding ErrorAction Stop will not aid:

C:\> & ".\build.ps1" -Target continue-after-test123
Preparing to run build script...
Running build script...
Analyzing build script...
Processing build script...
Installing addins...
Compiling build script...

========================================
test123
========================================
Executing task: test123
Executing: Import-Module -ErrorAction Stop -Verbose
Import-Module : Cannot process command because of one or more missing mandatory parameters: Name.
At line:1 char:1
+ Import-Module -ErrorAction Stop -Verbose
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidArgument: (:) [Import-Module], ParameterBindingException
    + FullyQualifiedErrorId : MissingMandatoryParameter,Microsoft.PowerShell.Commands.ImportModuleCommand

Finished executing task: test123

========================================
continue-after-test123
========================================
Executing task: continue-after-test123
I should not be here. previous method failed.
Finished executing task: continue-after-test123

Task                          Duration
--------------------------------------------------
test123                       00:00:00.6469561
continue-after-test123        00:00:00.0022057
--------------------------------------------------
Total:                        00:00:00.6491618

but if you use 'throw' directly from a powershell script it will work as expected:

cake:

Task("continue-after-newtest")
    .IsDependentOn("newtest")
    .Does(() => 
    {
        Verbose("I should not be here. previous method failed.");
    });

Task("newtest")
    .Does(() => 
    {
        StartPowershellScript("throw 'This failed!'",
            new PowershellSettings()
            {
                FormatOutput = true,
                LogOutput = true
            });
    });

output:

C:\> & ".\build.ps1" -Target continue-after-newtest
Preparing to run build script...
Running build script...
Analyzing build script...
Processing build script...
Installing addins...
Compiling build script...

========================================
newtest
========================================
Executing task: newtest
Executing: throw 'This failed!'
An error occurred when executing task 'newtest'.
Error: Failed to Execute Powershell Script: System.Management.Automation.RuntimeException: This failed! ---> System.Management.Automation.RuntimeException: This failed!
   --- End of inner exception stack trace ---
   at System.Management.Automation.ExceptionHandlingOps.CheckActionPreference(FunctionContext funcContext, Exception exception)
   at System.Management.Automation.Interpreter.ActionCallInstruction`2.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.EnterTryCatchFinallyInstruction.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.Interpreter.Run(InterpretedFrame frame)
   at System.Management.Automation.Interpreter.LightLambda.RunVoid1[T0](T0 arg0)
   at System.Management.Automation.DlrScriptCommandProcessor.RunClause(Action`1 clause, Object dollarUnderbar, Object inputToProcess)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Management.Automation.Internal.PipelineProcessor.SynchronousExecuteEnumerate(Object input)
   at System.Management.Automation.Runspaces.LocalPipeline.InvokeHelper()
   at System.Management.Automation.Runspaces.LocalPipeline.InvokeThreadProc()

But I should not have to have to wrap all of my calls in a try-catch-throw-rethrow:

string script = @"
try
{
    Import-Module -Verbose -ErrorAction Stop
} catch { throw $_ }
";

StartPowershellScript(script, 
    new PowershellSettings()
    {
        FormatOutput = true,
        LogOutput = true
    });

Thanks!

hmvs commented 6 years ago

Fully agree with you @tcables. Spent quite a time to find out how I can handle errors. So I've created #38 to help others.

But, I want to create an option ThrowIfErrorOccured in PowershellSettings. And method StartPowershellScript will throw exception if it's true.

Would @SharpeRAD accept such PR?

SharpeRAD commented 6 years ago

Throwing errors should be the default action, so no need in an extra setting.

Submit a PR that effectively duplicates this (L303 - L310) for "ErrorRecord" as well as Exceptions.

hmvs commented 6 years ago

@SharpeRAD So, it means that there is a bug. Code you pointed out will not work for errors (not exceptions).

 var exceptions = _pipelineResults.Where(result => result.BaseObject is Exception)
                    .Select(e => e.BaseObject as Exception).ToArray();

Because in case of errors in scripts result.BaseObject will be ErrorRecord not the Exception type. I've checked that yesterday. So we either should introduce new property or it will break builds someone who do not care about errors (not exceptions).

SharpeRAD commented 6 years ago

This thread is talking about being consistent with Powershell and throwing exceptions for errors. I pointed you to those lines as that's the area you would need to change.

Can you name an instance where you would not throw on an error? EG: Cake only has a task method "OnError" it doesn't have a method "OnException" 😄

SharpeRAD commented 6 years ago

Add a setting to be safe, as I shouldn't base everything on my own preference.

hmvs commented 6 years ago

@SharpeRAD Actually for me always throwing exceptions is also preferred choice. But I am afraid that it will break builds for guys for whom errors is not critical right now So we have 3 options: 1) Always throw exceptions on errors 2) Throw exception on errors if ThrowIfErrorOccured option is set. 3) Throw by default but make it optional via option SwallowErrorsInsteadThrow

1st and 3rd options may break builds. (we need to make a statement about it in release notes). but in 3rd option at least anyone can fix it via passing additional parameter. (Names are subject for discussion)

Can you probably make some mention in gitter chat to make a quick poll here via smiles. Heart - option 1 Laugh - option 2 Hooray - option 3

SharpeRAD commented 6 years ago

Resolved in #46