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

cask/uninstall: skip quit/signal directives when upgrading or reinstalling #16507

Merged
merged 1 commit into from Jan 21, 2024

Conversation

bevanjkay
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

To ensure that a fresh copy of an application is installed correctly, and that the previous copy is removed entirely, when brew upgrade or brew reinstall is called, we first uninstall the prior version of the application.

This can have a side-effect of being destructive in that a running application can be closed using SIGTERM (or similar commands) in a forceful manner without an user confirmation. This is the reasonably expected behaviour when running brew uninstall as the user is explicitly asking for the application to be removed.

However, in cases where the user is running brew upgrade it may not be anticipated that the application will be forcefully terminated, potentially resulting in the loss of unsaved work.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @bevanjkay. This makes a lot of sense to me. I wonder if there's any other stanzas this may apply to? I've personally hit a situation in the past where I wanted this to apply to a script but it may need to be opt-in for that case.

@MikeMcQuaid MikeMcQuaid requested review from a team and krehel January 19, 2024 14:55
Copy link
Member

@p-linnane p-linnane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me for the most part.

I will note that there are applications like Google Chrome that will work fine for a while without quitting them after running brew upgrade. Later on all tabs start crashing until you close out the program and reopen it. Probably more of an edge case though.

@bevanjkay
Copy link
Member Author

@p-linnane
There's definitely some other cases like google-drive that upgrade a bit cleaner when they are quit, however it requires you to manually reopen to start it up again.

As a user, I think it's worse that you lose progress in an online form you're filling out in your browser, than having to close and reopen after a few minutes.

But it's not a clear-cut resolution either way.

@bevanjkay
Copy link
Member Author

Great work @bevanjkay. This makes a lot of sense to me. I wonder if there's any other stanzas this may apply to? I've personally hit a situation in the past where I wanted this to apply to a script but it may need to be opt-in for that case.

When I checked list, quit and signal were the two that made most sense to me to apply globally.

If we allowed a preference to skip additional on upgrade/reinstall I expect it would need to be on a per command level, not an environment variable. Otherwise maybe changing the uninstall stanza DSL, to codify directives as being skipping on uninstall/reinstall.

I'm not sure it is worth the extra effort at this stage, unless there are currently specific examples where this is problematic.


In the case of uninstall scripts often being overzealous (vendors often clear settings etc... because they are designed for an actual uninstall/cleanup), we usually skip them and use our own stanzas to replicate.

We have an issue open somewhere to allow these to be run from zap as they're currently broken. Hopefully can get this off the back burner soon.

@MikeMcQuaid
Copy link
Member

There's definitely some other cases like google-drive that upgrade a bit cleaner when they are quit, however it requires you to manually reopen to start it up again.

I think these should opt-in to something like uninstall: ..., on_upgrade: true stanza or something.

@MikeMcQuaid MikeMcQuaid merged commit 2ed2b33 into Homebrew:master Jan 21, 2024
24 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants