cask uninstall: opt-in quit/signal on upgrade/reinstall

[toobuntu]

• 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 lgtm (style, typechecking and tests) with your changes locally?

---

Implements #17247 by letting casks opt into running uninstall quit/signal during brew upgrade and brew reinstall (default is to skip them) via quit_on_upgrade and signal_on_upgrade.

Adds fixtures, tests, updates docs, and teaches UninstallMethodsOrder to ignore these metadata keys when checking uninstall method ordering. Default behavior remains unchanged unless a cask opts in.

[bevanjkay]

This is great! I'll give it a proper look through and test a bit later today or tomorrow. But thanks for looking at this @toobuntu

[carlocab]

Having to always write true here (and for :signal_on_upgrade) feels redundant, since this should never be anything other than true (because false is inferred when the key is omitted).

I wonder if there's a nicer/DRYer way to do this.

[MikeMcQuaid]

Thanks @carlocab. I agree but can't figure out a perfect option here. Some ideas:

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: true

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", when: :upgrade

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit_on_upgrade: "my.fancy.package.app"

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall_and_quit "my.fancy.package.app"

[carlocab]

Yea, I don't have great suggestions, hence the ✅. I was thinking something like

uninstall quit: ["my.fancy.package.app", :on_upgrade]

but not a huge fan of it. I'm fine with

uninstall quit_on_upgrade: "my.fancy.package.app"

[toobuntu]

Thanks, both – agreed it's a bit wordy to always write *_on_upgrade: true.

The main reason I went with this approach is that it lets the new behavior piggy‑back on the existing “directive value + boolean metadata” pattern in the uninstall DSL (e.g., script: { executable: ..., sudo: true }). quit_on_upgrade: true / signal_on_upgrade: true follow that convention: the directive value remains the single source of truth for the bundle IDs, and the opt-in boolean just says “also run this during upgrade/reinstall” without adding a new directive name.

Keeping them as separate metadata keys lets AbstractUninstall validate them alongside the existing directive keys, and lets the RuboCop cop simply treat them as “keys to ignore for ordering” without introducing a new directive name or a special case path like uninstall_and_quit.

The more generic keys you suggested seem to want either context‑dependent semantics (“what does this mean next to other directives?”) which felt harder to explain in the Cookbook and in code comments, or a new entry point that would be a bigger DSL/implementation change than this PR set out to make. In particular, a form like uninstall quit_on_upgrade: "..." or uninstall_and_quit "..." would work fine on its own, but it’s then either a full replacement for uninstall quit: (a bigger DSL change, since we now have two ways to direct “quit this app on uninstall-like operations”) or it becomes an upgrade-only directive that tends to duplicate the same bundle IDs in a second place. Keeping the bundle IDs only on quit: and using a small flag to control when it also runs during upgrade/reinstall sidesteps that and keeps the change smaller.

If we want to reduce the visual noise a bit, one incremental tweak would be to treat the presence of quit_on_upgrade / signal_on_upgrade as the opt‑in and not require = true everywhere, while keeping the rest of the wiring as is.

If there’s consensus that one of the alternative wordings is worth the extra refactor, I’m happy to take guidance and adjust, but my goal here was to keep the opt‑in clearly as metadata, avoid duplicating the bundle IDs, and keep the changes small and local enough to be easy to maintain.

[MikeMcQuaid]

Thanks @toobuntu! Rethinking from your message and the code, how about:

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: :quit

and

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: :signal

as the behaviour seems to be mutually exclusive (you cannot both quit and signal at once for the same ID on uninstall/reinstall, right?).

if I'm wrong about being exclusive then I think we're back to:

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: true

which would affect both quitting and signal if either/both are specified.

[toobuntu]

In Library/Homebrew/cask/artifact/uninstall.rb, the uninstall_phase method runs uninstall directives sequentially. Those directive keys are listed in ORDERED_DIRECTIVES in Library/Homebrew/cask/artifact/abstract_uninstall.rb, and the uninstall logic iterates over those keys in their defined order. Nothing prevents both quit and signal from running for the same bundle ID, and there are at least 9 casks in homebrew/cask which do use both quit and signal on the same bundle ID—where signal essentially serves as a fallback if quit didn't suffice.

Distinguishing quit v. signal in the DSL might be an unnecessary complexity, and a single opt-in for both during upgrade/reinstall is likely sufficient. Intuitively, if a cask should quit on upgrade/reinstall, it seems reasonable that it should also signal.

I had intended the two new keys in this PR to be top-level boolean metadata keys alongside the other uninstall directives (though without enforcing an order on them), to keep the diff minimal.

It seems you envision the opt-in flag nested within the quit/signal directive itself, which is both easier to read and more semantically precise. However, it seems that no existing infrastructure supports such per-directive conditional execution flags (e.g.,on_upgrade: true, when: "default" | "always" | "upgrade" | "reinstall", on_upgrade_reinstall: true, etc.). Implementing this would seemingly require refactoring the uninstall DSL parsing, runtime execution logic, and linting cops. Among those options, an on_upgrade: boolean seems simpler to implement than the more extensible when: strings which might be unnecessary given the current needs.

Given that, I have two key queries for direction:

• Should we simplify the DSL by assuming opting to quit on upgrade/reinstall also implies opting to signal, and vice versa, via a single shared flag (e.g., on_upgrade: true)?
• Or is a more substantial refactor to embed conditional flags inside each directive desired?

[carlocab]

Doing something like

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: :quit

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: :signal

Suggested change

	uninstall quit: "my.fancy.package.app", quit_on_upgrade: true
	uninstall quit: "my.fancy.package.app", on_upgrade: [:signal, :quit]

seems ideal here, depending on how much refactoring this really requires.

[MikeMcQuaid]

• Should we simplify the DSL by assuming opting to quit on upgrade/reinstall also implies opting to signal, and vice versa, via a single shared flag (e.g., on_upgrade: true)?

Either this or what @carlocab has proposed above work for me. No strong preference either way.