chocolatey
Run `chocolateyBeforeModify.ps1` by default on reinstallations
Started on https://github.com/chocolatey-community/chocolatey-coreteampackages/pull/1517:
I just noticed one problem:
- Install this package
- Open Code Insiders
- Force reinstall
It will fail because
chocolateyBeforeModify.ps1does not run before a reinstallation.I'll make it run in
chocolateyInstall.ps1so. By the way, I suggest considering runchocolateyBeforeModify.ps1by default on reinstallations.
This would help both users trying to reinstall a package and also users who had the package installed by other means and wanted to reinstall using Chocolatey.
@ferventcoder I think you misunderstood. I was not asking on how to do it, but to change the default behavior of choco (or at least discuss that).
@felipecrs Force is not recommended as a first step in any kind of situation (and should never be in scripts without a fantastic reason).
Force says "I know what I'm doing, bypass all the stuff/checks/constraints and just do what I ask".
I'm not sure what the before Modify is doing in this case, but I'd almost be assured that most of its logic should probably be in chocolateyInstall.ps1 to handle bringing Code into Chocolatey if you had an existing installation and you were choco installing it with a newer version to make Chocolatey aware of that program. If they are relying on the chocolateyBeforeModify.ps1, they are missing this valid use case scenario and should fix the packaging. HTH
I dropped a comment in your PR, looks like you are handling that use case already!
We have documented behaviors of what runs before modify, and would need to consider the ramifications of adding additional use cases. I'll drop triaging on this and reopen it.
I'm not sure what the before Modify is doing in this case, but I'd almost be assured that most of its logic should probably be in chocolateyInstall.ps1 to handle bringing Code into Chocolatey if you had an existing installation and you were choco installing it with a newer version to make Chocolatey aware of that program. If they are relying on the chocolateyBeforeModify.ps1, they are missing this valid use case scenario and should fix the packaging. HTH
But in fact, it's not that use case. I'm not talking about installing a package that is already installed so choco would be aware of it (for such cases, choco install without --force is enough).
The use case I have is: choco install --force where choco is already aware of the package installed on the system, but the user is intentionally trying to reinstall it. Under the hoods, this is also a modify action, so that's why I propose Chocolatey to run beforeModify.ps1 by default when performing a reinstallation.
If you want a real example:
In vscode-insiders beforeModify.ps1 we check if VSCode is open and then close it, otherwise any further installation would fail.
If running choco install (without --force), we know that this package is not installed so we don't need to check if it is open or not. Good: choco install does not run beforeModify.ps1.
If running choco install --force, I'm explicitly saying: look Chocolatey, I know VSCode exists on my system, but please reinstall it. If beforeModify.ps1 is not run and the VSCode is open, the reinstallation would fail.
I hope this helps.
In vscode-insiders beforeModify.ps1 we check if VSCode is open and then close it, otherwise any further installation would fail.
No, we do not.
That code is located in a helper script that gets called directly from chocolateyInstall.ps1 and chocolateyUninstall.ps1.
The chocolateyBeforeModify.ps1 file was removed in favour of doing it that way.
@ferventcoder that's the point. My proposal on this issue makes us able to get rid of that complexity. Thus, we would not need to have a helper.ps1 and to load it in both install.ps1 and beforeModify.ps1.
I think maybe we should take a step back and come to a common understanding of the before modify, its behaviors, and our vision of why it was introduced and what it should be used for.
Vision
The reason before modify was created is to ensure that there is no locking of files inside the packaging folder itself, which is necessary if you are doing something like running a service out of those folders. In 98% of use cases for packages (like where they install the software into Programs and Features and that needs to be shut down if it is running), that should all be handled in chocolateyInstall.ps1 for installs/upgrades and chocolateyUninstall.ps1 for uninstalls. That encapsulates all the logic in one place, except when you need to introduce added complexity with the before modify.
Considerations and Behaviors
A few additional considerations about before modify and why its use should remain limited:
- Before Modify logic is always run from the currently installed package
- That logic could have been buggy, and there is no chance to correct that as part of an upgrade.
- With those two things considered - If the before modify logic fails/errors, it will never fail the package upgrade/uninstall. This point is especially important and should be restated - if VS code is unable to be shut down by this, it will still move on to run chocolateyInstall.ps1 without any additional considerations. This is unlikely to change (or at least change from being a default behavior).
- A method of ensuring something is shut down that could previously exist outside of Chocolatey still needs to be handled by an installation script as it is an absolutely valid and widely found use case scenario. Another way to litmus test this - can I install it without Chocolatey? If yes, what happens if I then upgrade/install a package that represents the software I already have installed? Should it error or should it handle things appropriately?
More / Closing
As it stands, having the logic in ChocolateyInstall/ChocolateyUninstall is correct for how we've designed Chocolatey and covers all of the scenarios under which packaging could be installed. While it technically duplicates a small bit of code, I don't see this holding additional complexity and I don't feel that what is being duplicated equates to a bad thing.
Thoughts?
@ferventcoder makes super sense. Thank you so much for this great explanation.
Action point:
- To document, because people can't guess such a thing.
When I was introduced to chocolateyBeforeModify.ps1, I didn't know that the chocolateyBeforeModify.ps1 was not intended to be used as a requirement for the modification (since, as you said, the modification happens even when the chocolateyBeforeModify.ps1 fails).
@felipecrs let me know if choco new test doesn't provide the right information inside the generated chocolateyBeforeModify.ps1. I see how you were introduced reading that and see that was suboptimal.
@ferventcoder sadly, it says anything about it:
$ cat .\chocolateybeforemodify.ps1
# This runs in 0.9.10+ before upgrade and uninstall.
# Use this file to do things like stop services prior to upgrade or uninstall.
# NOTE: It is an anti-pattern to call chocolateyUninstall.ps1 from here. If you
# need to uninstall an MSI prior to upgrade, put the functionality in this
# file without calling the uninstall script. Make it idempotent in the
# uninstall script so that it doesn't fail when it is already uninstalled.
# NOTE: For upgrades - like the uninstall script, this script always runs from
# the currently installed version, not from the new upgraded package version.
It even advises doing exactly what you pointed not to do.
I thought we added notes about how it was not needed in 98% of use cases. We'll definitely look to document that better.