cask: gracefully handle removed DSL methods#21624
Conversation
008a6c0 to
8e30488
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Thanks @costajohnt! This is good progress but I'm not sure the right approach yet.
ofail sets a failing error code and just prints an Error: message so it's not really doing much different to opoo's Warning: equivalent.
Instead, this should involve changing some logic in cask_loader.rb (see rescue NameError, ArgumentError, ScriptError, MethodDeprecatedError, MacOSVersion::Error in formulary.rb for similar logic) so that we don't try to read invalid casks at all. For certain operations (e.g. no to info, yes to uninstall) we will also need an override in this case so that we can still uninstall even with no readable cask .rb or .json file.
Happy to continue to give feedback or answer questions, thanks!
bf610ee to
d888d5c
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
@costajohnt Looking much better, thanks! Can you confirm you've manually tested the info, upgrade and uninstall cases and found them all to work as expected now?
Casks installed before `appcast` was removed from the DSL cannot be managed (uninstall/upgrade/info) because loading the cached .rb caskfile triggers `method_missing`, which calls `ofail` and sets `Homebrew.failed = true`. Add a `REMOVED_METHODS` set to the DSL class. When a removed method is encountered, issue a warning (`opoo`) instead of an error (`ofail`) so the cask still loads cleanly and operations like `uninstall` exit with code 0. Fixes Homebrew#21602
Move error handling from DSL method_missing (opoo/ofail) to the loader level. Unknown DSL methods now raise NoMethodError, which Cask#refresh converts to CaskInvalidError. This lets the existing fallback infrastructure in named_args.rb handle it gracefully for uninstall/upgrade operations.
Remove unreachable `if method` guard in DSL#method_missing (Ruby's method_missing always receives a Symbol, which is always truthy). Combine test assertions so the Homebrew.failed check also verifies the exception is raised.
…files When loading an installed caskfile that contains removed DSL methods, wrap the resulting CaskInvalidError as CaskUnreadableError so the existing fallback in named_args.rb can handle it gracefully. This is gated on @from_installed_caskfile to avoid affecting normal cask loading, following the same pattern as formulary.rb.
- respond_to_missing? now returns false to match method_missing raising NoMethodError (previously returned true unconditionally) - Test CaskUnreadableError message includes the removed method name - Add comment explaining rescue ordering dependency between cask.rb and cask_loader.rb
d888d5c to
9ab570c
Compare
|
@MikeMcQuaid Manually tested all three cases against a simulated installed cask with the removed
All three handle the removed DSL method gracefully. No |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Looks good, thanks again @costajohnt!
Summary
Casks installed before the
appcaststanza was removed cannot be managed (brew uninstall,brew upgrade, etc.) because loading the cached.rbcaskfile triggersDSL#method_missing, which callsofailand setsHomebrew.failed = truewithout raising an exception.Approach
Per review feedback, this moves error handling to the loader level following the
formulary.rbrescue pattern:DSL#method_missingnow raisesNoMethodErrorinstead of callingofail. This makes unknown/removed DSL methods a hard error instead of silently poisoning global state.FromPathLoader#loadcatchesCaskInvalidErrorand wraps it asCaskUnreadableErrorwhen@from_installed_caskfileis set. This flag is only set byFromInstalledPathLoader(used for stale cached.rbfiles). Normal cask loading re-raisesCaskInvalidErrorunchanged.Error propagation chain:
NoMethodError→Cask#refreshcatches it → raisesCaskInvalidError→FromPathLoader#loadwraps asCaskUnreadableError(installed only) →named_args.rbfallback (line 410) catches it and creates a minimal cask → uninstall/upgrade succeeds.This follows the same pattern as
formulary.rb's error handling — let invalid definitions fail to load, selectively soften errors for installed caskfiles, and handle the failure at the caller level based on the operation.Before:
brew uninstall --cask qlimagesize→Error: Unexpected method 'appcast'...+Homebrew.failed = true(exit code 1, but cask still loads with corrupted state)After:
brew uninstall --cask qlimagesize→CaskInvalidError→ wrapped asCaskUnreadableErrorfor installed caskfile →named_args.rbfallback creates minimal cask → uninstall succeedsFixes #21602
Test Plan
FromPathLoadertest verifying cask with removed DSL method raisesCaskInvalidErrorFromPathLoadertest verifying installed caskfiles with removed DSL methods raiseCaskUnreadableErrorHomebrew.failedis NOT set after the errorCaskInvalidErrorbrew tests --only cask/dsl,cask/cask_loaderpasses (447 examples, 0 failures)