-
Notifications
You must be signed in to change notification settings - Fork 903
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
Update MSI installer to prevent upgrades and not partially remove Chocolatey #3290
Conversation
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.
Note, I've only reviewed the chocolatey.en-us.wxl file.
<CustomAction Id="SetHelpersDirectoryPath" Property="HelpersDirectoryPath" Value="[INSTALLDIR]\\helpers" /> | ||
<CustomAction Id="SetRedirectsDirectoryPath" Property="RedirectsDirectoryPath" Value="[INSTALLDIR]\\redirects" /> | ||
<CustomAction Id="SetToolsDirectoryPath" Property="ToolsDirectoryPath" Value="[INSTALLDIR]\\tools" /> | ||
|
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.
Why are you removing these custom actions?
I was under the impression they are required to be available?
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.
These custom actions only fire when Chocolatey is already installed, which only happens on an upgrade, change, repair, or uninstall. None of which we currently do anything with.
<!-- Set these values during the uninstall such that we can remove the folders --> | ||
<Custom Action="SetHelpersDirectoryPath" Before="CostInitialize"><![CDATA[Installed]]></Custom> | ||
<Custom Action="SetRedirectsDirectoryPath" Before="CostInitialize"><![CDATA[Installed]]></Custom> | ||
<Custom Action="SetToolsDirectoryPath" Before="CostInitialize"><![CDATA[Installed]]></Custom> |
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.
Same as above.
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.
These custom actions only fire when Chocolatey is already installed, which only happens on an upgrade, change, repair, or uninstall. None of which we currently do anything with.
<RemoveFile Id="ChocoEXE" On="uninstall" Name="choco.exe" /> | ||
<RemoveFile Id="ChocoIgnore" On="uninstall" Name="choco.exe.ignore" /> | ||
<RemoveFile Id="ChocoManifest" On="uninstall" Name="choco.exe.manifest" /> | ||
<RemoveFile Id="ChocoCredits" On="uninstall" Name="CREDITS.txt" /> | ||
<RemoveFile Id="ChocoLicense" On="uninstall" Name="LICENSE.txt" /> | ||
<util:RemoveFolderEx Id="HelpersDirectory" On="uninstall" Property="HelpersDirectoryPath" /> | ||
<util:RemoveFolderEx Id="RedirectsDirectory" On="uninstall" Property="RedirectsDirectoryPath" /> | ||
<util:RemoveFolderEx Id="ToolsDirectory" On="uninstall" Property="ToolsDirectoryPath" /> |
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.
So we intend to remove the support to uninstall Chocolatey CLI through the MSI?
Or mainly making it a no-op?
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.
It should be a no-op. The goal is to remove from Programs and Features but not actually remove Chocolatey itself in this version. We should notify the user to that @corbob ?
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.
Yeah, in this version we're going to make it a no-op since the previous behaviour doesn't actually remove all of Chocolatey, and instead leaves it in a broken state that doesn't allow reinstallation without manual intervention. I'll be doing up an ADR for this with the reasoning and options considered. The main reason to do a no-op right now is we would rather have a non operative uninstall than one that isn't working and don't want to rush the work of having a proper uninstall.
With regards to notifying the user @pauby, I wasn't finding anything in my searching about changing things, but I just noticed some things in this diff that I think can work, so I'll look into that.
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.
Just two small changes Cory. If the typo with the missing full stop wasn't there I wouldn't have asked for the other one. So technically this isn't my fault 😄
@corbob this will need targeted to the hotfix branch before we can merge. |
Thanks @corbob. Approved this now. |
<String Id="RequiresDotNet48" Overridable="yes">Chocolatey CLI application requires .NET Framework 4.8. Please install the .NET Framework then run this installer again.</String> |
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.
The wording seems a little off here, shouldn't this be:
<String Id="RequiresDotNet48" Overridable="yes">Chocolatey CLI application requires .NET Framework 4.8. Please install the .NET Framework then run this installer again.</String> | |
<String Id="RequiresDotNet48" Overridable="yes">Chocolatey CLI requires .NET Framework 4.8. Please install the .NET Framework then run this installer again.</String> |
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.
Good catch. I had that above but didn't notice it had been missed.
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.
Apologies, fi thought I had copy/pasted the message from the suggestion, but it seems I didn't. I've updated this now.
The MSI previously included logic to remove *some* Chocolatey files. This would leave Chocolatey in a broken state, but not actually removed. This commit removes the entries from the MSI, so an "uninstall" of the MSI is merely a removal of the installer's entry from Add/Remove Programs.
Currently it's not possible to upgrade Chocolatey with the MSI. This commit adds functionality to the MSI that will error instead of blindly allowing an upgrade that doesn't do anything.
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.
Looks good to me, thanks for sorting this out @corbob! 💖
Description Of Changes
Update the MSI settings to prevent an upgrade, and to not remove the Chocolatey files.
Motivation and Context
If you try to upgrade Chocolatey using the MSI with an already installed MSI, the upgrade was attempting to uninstall the old Chocolatey, and did not actually upgrade to the new Chocolatey.
Testing
MSI Uninstall testing
choco --version --allow-unofficial
)choco --version --allow-unofficial
)Upgrade testing
cmd.exe
run:start /wait msiexec /i <path to MSI> /norestart /quiet
msiexec
exits with 1603 (echo %errorlevel%
)Winget output tests
I have built a custom manifest looking to an internal server, and confirmed that it generates a log file. The log file when I ran it came out like this:
Details
This is the display when manually running the MSI:
Operating Systems Testing
Windows 11
Change Types Made
Change Checklist
Related Issue
Fixes #3286