-
Notifications
You must be signed in to change notification settings - Fork 123
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
Initial RFC Native Command Exit Error #88
Initial RFC Native Command Exit Error #88
Conversation
follow-up to comittee review of pull request #3523 for issue #3415
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.
I like the simple proposal.
Any special casing for "tested" positions feels magic, seems like it would trip over newcomers and seems to just be legacy BC decisions from bourne carried over.
Anything that can be currently done should still be possible by wrapping a command in a try
/catch
. Things that come to my mind:
$LASTEXITCODE
can still be inspected in thecatch
(or from the error object?)- Is there a way to get the stdout/stderr string inside the
catch
? E.g. from the error object$_.Exception.stderr
/$_.Exception.stdout
?
As a fallback, it would always be possible to change ErrorActionPreference
for just one command and resetting it.
Another thing that I believe would be very important: To bash you can directly launch bash with bash -e
. E.g. CircleCI uses bash -eo pipefail
as the default shell. It would be great if pwsh
had a similar option (like we already have -ExecutionPolicy
), so I could e.g. set the the default shell in Circle to pwsh -ErrorActionPreference Stop -IncludeNativeCommandInErrorActionPreference
.
when a native command exits with a non-zero exit code, similar to | ||
the `set -e` option in bourne type shells. | ||
|
||
The `$PSIncludeNativeCommandInErrorActionPreference` preference |
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.
That's a hella long variable name
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.
Maybe $NativeErrorInclusionPreference
? or $NativeErrorPreference
? Since it's effectively an enum, the value could elucidate the effect: $NativeErrorPreference = 'IncludeInError'
or something
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.
Maybe $NonZeroExitCodeActionPreference
? It's a bit more self-explanatory.
|
||
The reported error record should be created with the following details: | ||
- exception: `ExitException`, with the exit code of the failed command. | ||
- error id: `"Program {0} failed with exit code {1}"`, with the command |
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.
I think it would be good to also include the STDERR of the command in the error message, as that is usually what would tell a user what went wrong
@felixfbecker, while your suggestions show the best intentions, I think they demand additional discussion. Requirement to publish process' stderr and stdout in the exception object will effectively require PowerShell to redirect standard streams and buffer output for each native command invocation when the new mode is active. As far as I understand the underlying machinery, that will be a huge memory consumption, performance and usability hit. |
Yeah, that's a good point. I was just thinking of how NodeJS |
@PowerShell/powershell-committee reviewed this and propose the following changes and we'll continue discussing desired behavior
|
@mopadden can you review the comments and update the RFC please? Thanks! |
Hi thanks for all your comments. I will study this a bit further, but this is my feedback so far: The revision seems to be equivalent to:
Would it be possible to add some notes to help explain these changes? |
To integrate native commands with PowerShell exception handing, there needs to be a simple way to specify on a command-by-command basis whether or not a non-zero exit code is an exception. This is an essential component of the operation of bash 'set -e'. A more PowerShell-y approach to this might be to bridge the gap by having the native command pipeline output a specific object sub-type. Pipeline execution would run all native commands in the pipeline until one gives a non-zero exit code, and create this object from the result of the last native command which was run by the pipeline. The type would define:
|
I wonder if a |
@SteveL-MSFT perhaps matching it to a modified version of the |
We had a good conversation with @PowerShell/powershell-committee today, and many of us agreed that However, we think it might be a good idea to have some kind of configuration store that can modify the execution of specific native executables (in addition to a global variable that would be overridden by these specific values). These values would allow for more than just setting what to do with stderr, but could also deal with exit codes, or even automatically pre-pending Ideally, this would be a community project where folks could contribute and maintain a "default" config file. Thoughts on this? |
A configuration stores is an interesting idea but it seems to be solving a different question, defining behaviour per exe not per command invocation. The idea behind this proposal is to make it easier to write robust scripts by throwing an exception for "unchecked" non-zero exit codes from a native command pipeline. This requires a mechanism for explicitly checking exit codes on native command pipeline invocations, so that PowerShell knows if the exit code has been checked and therefore whether or not to throw an exception for a non-zero code. A hypothetical Invoke-nativecommand might be a useful thought experiment to consider what switches would be needed to support the desired functionality on a native command pipeline. Perhaps native commands could be defined so that they are run either implicitly or explicitly by invoke-nativecommand, but where invoke-nativecommand is run implictly, it's options are taken from corresponding environment variables as "defaults" |
@SteveL-MSFT @joeyaiello Given that we feel that requiring the use of a cmdlet for native invocation is undesirable, perhaps what we need is an in-between approach, such as perhaps some specialised parameter parsing for native commands? In other words you could invoke a native executable with some slightly modified syntax to determine what happens when it has a non-zero exit code, or what counts as a "success" exit code in the given situation, etc. PS> net use * "\\server\my media" /persistent:no %@{ SuccessExitCodes = @( 0 ); FailAction = 'Stop' } Obviously, the exact syntax is up for debate but I feel like allowing users to just throw on a couple extra parameters, or perhaps set a temporary actionpref variable for the next native command invocation is potentially a good avenue to explore. 🙂 |
I don't really understand how this conversation suddenly switched to |
@felixfbecker I believe the feedback was that native commands don't all follow conventions that stderr is for errors or that exit code != 0 means error. So it might make sense to have it on a per command or per invocation basis. However, it would also seem that basic |
@SteveL-MSFT Thanks for the explanation of the motivation behind the global configuration stores. As you say treating a configuration store for native commands that don't follow conventions as a separate dependent issue to 'set -e' seems like a good way forward. @felixbecker. The reason for pursuing a discussion on Invoke-NativeCommand was to explore alternatives to changing the behavior of conditionals which seemed a bit too much like magic. The expected mode of use for this PR as written is with $NativeCommandPipeFailPreference=“stop” and $NativeCommandExceptionPreference “stop”. In this mode, native command exit-code = 0 means failure; which is promoted to an exception by "set -e" mode in a context where the success of the command is not tested. This is intended to be an exact analog to bash 'set -e' and 'set -o pipefail'. |
@mpadden what is "in a context where the success of the command is not tested" in PowerShell? In bash you test exit codes within |
@felixfbecker true, but what conditionals like if actually do if you say for example if(gci), is convert the output to a boolean by taking all non-empty output to be true and empty output to be false. |
@felixbecker "in a context where the success of the command is not tested" means contexts which do not send the output to a pipe and do not convert the "result" of a pipeline to a boolean as described above. |
@felixfbecker I believe I explained my concerns with using try/catch for this purpose above. It would make the behavior of native commands which are used to perform tests subject to exception handling rules such as $ErrorActionPreference which will undermine the basic purpose of this change, i.e. making it easier to write robust scripts. |
Thanks for all the discussion, everyone. You raised some very good points about more advanced scenarios, and we can discuss those in the future. However, for the time being, we on the @PowerShell/powershell-committee would like to stick with the plan of record in my comment here. For any of the items where we hadn't yet made a decision, we're going to lean on the side of inaction and do nothing there (for now). Right now, our reasoning is that we want to wait for real user feedback on the more advanced scenarios. So basically all we want to implement now is:
@mopadden does this address your initial problem case? I know we're ignoring some of the more complex scenarios right now, but we'd like to get the base case right before continuing. If yes, are you okay updating your RFC with this plan of record? Alternatively, we can play a more active hand here in updating it for you. Also, are you still interested in implementing the behavior? |
@joeyaiello don't want to say no, but I would be wary of introducing a breaking change to a feature with a cognate purpose like pipeline chaining operators. |
@joeyaiello if you like as a first step I can update the rfc, and see what feedback we get from users of the pipeline operators. |
My feeling on the pipeline chain operators is that they are promised currently to be semantically identical to: cmd1; if ($?) { cmd2 } If Concretely, I wouldn't view making a configuration available that causes native commands to throw as a breaking change to pipeline chain operators given that it's not on by default. If it turns out that that a large contingent of users ends up writing around this in order to get absorptive error semantics, we can easily add a new preference value. Unlike making the operators themselves more complex (as has been suggested in other scenarios), it has the virtue of being configurable with a simple default; if the more complex behaviour turns out to be too magical or edge-cased, the simple option is still available (and is the default). |
@@ -0,0 +1,236 @@ | |||
--- | |||
RFC: RFCNNNN-RFC-Native-Command-Exit-Errors | |||
Author: Micheal Padden |
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.
@mopadden is this the correct spelling of your name? If you'd prefer to use your GitHub moniker or similar, that might be acceptable too
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.
Yes, that's me Micheal Padden. Is it preferrable to use the name or the github moniker?
@rjmholt I'm not sure I follow the argument about pipeline chain operators. I'm not proposing that a throw be swallowed. Native commands can also throw, and this exception would not be swallowed by this change whether a pipeline operator is involved or not. It may be pedantically correct to say that the fact that it is off by default means it is not a breaking change. The point I was making is that the use cases are congruent so if you switch this on you definitely won't want it to break the pipeline chain operators for native commands, which it will. |
In considering the effect on pipeline chaining, it might be useful to consider in what circumstances the third code snippet below will give a different result to the other two. |
Co-Authored-By: Robert Holt <[email protected]>
In discussing with @rjmholt, @JamesWTruher, @daxian-dbw, and @SteveL-MSFT, our intent is that the cmd1; if($?) { cmd2 } In your first and third examples, if Pipeline chain operators have little to do with the third example here. Could you elaborate on more on why you think the third example is relevant? In any case, we have quorum to move forward with this plan as an experimental feature. It'd be awesome if you could update this RFC to reflect that plan, but we're happy to push some commits into your branch as well. We're also open to more feedback as we develop the feature and get new feedback on its usage. |
Thanks for considering the difference between the examples. If we broaden the explanation to include cmdlets, I hope it will expose the problem in Powershell's treatment of native commands vs cmdlets which underlies this issue. Looking at the broader case, cmd2 would be executed in the first but not third example where the exit status of cmd1 is true, but the result of cmd1 is not truthy, i.e. because the converting the output to boolean yields $false. I'm open to correction here, but I believe that this is not a scenario that should reasonably occur with cmdlets, because powershell cmdlets follow a model of adding an object to the output stream on success and not doing so on failure. In practice, the scenario will only apply to native commands, because it will not normally conform to this powershell convention. The original RFC considers resolving this issue by redefining "truthiness" for native commands as the exit status of the command and not whether or not the output stream contains something. There may be an argument that this would be a potentially breaking change that would invalidate existing code in a way that the proposed approach wouldn't. It would be interesting to see an example of this. |
I did some work on updating the RFC, but it has become rather complicated to explain the change as a result of dividing up the standard bash mechanism into different "scenarios" where parts of the core functionality are treated as "advanced" scenarios. |
7f37926
to
e642292
Compare
e642292
to
67eca33
Compare
Have updated the RFC per the plan as I understand it. I didn't add an invocation property on the ExitExeception as I thiink this would be dropped into the ErrorRecord rather than the exception? |
} | ||
``` | ||
|
||
The Bourne again shell provides a `set -eo pipefail` exit code handling mode |
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.
My understanding of -o pipefail
is that it does two things:
- stop in the middle of a pipeline if any native exe has non-zero exit
- preserve exit code so subsequent commands with zero exit code doesn't clobber it
Current PowerShell behavior has subsequent native commands clobber $LastExitCode
today. So I would suggest we don't make any change here. We are just adopting the first behavior above which is if any native exe has non-zero exit and $PSNativeCommandError
= Stop
, then it terminates right away.
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.
@SteveL-MSFT: set -o pipefail
doesn't impact the control flow: the pipeline completes as it normally would; the only difference is that the first command participating in the pipeline that reports a non-zero exit code, if any, determines the pipeline's overall exit code; otherwise, it is the last command (which otherwise alone determines the exit code).
The only way for a pipeline in a POSIX-like shell to terminate prematurely is for a participating command to stop reading from the pipeline prematurely, as head
does, for instance (by design, and it therefore itself reports exit code 0
).
Otherwise, the pipeline runs to completion, and the -e
option, if set, then acts on whatever the pipeline's overall exit code is.
@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 PowerShell/PowerShell#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. |
The RFC was much changed. So perhaps it is better to open new PR. |
Closing this PR as it's been made obsolete by the new RFC opened at #261 @mopadden I want to thank you again for filing this RFC initially. It moved forward our thinking on this problem significantly, and we absolutely would not have arrived at the new RFC without the proposal and discussion that took place here. |
follow-up to comittee review of pull request PowerShell/PowerShell#3523
for issue PowerShell/PowerShell#3415
This change is