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

PSNativeCommandErrorActionPreference non-zero 0 exit code exception false positive #20034

Closed
5 tasks done
ThomasNieto opened this issue Jul 28, 2023 · 8 comments
Closed
5 tasks done
Assignees
Labels
Resolution-Declined The proposed feature is declined. WG-Engine core PowerShell engine, interpreter, and runtime WG-Reviewed A Working Group has reviewed this and made a recommendation

Comments

@ThomasNieto
Copy link
Contributor

ThomasNieto commented Jul 28, 2023

Prerequisites

Steps to reproduce

There are quite a few programs that return non-zero success exit codes. With the PSNativeCommandErrorActionPreference experimental feature turned on this causes errors to be emitted to the console.

I propose that a variable and/or cmdlet maintain a dictionary of program successful exit codes to be checked at runtime when a program runs a non-zero exit code. The dictionary could also be expanded to include error codes. Imagine a community module(s) with a set of programs and success/error exit codes with descriptions that PowerShell could leverage to return to the user.

Register-ProgramExitCode [-Name] <string> [-ExitCode] <int> [[-Description] <string>] [[-Status] {Success | Error}] [<CommonParameters>]

A current example with brew:

PS /home/thomas> brew
Example usage:
  brew search TEXT|/REGEX/
  brew info [FORMULA|CASK...]
  brew install FORMULA|CASK...
  brew update
  brew upgrade [FORMULA|CASK...]
  brew uninstall FORMULA|CASK...
  brew list [FORMULA|CASK...]

Troubleshooting:
  brew config
  brew doctor
  brew install --verbose --debug FORMULA|CASK

Contributing:
  brew create URL [--no-fetch]
  brew edit [FORMULA|CASK...]

Further help:
  brew commands
  brew help [COMMAND]
  man brew
  https://docs.brew.sh
NativeCommandExitException: Program "brew" ended with non-zero exit code: 1.

Expected behavior

Programs that return successful non-zero exit codes; PowerShell does not write errors.

Actual behavior

Errors are written on all non-zero exit codes.

Error details

Exception             :
    Type        : System.Management.Automation.NativeCommandExitException
    Path        : /home/linuxbrew/.linuxbrew/bin/brew
    ExitCode    : 1
    ProcessId   : 1125
    ErrorRecord :
        Exception             :
            Type    : System.Management.Automation.ParentContainsErrorRecordException
            Message : Program "brew" ended with non-zero exit code: 1.
            HResult : -2146233087
        CategoryInfo          : NotSpecified: (:) [], ParentContainsErrorRecordException
        FullyQualifiedErrorId : ProgramExitedWithNonZeroCode
    Message     : Program "brew" ended with non-zero exit code: 1.
    HResult     : -2146233087
TargetObject          : /home/linuxbrew/.linuxbrew/bin/brew
CategoryInfo          : NotSpecified: (/home/linuxbrew/.linuxbrew/bin/brew:String) [], NativeCommandExitException
FullyQualifiedErrorId : ProgramExitedWithNonZeroCode

Environment data

7.4-preview4

Visuals

No response

@ThomasNieto ThomasNieto added the Needs-Triage The issue is new and needs to be triaged by a work group. label Jul 28, 2023
@mklement0
Copy link
Contributor

As a (largely inconsequential) aside: brew without arguments does result in an error condition: brew considers this invalid syntax, which it signals with exit code 1 and - as a courtesy - outputs its CLI help to stderr. brew -h or brew --help are the expected ways to invoke help (and result in exit code 0).
In the examples below, I'll use test "$(Get-Item nosuch*)" as a (contrived) example instead; the Unix test utility by design sets its exit code to 1 if the test evaluates to false (which isn't a failure condition).
A more realistic example is robocopy.exe, which uses several nonzero exit codes for success conditions - see the discussion in #18944.

An alternative per-call solution - not mutually exclusive with your proposal - is part of the larger Invoke-NativeCommand proposal in #18991 (it also proposes a per-call solution for overriding $PSNativeCommandArgumentPassing); it currently proposes something like
Invoke-NativeCommand -IgnoreExitCode { test "$(Get-Item nosuch*)" }; if ($LASTEXITCODE) { ... }
That is, the assumption is that the caller will inspect the exit code later and take appropriate action.
Conceivably, this could be augmented as follows, in cases where the caller only cares about success vs. failure, not the specific exit code (which with test obviously doesn't make sense):
Invoke-NativeCommand -SuccessExitCode 0, 1 { test "$(Get-Item nosuch*)" }; ...

My personal concerns with respect to a potential Register-ProgramExitCode cmdlet are twofold:

  • While there definitely are high-profile CLIs - notably robocopy - that call for a solution that doesn't conflict with $PSNativeCommandUseErrorActionPreference, my sense is that they are a small percentage overall - most utilities play by the rules (0 for success, nonzero for failure), so a registry of such exit codes may not be worth the effort.
    On the flip side, if maintained at an organization level (rather than via a community module, see next point), I can see the appeal of a set-it-and-forget-it solution (but, as with any preference-variable-like solution, it amounts to invisible state, so that looking at a call in isolation you won't know how its exit code will be handled).

  • While a community effort always has its appeal, entrusting something as critical as error handling to a community-maintained module sounds problematic to me; for instance, users can reasonably argue over which of robocopy's nonzero exit codes should be considered a failure, utilities as a whole may be missing, values may be missing, the registered values can go stale over time, ...

@daxian-dbw daxian-dbw added the WG-Engine core PowerShell engine, interpreter, and runtime label Jul 28, 2023
@ThomasNieto
Copy link
Contributor Author

ThomasNieto commented Jul 29, 2023

I agree that the example provided was not the best and robocopy would have been a better one.

Lets first level set on some common points/facts.

  • Exit code 0 as success and any other value as failure is a best practice
  • There is nothing enforcing exit code best practice and developers can and will do their own thing (robocopy, wusa, etc)
  • $PSNativeCommandUseErrorActionPreference set to $true by default is a breaking change due to a new unhandled exception being thrown. Looking at the RFC it should be disabled by default but might just be enabled for preview?

Now for my views on this feature:

  • Since we know that exit codes values are arbitrary and best practices not enforced, PowerShell cannot assume zero is success and non-zero is a failure.
  • The user should be able to configure this feature on a per executable basis without having to change their execution method i.e. Invoke-NativeCommand
  • If the Register-ProgramExitCode feature was implemented, PowerShell should not ship/enable a default registry of exit codes. That should be up to the user on what they want configured. If a user, community, or official PowerShell module(s) was created with a registry, it shouldn't be a concern since the user would make the active choice to install and import the module with the ability to overwrite any registration. I would equate this to Register-ArgumentCompleter, the command exists for users to add their own completers and community modules like TabExpansionPlusPlus are available. The user is free to install a module with completers and overwrite them as desired.
  • User defined exit codes provide user feedback when running an executable. If the user is able to register a failure exit code with a message or custom error/exception then the PoweShell engine could retrieve the exit code and compare to the registry to get a helpful error message other than non-zero exit code is a failure. Lets say a program test.exe connects to a web service, exit code 5 is invalid credential. The user creates a registration Register-ProgramExitCode -Name test.exe -ExitCode 5 -Description 'Invalid credential'. Now when the user runs the program they get an error with the specific message instead of a generic non-zero exit code exception with a number they have to look up for the meaning.

@mklement0
Copy link
Contributor

mklement0 commented Jul 29, 2023

$PSNativeCommandUseErrorActionPreference set to $true by default is a breaking change

Fortunately that won't be the case: As @SteveL-MSFT said here:

I believe it was mentioned in a separate issue that $PSNativeCommandUseErrorActionPreference is default to $false and only set to $true in 7.4 previews to get feedback.

In other words: the feature will be off by default in the release version - sensibly so.
It will be opt-in, just like Bash's set -e is.

@mklement0
Copy link
Contributor

Register-ProgramExitCode feature was implemented, PowerShell should not ship/enable a default registry of exit codes.

Glad to hear it; that addresses my second concern.

exit codes values are arbitrary and best practices not enforced

I think proper framing is important here:

  • The 0-as-success-nonzero-as-failure is only a convention, yes, but a widely observed one with a long history.
  • Crucially, core features of many shells build on this convention:
    • && and || in POSIX-compatible shells, cmd.exe, and nowadays also in PowerShell
    • Conditionals in POSIX-compatible shells
    • set -e in Bash, for instance, and soon $PSNativeCommandUseErrorActionPreference in PowerShell

So it should be noted that straying from this convention is ill-advised, because it runs afoul of core shell features, especially in the Unix world.

@mklement0
Copy link
Contributor

mklement0 commented Jul 29, 2023

P.S.: To be clear:

  • I can see the appeal of what you're proposing to handle those programs that don't play by the rules - although there are only a few, and hopefully their number won't grow.
  • The zero-vs.-nonzero convention hast the inherent limitation that only specific error conditions can be differentiated, not success conditions, which is what robocopy does. And, in the case of error conditions, a corresponding error message is usually emitted by the program itself, to stderr.

@SteveL-MSFT SteveL-MSFT added the WG-NeedsReview Needs a review by the labeled Working Group label Oct 2, 2023
@SteveL-MSFT
Copy link
Member

Although PSNativeCommandUseErrorActionPreference is to be stable in 7.4, it does default to $false to preserve backwards compatibility. For robocopy type situations, you would need to write that to disable and re-enable this feature if you intend to use this feature in your script. There is still debate in the WG if having a user settable dictionary is desirable for the longterm.

@rkeithhill
Copy link
Collaborator

The @PowerShell/wg-powershell-engine discussed this and concluded that it's not worth adding a mechanism to register successful non-zero exit codes. As @SteveL-MSFT points out, this feature is not enabled by default. It is "opt-in". And there is a documented mechanism to handle these exception cases in scripts where you have enabled $PSNativeCommandUseErrorActionPreference. From the about_Preference_Variables docs:

# Some native commands, like robocopy use non-zero exit codes to represent
# information other than errors. In these cases, you can temporarily disable
# the behavior and prevent non-zero exit codes from issuing errors.

$definedPreference = $PSNativeCommandUseErrorActionPreference
$PSNativeCommandUseErrorActionPreference = $false
robocopy.exe D:\reports\operational "\\reporting\ops" CY2022Q4.md
$robocopyExitCode = $LASTEXITCODE
if ($robocopyExitCode -gt 8) {
    throw "robocopy failed with exit code $robocopyExitCode"
}
$PSNativeCommandUseErrorActionPreference = $definedPreference

Although I suggest we update the docs to take advantage of scriptblock scoping & cleanup e.g.:

# Enable non-zero exit codes to stop script
$ErrorActionPreference = 'Stop'
$PSNativeCommandUseErrorActionPreference = $true

# This should stop the script if a non-zero exit code is returned
dotnet build

# Handle app that returns non-zero "successful" exit code
& {
    $PSNativeCommandUseErrorActionPreference = $false
    robocopy.exe D:\reports\operational "\\reporting\ops" CY2022Q4.md
    if ($LASTEXITCODE -gt 8) {
        throw "robocopy failed with exit code $robocopyExitCode"
    }
}

# This should stop the script if a non-zero exit code is returned
dotnet test

@rkeithhill rkeithhill added WG-Reviewed A Working Group has reviewed this and made a recommendation and removed Needs-Triage The issue is new and needs to be triaged by a work group. WG-NeedsReview Needs a review by the labeled Working Group labels Nov 28, 2023
@rkeithhill rkeithhill added the Resolution-Declined The proposed feature is declined. label Dec 1, 2023
Copy link
Contributor

This issue has been marked as declined and has not had any activity for 1 day. It has been closed for housekeeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Declined The proposed feature is declined. WG-Engine core PowerShell engine, interpreter, and runtime WG-Reviewed A Working Group has reviewed this and made a recommendation
Projects
None yet
Development

No branches or pull requests

6 participants