Fix removing batch redirects when uninstalling a package

[jberezanski]

Currently, when a package is uninstalled, instead of removing batch redirect files from $Env:ChocolateyInstall\bin, Chocolatey recreates them (acts as if the package was being installed).

This happens because the "uninstall" switch is being passed to Get-ChocolateyBins incorrectly (bug introduced by commit b12e91b "add error level checking to chocolatey install process (nuget)").

That commit wrapped the Get-ChocolateyBins call with Invoke-ChocolateyFunction, but passed the parameter values wrapped in an array, which works for positional parameters only. The solution is to use a hashtable (a good practice anyway, since the code is more maintainable).

As a side note, apart from (now a bit improved) logging of parameter values for diagnostic purposes, Invoke-ChocolateyFunction does not seem to add any other value, but only introduces complexity, increases possibility of bugs and prevents IntelliSense when editing the scripts. I'd gladly see it removed.

[rismoney]

the purpose of that commit functionality comes from a requirement to be integrated in a non interactive environment. if the functions blindly exit state 0s without exception raising then any automation around chocolatey where failures occur, the errors will be swallowed. the outer lying tool will think everything was honky dory, when in reality failures occured.
its not thrilling to me, but i was seeing exit success when failures occured. I am using puppet with chocolatey.

[rismoney]

all for the hashtables btw :)

[jberezanski]

The current implementation of Invoke-ChocolateyFunction simply catches the exception and rethrows its message. This has no effect on control flow. (It is also, by the way, suboptimal behavior because it loses valuable information, such as the type of the exception; a throw statement without arguments would rethrow the exception without losing information. But that's another matter.)

Your original implementation would indeed use the exit keyword to terminate running the entire script (and return an exit code). In my opinion internal functions should never make such decisions, as they have no knowledge about the context in which they were called. In such scenario Chocolatey had no chances of handling the error in any way or perform any rollback or cleanup actions. Indeed, a later commit by @ferventcoder (54716e0) disabled the mandatory exit because it interfered with the ability to continue installing subsequent packages if one package failed (ideally, that behavior should be controllable, but in any case the authority of deciding whether to stop execution is with chocolatey.ps1, not with a lower-level, internal command).

Having said that, you certainly have a point about returning an error code when something goes wrong. Powershell not doing that is actually a deficiency of powershell.exe when invoked with the -file argument (and documented in about_Language_Keywords). I don't know how Powershell is invoked by Puppet, but in my automation scenario (a Jenkins deployment job) I worked around it by invoking Powershell with my script using the -command argument:

powershell.exe -command "& myscript.ps1 args..."

this correctly returns a nonzero exit code if the script encounters an error. Please also note that some ("non-terminating") errors are subject to $ErrorHandlingPreference; I usually set it to "Stop" (most restrictive) immediately at the beginning of my scripts, so all errors behave like exceptions (e.g. transfer control to the nearest catch clause).

[rismoney]

disabled the mandatory exit because it interfered with the ability to continue installing subsequent packages if one package failed

imho the mandatory exit is the right and default behavior. i agree a switch show control -ignorefailures... i am a big proponent of fail early and stop all things.

right now for better or worse its being called from puppet via chocolatey.cmd. that uses -command

[jberezanski]

My point is, with an exit deep inside an internal function, there would be no way to implement that switch. Also, when an error is encountered during the installation, there is usually some cleanup to be performed, such as deleting the failed package from $Env:ChocolateyInstall\lib.

I also believe in failing early, but in a controllable fashion. I would not like it, for example, if fopen() called ExitProcess() if it failed to open the file.

[rismoney]

agree with you 100%. Inserting proper exception management is needed. My short term goal was selfish, get the error out. Improvement is def needed and i think choco has longer term plans to go c# which will probably help resiliency overall.

[batzen]

I too think that Invoke-ChocolateyFunction should be removed. It really adds zero value at all and only introduces diffucult to find bugs.

Are there any plans merging the fixes jberezanski made? It's really annoying that uninstalling something does not remove the batch redirects.

[rismoney]

ok someone submit pr and make sure non exit 0 is bubbled all the way back out to cmd/posh on any failure.

[batzen]

I don't want to argue and am not that familiar with powershell, but when i look at the code:

function Invoke-ChocolateyFunction ($ChocoFunction,$paramlist) {
  try {
    Write-Debug "Invoke-ChocolateyFunction is calling: `$ChocoFunction='$ChocoFunction'|`@paramlist='@paramlist'"
    invoke-expression "$ChocoFunction @paramlist;"
  }
  #catch {Write-Host $_.exception.message -BackgroundColor Red -ForegroundColor White ;exit 1}
  catch {
    Write-Debug "Caught `'$_`'"
    throw "$($_.Exception.Message)"
  }
}

It seems obvious to me that this is just a try{...}catch{rethrow}. The code responsible for providing a proper exit code is commented out.
Am i wrong, or does everyone agree that this function adds zero value in it's current state?

[jberezanski]

Obsoleted by #446. Now that this works, I found a new issue concerning shimgen-generated exes - #449.