Add postinstall support for brew and cask
#1555
Conversation
MikeMcQuaid
force-pushed
the
postinstall
branch
2 times, most recently
from
5525de6 to
874600a
Compare
| it "is true with {restart_service: true}" do | ||
| expect(described_class.new(formula, restart_service: true).restart_service_needed?).to be(true) | ||
| it "is false with {restart_service: true}" do | ||
| expect(described_class.new(formula, restart_service: true).restart_service_needed?).to be(false) | ||
| end |
There was a problem hiding this comment.
This seems like a breaking change? https://github.com/search?q=%2Fbrew.*restart_service%3A+true%2F+NOT+is%3Afork&type=code.
brew_dumper also outputs restart_service: true.
I checked where the comment originated from, and it seems like it was a typo and true was the always-mode: c97d044.
There was a problem hiding this comment.
@Bo98 It's a behaviour change but not a breaking one. It'll still be run on installs/upgrades, just no longer unconditionally on every brew bundle run. restart_service: true will imply "on change" rather than "every time regardless of change". Also, if the service isn't running, it'll be started regardless.
Given we're doing this behaviour now for services, link and post install it feels like it makes more sense to be consistent here than hold onto previous behaviour.
Of course, if people loudly complain otherwise: we can switch it back.
Thanks for calling this out, though, it warranted further explanation! ❤️
There was a problem hiding this comment.
Ok if it's intentional then sounds good. Just one further question: is brew_dumper still outputting true correct with these changes?
There was a problem hiding this comment.
@Bo98 I've adjusted it to output :changed and updated the README accordingly, good idea.
| return true unless changed? | ||
|
|
||
| puts "Running postinstall for #{@name}." if verbose | ||
| Bundle.system(@postinstall, verbose:) |
There was a problem hiding this comment.
Does this work with the README example where it's all one string rather than separate arguments? Bundle.system intentionally avoids shell execution so may need another method.
There was a problem hiding this comment.
@Bo98 Yup, still works, that's what I tested locally.
This allows `postinstall` to be used to run a command after a formula or cask is installed. While we're here, also adjust the `restart_service` behaviour to correctly match the comment i.e. require `always` to restart every time and otherwise just restart on an install or upgrade of a formula.
MikeMcQuaid
force-pushed
the
postinstall
branch
from
874600a to
0dfc121
Compare
This allows
postinstallto be used to run a command after a formula or cask is installed.While we're here, also adjust the
restart_servicebehaviour to correctly match the comment i.e. requirealwaysto restart every time and otherwise just restart on an install or upgrade of a formula.