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

Do Not Check $LastExitCode - Only error a package install if script errors or set a different exit code when it is specifically set #1000

Closed
ferventcoder opened this issue Oct 5, 2016 · 7 comments

Comments

@ferventcoder
Copy link
Member

ferventcoder commented Oct 5, 2016

This one is a bit tricky, in 0.9.10 we were trying to be more helpful in determining whether the script was a success, but ran into issues instead. So we need to stop looking at $LASTEXITCODE and only look at whether the PowerShell script that ran was successful and whether $env:ChocolateyExitCode has been specifically set.

This is reverting part of #512 - that caused issues - specifically some code that was included in 33b6285.

@ferventcoder ferventcoder added this to the 0.10.3 milestone Oct 5, 2016
@ferventcoder ferventcoder self-assigned this Oct 5, 2016
@ferventcoder ferventcoder changed the title Do Not Check $LastExitCode - Only set 1 if script errors or exit code is specifically set Do Not Check $LastExitCode - Only set exit 1 if script errors or a different exit code when it is specifically set Oct 5, 2016
@ferventcoder
Copy link
Member Author

@DarwinJS FYI

@ferventcoder
Copy link
Member Author

This might be considered a breaking change for a couple folks if they were calling Exit 1

@DarwinJS
Copy link
Contributor

DarwinJS commented Oct 5, 2016

Maybe along with this do an uselastexitcode per package switch, and Choco feature and always report last exit code in the log so it is easy to spot even when being ignored?

What would be challenging is if the design of this means that a packager can't make the package pay attention to lastexitcode if they need it to.

@ferventcoder
Copy link
Member Author

You can as a packager pay attention and capture the exit code of arbirtrary executables you run. If you are running any of choco's functions that run processes, it already performs this for you. If your script exits with an error, this is already covered. The only thing that doesn't work is exit 1 in your scripts.

Also, the only reason I did not finish a release last night was due to adding a feature switch to set the older behavior back on b/c I deemed this a possible breaking change for some folks (who might have been doing things in a non-recommended fashion).

@ferventcoder
Copy link
Member Author

ferventcoder commented Oct 5, 2016

Nothing prevents a packager from saying, I'm going to pay attention to the exit codes from xcopy - this is why I mentioned it for you because I know you ran into this issue and it was very perplexing to determine what was causing the package to fail (exit 2 from xcopy).

@DarwinJS
Copy link
Contributor

DarwinJS commented Oct 5, 2016

Yes, I was going to mention this in my reply - the ability to globally configure the exit code behavior globally is preferable to having to play with each instance individually. Not only is it easier for the packager, but an admin can turn it on for a package they did not author to gain insight into failure conditions that affect only their environment. Glad to hear something was already in the works!

ferventcoder added a commit that referenced this issue Oct 5, 2016
Starting with 33b6285, which was released as part of GH-512 in
0.9.10, we started checking `$LASTEXITCODE` in addition to the script
command success. This meant it offered the ability to capture when a
script exited with `exit 1` and handle that accordingly. However that
was not a recommended scenario for returning errors from scripts.

Checking `$LastExitCode` checks the last executable's exit code when the
script specifically does not call `Exit`. This can lead to very
perplexing failures, such as running a successful xcopy that exits with
2 and seeing package failures without understanding why. Since it is
not typically recommended to call `Exit` to return a value from
PowerShell because of issues with different hosts, it's less of a
concern to only look at explicit failures.
ferventcoder added a commit that referenced this issue Oct 5, 2016
For folks that may need it, allow failing a package again by the last
external command exit code or `exit` from a PowerShell script. Note
that this is not recommended.

To do this, one just needs to flip on the feature
`scriptsCheckLastExitCode`.
ferventcoder added a commit to ferventcoder/choco that referenced this issue Oct 5, 2016
* stable:
  (chocolateyGH-1000) Allow `$LASTEXITCODE` check by feature
  (chocolateyGH-1000) Don't check $LASTEXITCODE by default
  (chocolateyGH-995) Ensure beforeModify runs as first action
  (maint) specs fixup
  (chocolateyGH-996) ISourceRunner.uninstall beforeModify action
  (chocolateyGH-996) beforemodify check before run
  (chocolateyGH-995) rename before_package_upgrade
@ferventcoder
Copy link
Member Author

This will be out in 0.10.3.

@ferventcoder ferventcoder changed the title Do Not Check $LastExitCode - Only set exit 1 if script errors or a different exit code when it is specifically set Do Not Check $LastExitCode - Only error a package install if script errors or set a different exit code when it is specifically set Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants