Skip to content
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

Equivalent of bash 'set -e' (#3415) #3523

Closed
wants to merge 1 commit into from

Conversation

mopadden
Copy link

@mopadden mopadden commented Apr 10, 2017

Add preference to throw exception on $lastexitcode!=0, like bash ’set -e’.
Only throw with commands not used in an expression/if/loop value.

$NativeCommandPipeFailPreference=“continue”: like ‘set -e;set +o pipefail’.
An exception is thrown for a native command giving a non-zero exit code,
if it is the the last command in the pipeline.

$NativeCommandPipeFailPreference=“silentlycontinue”: like ‘set +e’.
No exception is thrown on non-zero exit codes. This is the default.

$NativeCommandPipeFailPreference=“stop”: like ‘set -e;set -o pipefail’.
An exception is thrown for a native command giving a non-zero exit code.

With $NativeCommandExceptionPreference "continue" (the default),
a RuntimeException is thrown, handing off to PowerShell error handling.

With $NativeCommandExceptionPreference “stop”, an ExitException is thrown,
terminating the shell regardless of other PowerShell error handling.
Note: If PowerShell exits, the OS is given exit code 1 rather than forwarding the command exit code

to resolve #3415

Add preference to throw exception on $lastexitcode!=0, like bash ’set -e’.
Only throw with commands *not* used in an expression/if/loop value.

$NativeCommandPipeFailPreference=“continue”: like ‘set -e;set +o pipefail’.
An exception is thrown for a native command giving a non-zero exit code,
if it is the the last command in the pipeline.

$NativeCommandPipeFailPreference=“silentlycontinue”: like ‘set +e’.
No exception is thrown on non-zero exit codes. This is the default.

$NativeCommandPipeFailPreference=“stop”: like ‘set -e;set -o pipefail’.
An exception is thrown for a native command giving a non-zero exit code.

With $NativeCommandExceptionPreference "continue" (the default),
a RuntimeException is thrown, handing off to PowerShell error handling.

With $NativeCommandExceptionPreference “stop”, an ExitException is thrown,
terminating the shell regardless of other PowerShell error handling.
@daxian-dbw daxian-dbw added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Apr 11, 2017
@daxian-dbw
Copy link
Member

@PowerShell/powershell-committee could you please review this change?

@HemantMahawar HemantMahawar added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Apr 12, 2017
@HemantMahawar
Copy link
Contributor

@PowerShell/powershell-committee discussed this briefly and has following observations:

  • The name of the variable should start with PS
  • Have 2 variables seems too many and we should consider a single variable

Giving the larger implication of this change, committee feels we should have an RFC for this. The RFC should also consider other approaches as well, such as:

  • Lexical scoping vs dynamic scope
  • Reusing ErrorActionPreference (such as 'StopIncludingNativeCommand' or 'Stop','IncludeNativeCommand')
  • Add a new (boolean) variable (such as $PSIncludeNativeCommandInErrorActionPreference)
  • Update the Set-StrictMode

@mopadden
Copy link
Author

mopadden commented Apr 13, 2017

Thank you for reviewing this. I have begun compiling these details for a draft RFC as described.
Should this be dropped in downstream of github.com/PowerShell/PowerShell-RFC?

@iSazonov
Copy link
Collaborator

@mopadden Yes, PowerShell/PowerShell-RFC is right place. (Please read docs there.)

@PowerShell PowerShell deleted a comment from msftclas Sep 27, 2017
@TravisEz13 TravisEz13 changed the title Equivalent of bash set -e (#3415) Equivalent of bash 'set -e' (#3415) Mar 14, 2018
@daxian-dbw daxian-dbw removed their assignment Mar 14, 2018
@daxian-dbw
Copy link
Member

Waiting on the RFC to be approved.

@stale
Copy link

stale bot commented Apr 13, 2018

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Apr 13, 2018
@stale
Copy link

stale bot commented Apr 23, 2018

This PR has be automatically close because it is stale. If you wish to continue working on the PR, please first update the PR, then reopen it.
Thanks again for your contribution.
Community members are welcome to grab these works.

@stale stale bot closed this Apr 23, 2018
@rkeithhill
Copy link
Collaborator

rkeithhill commented Jun 12, 2019

@SteveL-MSFT In place of this, how about providing the community with a hook ala CommandNotFoundAction. Something like PostNativeCommandInvocationAction that would pass to the handler the exit code of the native command (along with the process args). Then the community could experiment with approaches (functions/scripts/modules) that hook this action to cause a non-zero exit code to fail. Could be as simple as a module with a function like Enable-ThrowOnNativeCommandFailure e.g.:

function Enable-ThrowOnNativeCommandFailure {
    $global:ExecutionContext.InvokeCommand.PostNativeCommandInvocationAction= {
        param($Name, $EventArgs)
        if ($EventArgs.ExitCode) {
            throw "$Name $($EventArgs.Args) failed with exit code $($EventArgs.ExitCode)"
        }
    }
} 

Or maybe instead of throwing directly in the action handler, there could be a property on $EventArgs we could set to indicate it should throw back to the invoker of the native command. Might be nice if we could provide some aspects of the exception message.

I have to say that as someone pushing PowerShell into critical build systems, I've gotten more than a few crusty looks when a PowerShell script has obviously failed when you look at the log file yet the build system (Jenkins, TFS Build) reports success. This has been a real pain point for my dev team especially as other teams are trying to push Python for build scripts (blech!).

@ghost ghost removed the Stale label Jun 12, 2019
@iSazonov
Copy link
Collaborator

@rkeithhill Please discuss in PowerShell/PowerShell-RFC#88. Or better pull new RFC with your proposal - I like it.

@rkeithhill
Copy link
Collaborator

Done PowerShell/PowerShell-RFC#88 (comment)

@SteveL-MSFT
Copy link
Member

@mopadden appears that @PowerShell/powershell-committee had closed on your RFC, can you continue with this PR?

@SteveL-MSFT SteveL-MSFT reopened this Jul 28, 2020
@joeyaiello
Copy link
Contributor

Quoting myself here so we have context:

@PowerShell/powershell-committee agrees that the RFC as currently written is what we believe to be the correct behavior. We can proceed with an experimental feature, and we'd like to use that implementation as a testbed for validating that this is the correct behavior.

@mopadden were you looking to implement this? It looks like you already had a code PR open in #3523. Would you be interested in updating that one? We'd really love for this to land in PowerShell 7.1, but we would need it to be landed (including through review) by end of August in order to make our RC release. If this isn't something you think you can commit to, let us know here, as we could potentially commit engineers from the PS team to work on it.

@ghost ghost added the Review - Needed The PR is being reviewed label Aug 5, 2020
@ghost
Copy link

ghost commented Aug 5, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@SteveL-MSFT
Copy link
Member

Closing this PR as it doesn't match the RFC

@ghost ghost removed the Review - Needed The PR is being reviewed label Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committee-Reviewed PS-Committee has reviewed this and made a decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Equivalent of bash set -e
10 participants