Uninstall template fails when encountering "bad" registry keys

[dtgm]

choco/src/chocolatey/infrastructure.app/templates/ChocolateyUninstallTemplate.cs

Lines 61 to 63 in 7ef16d6

	[array]$key = Get-ItemProperty -Path @($machine_key6432,$machine_key, $local_key) `
	-ErrorAction SilentlyContinue `
	| ? { $_.DisplayName -like ""$softwareName"" }

[array]$key = Get-ItemProperty -Path @($machine_key6432,$machine_key, $local_key) `
                        -ErrorAction SilentlyContinue `
         | ? { $_.DisplayName -like ""$softwareName"" }

This code will fail if a registry key exists that was written with binary data and incorrectly typed. [array]$key does not resolve issue as Get-ItemProperty will fail before set to variable.

For more details, see dtgm/chocolatey-packages#92 (comment)

[ferventcoder]

What is the proposed solution to this issue? I do think making the template more bulletproof would be helpful.

[dtgm]

This is what I came up with chocolatey-community/chocolatey-packages@f7fe281#diff-1213b3569e3787f648fc1ae040fd00d4

I was thinking of distributing the fix as an extension to keep chocolateyUninstall.ps1 files less complicated as noted at chocolatey-community/chocolatey-packages#214

[dtgm]

So these lines in the template would be replaced with

[array]$key = Get-UninstallRegistryKey -SoftwareName $softwareName

And the package would also have to include in the nuspec:

<dependencies>
  <dependency id="chocolatey-uninstall.extension" />
</dependencies>

[dtgm]

I am looking at adding <dependency id="chocolatey-uninstall.extension" /> to about 500 of my pkgs. And it should be applied to another 500 pkgs in the wild, if not more. Due to the scope of packages this applies to I am reconsidering my proposed solution of having to add the extension to each package. The other options are:

1. Add the extension function as an internal helper. And it looks like some effort has went into this direction already Helper for retrieving installed applications list #396
2. Adding the extension as a dependency to chocolatey package? This would allow not having to have >50% of packages having the same dependency. Also it eases managing the helpers by keeping development outside of chocolatey program.
3. Keep as is, requiring each package using the helpers to include as a dep. Some downsides:

• If many pkgs with this dep are installed, the dep would be checked repeatedly. This would slow choco down a bit until local indexes are developed.
• It would require lots of maintainers to have to add that dependency code to their nuspec file; just one more thing to do. I don't consider this a problem for me as I already wrote the script to add the dep to my nuspecs.

Thoughts? Are there any clear guidelines or recommendations of when helpers should be extensions or internal helpers? Is the compromise of adding a dependency to the chocolatey package a horrible idea for a reason I overlooked?

[ferventcoder]

2 Adding the extension as a dependency to chocolatey package?

Chocolatey has no dependencies, it would just include the helper as part of its framework.

[dtgm]

Right, I was imagining if you ignore everything a "dependency" is for a sec :), it would allow distributing the extension without including it as an internal helper for management outside of chocolatey codebase. Which might be better(?)

When should a helper be external or internal?

[ferventcoder]

@dtgm I tend to think of it as a both argument - you load up the extension and only make it live if the built-in function isn't there (or if it is a known broken version).

[ferventcoder]

And we are starting to get into the reasons I have been hesitant to document the extensions feature.

It's super powerful, but also makes it extremely easy to cut yourself with.

[dtgm]

I'm thinking more of the performance impact of including it in many packages as a dep and the many redundant checks for an updated version. How significant is that?

[ferventcoder]

Included without a version dependency, there is no check IIRC. Please check for sure though.

It would just upgrade as part of cup all (or specific upgrade).

[ferventcoder]

I could be totally wrong about versionless dependencies

[dtgm]

As a basepoint, installing an empty package working from $pwd, if extension dep is included, it adds 0.2 - 0.3 ms to install when dep is already installed. Which is about 1.5x longer than no dependency. So scaling up for 200 pkgs installed using this dep would take cumulatively, 1 min longer.

Summary
measure-command {choco install test-pkg -s "$pwd" -y}
~350 ms -- no dep
~550 ms -- with dep, dep not installed
~850 ms -- with dep, dep already installed

[dtgm]

test-pkg uninstalled between tests. Times didn't deviate much between tests.

[ferventcoder]

Also subject to issues with chocolatey-community/chocolatey-packages#222

[ferventcoder]

Merged into stable at 91db949

[ferventcoder]

https://chocolatey.org/docs/helpers-get-uninstall-registry-key

[ferventcoder]

Needs a small fixup, but 👍