cmd/untap: fix untapping syntax failure.

[MikeMcQuaid]

If an installed cask is invalid on attempting an untap: it will prevent untapping that cask.

Fix this in two ways: one more specific to untap and one more generally to other commands too:

• specific: only read the actual formulae/casks from the tap we're untapping instead of all of those that are installed
• general: rescue more exceptions in Cask::Caskroom.casks (like we already do for Formula.installed

Fixes #16321

[Bo98]

Looks generally good but one concern I have is shadowed names. It's very much possible for two taps to have the same package name but we are no longer checking here which tap we have actually installed from.

That information is probably not available for casks, but it is for formulae (in the tab).

[MikeMcQuaid]

Looks generally good but one concern I have is shadowed names. It's very much possible for two taps to have the same package name but we are no longer checking here which tap we have actually installed from.

@Bo98 Sorry, didn't see this before auto-merge kicked in.

Looking again at the implementation: I think this is handled/should be fine the name shadowing will only result in e.g. installed_formula_names.include? being more liberal than it should be but then this will be caught by Formulary.factory("#{tap.name}/#{formula_name}").installed? being false.

If there's a missing case here (particularly if overly liberal rather than conservative): will happily open a follow-up PR!

[Bo98]

Oh right, yeah I think I misread it sorry! There's two installed checks yeah - the initial list and then the second real-formula check.

All looks good!

[Bo98]

Or actually, I think any_version_installed? doesn't check the tap:

brew/Library/Homebrew/formula.rb

Lines 617 to 619 in bd378a7

	def any_version_installed?
	installed_prefixes.any? { |keg| (keg/Tab::FILENAME).file? }
	end

I'm cautious about changing any_version_installed? so maybe best to adjust logic on the untap side for now.

[MikeMcQuaid]

Or actually, I think any_version_installed? doesn't check the tap:

It doesn't but: the formula load has already explicitly loaded from the tap.

[Bo98]

To demonstrate what I mean:

$ brew ruby -rformula -e 'puts Formula["homebrew/core/openssl@3"].any_version_installed?'    
true

$ cat >$(brew --repo bo98/experimental)/Formula/[email protected] <<EOS                       
class OpensslAT3 < Formula
  url "abc"
  version "3"
end
EOS

$ brew ruby -rformula -e 'puts Formula["bo98/experimental/openssl@3"].any_version_installed?'
true

$ jq '.source.tap' $(brew --prefix openssl@3)/INSTALL_RECEIPT.json
"homebrew/core"

Instead of formula.any_version_installed? we could do formula.installed_kegs.any? { |keg| keg.tab.tap == formula.tap }

$ brew ruby -rformula -e 'f = Formula["homebrew/core/openssl@3"]; puts f.installed_kegs.any? { |keg| keg.tab.tap == f.tap }'      
true

$ brew ruby -rformula -e 'f = Formula["bo98/experimental/openssl@3"]; puts f.installed_kegs.any? { |keg| keg.tab.tap == f.tap }'
false

[MikeMcQuaid]

@Bo98 Thanks, that helps. Can you reproduce a similar issue/fix with casks? It looks like they don't store taps in their metadata at all.

[Bo98]

The same issue likely applies to casks, but unfortunately the information is not there to sufficiently fix it if installed from Ruby (JSON seems to have more info). We should start storing tap metadata for Ruby installs - this is not the first time I've noticed tap information has been lacking there.

Casks in third-party taps has been somewhat lacking and undertested in general. I've noticed several things hardcoded to homebrew/cask* that really shouldn't be (e.g. https://github.com/Homebrew/brew/pull/16252/files#diff-05ec45190dab3db11df80b2c154648c8023837fa34917dce9d1bcde0acf9c005L644). Most third-party binary packages I see just use formulae instead.

[MikeMcQuaid]

@Bo98 cool, thanks, opened #16331