-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
fix(gitify): add preflight steps to reduce errors during brew upgrade
#173061
Conversation
This is a known and somewhat expected issue. We have changed the way the The other option which was discussed to a certain level, was adding an option to |
Thanks for the speedy reply, @bevanjkay 🙇 In the case of If force quiting the app helps remove/reduce the unexpected errors, then that's a win imho I've also added a postflight script to relaunch the app.
Nice! Yes, this would be cleaner for cask maintainers to opt-in where safe. Even better, if the app could be relaunched afterwards, too |
brew upgrade
brew upgrade
brew upgrade
brew upgrade
preflight do | ||
retries ||= 3 | ||
ohai "Attempting to close Gitify.app to avoid unwanted user intervention" if retries >= 3 | ||
return unless system_command "/usr/bin/pkill", args: ["-f", "/Applications/Gitify.app"] | ||
rescue RuntimeError | ||
sleep 1 | ||
retry unless (retries -= 1).zero? | ||
opoo "Unable to forcibly close Gitify.app" | ||
end |
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.
I'm okay with this in principle, but I think we would be better off adjusting the uninstall quit
dsl to allow for this use case. If we add a preflight
block here that closes the app, it sets a precedent to customise this in other casks. We try to avoid preflight/postflight
blocks as much as possible and are looking to remove them. We should file an issue at github.com/homebrew/brew to track the idea of allowing certain apps to be quit forcibly first.
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.
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.
While discussion on DSL changes continues on Homebrew/brew#17247, would it be possible to move forward with this fix @bevanjkay.
Happy to pivot once the alternate route is 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.
Yeah I guess it's the best option for now. I was hoping for a second opinion from another maintainer, I'll see if I can get one.
brew upgrade
brew upgrade
@bevanjkay - we're planning a new release of Gitify this week. Do you think this can proceed in its current form? |
Important: Do not tick a checkbox if you haven’t performed its action. Honesty is indispensable for a smooth review process.
In the following questions
<cask>
is the token of the cask you're submitting.After making any changes to a cask, existing or new, verify:
brew audit --cask --online <cask>
is error-free.brew style --fix <cask>
reports no offenses.Additionally, if adding a new cask:
brew audit --cask --new <cask>
worked successfully.HOMEBREW_NO_INSTALL_FROM_API=1 brew install --cask <cask>
worked successfully.brew uninstall --cask <cask>
worked successfully.Appreciate any wisdom from the maintainers. 🙏
We've been seeing the error documented at gitify-app/gitify#918 when performing a
brew upgrade
for Gitify.My most recent theory is it's due to the previous
Gitify.app
is left running which causes the resource not found issue.