Casks can fail to load when source file path is nil about brew

[apainintheneck]

The solution for brew bump is to require all loaded casks and formulae to be in installed taps. This pattern shows up in a few other places though and in those cases things should maybe be handled differently. It might not even be a problem if we know the cask will always have this field.

brew --cache

brew/Library/Homebrew/cmd/--cache.rb

Lines 72 to 82 in dfbf269

	cask = formula_or_cask
	ref = cask.loaded_from_api? ? cask.full_name : cask.sourcefile_path
	os_arch_combinations.each do |os, arch|
	next if os == :linux
	SimulateSystem.with(os:, arch:) do
	loaded_cask = Cask::CaskLoader.load(ref)
	print_cask_cache(loaded_cask)
	end
	end

brew fetch

brew/Library/Homebrew/cmd/fetch.rb

Lines 161 to 168 in dfbf269

	cask = formula_or_cask
	ref = cask.loaded_from_api? ? cask.full_name : cask.sourcefile_path
	os_arch_combinations.each do |os, arch|
	next if os == :linux
	SimulateSystem.with(os:, arch:) do
	cask = Cask::CaskLoader.load(ref)

brew audit

brew/Library/Homebrew/dev-cmd/audit.rb

Lines 246 to 255 in dfbf269

	path = cask.sourcefile_path
	errors = os_arch_combinations.flat_map do |os, arch|
	next [] if os == :linux
	SimulateSystem.with(os:, arch:) do
	odebug "Auditing Cask #{cask} on os #{os} and arch #{arch}"
	Cask::Auditor.audit(
	Cask::CaskLoader.load(path),

brew bump-cask-pr

Edit: This already gets checked for casks without associated taps which solves this problem.

brew/Library/Homebrew/dev-cmd/bump-cask-pr.rb

Line 129 in dfbf269

old_contents = File.read(cask.sourcefile_path)

brew bump

brew/Library/Homebrew/dev-cmd/bump.rb

Line 356 in dfbf269

loaded_formula_or_cask = Cask::CaskLoader.load(formula_or_cask.sourcefile_path)

brew livecheck

brew/Library/Homebrew/livecheck/strategy/extract_plist.rb

Line 105 in dfbf269

cask_copy = Cask::CaskLoader.load(cask.sourcefile_path)

[apainintheneck]

This is just one example of the problems that loading from installed cask files can cause.

It seems like we try to load casks using Cask::Cask#sourcefile_path as the ref and that causes problems sometimes because the source file path is nilable depending on which cask loader is used. We use this pattern in a few places to reload the same cask for different os/arch combinations. In this case, it was nil because it was loading the installed cask file instead of the one from the core cask repo.

I almost feel like there are some commands where we don’t want certain cask loaders to be used. Beyond that I wonder if we’d be able to catch this type of error beforehand with more typing in the Cask::Cask and Cask::CaskLoader classes.

-- Rescued from internal team slack

[apainintheneck]

It looks like what happens with a core formula when the core tap is not installed is that we load it from the keg and then use the path the installed formula file when reloading the formula for different architectures.

$ brew audit gawk --verbose --debug
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/bundle clean
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromKegLoader): loading gawk
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::NullLoader): loading gawk
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromKegLoader): loading gawk
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::NullLoader): loading gawk
/usr/bin/env XDG_CACHE_HOME=/Users/kevinrobell/Library/Caches/Homebrew/style /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/bin/ruby -W1 --disable=gems,rubyopt -- /usr/local/Homebrew/Library/Homebrew/utils/rubocop.rb --format json --force-exclusion --parallel --extra-details --except FormulaAuditStrict --config /usr/local/Homebrew/Library/.rubocop.yml /usr/local/opt/gawk/.brew/gawk.rb
==> Auditing Formula gawk on os catalina and arch intel
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromPathLoader): loading /usr/local/opt/gawk/.brew/gawk.rb
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading gettext
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading mpfr
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading readline
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading curl
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading pkg-config
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading brotli
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading cmake
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading libidn2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading libunistring
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading libnghttp2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading libssh2
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading openssl@3
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading ca-certificates
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading openldap
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading rtmpdump
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading zstd
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading lz4
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading xz
/usr/local/Homebrew/Library/Homebrew/brew.rb (Formulary::FromAPILoader): loading gmp

We could easily make casks work the same way but I'm not sure it really captures the desired behavior of most of the dev commands. It'd still be an improvement though since it wouldn't fail loudly. In general, the livecheck, bump* and audit commands assume the core taps are tapped locally.

Maybe we could add some way of turning off installed formula/cask file loading in the parser and then update Formulary.loader_for and Cask::CaskLoader.for to respect that.

[MikeMcQuaid]

It'd still be an improvement though since it wouldn't fail loudly. In general, the livecheck, bump* and audit commands assume the core taps are tapped locally.

Simplest/best case I think would be to make said commands exit (very) early if the relevant formula's tap is not tapped.

Maybe we could add some way of turning off installed formula/cask file loading in the parser and then update Formulary.loader_for and Cask::CaskLoader.for to respect that.

This seems like it might be a lot more complex than the prior option unless we somehow have some commands that need to load only from tap in some places but are fine with API in others.

[apainintheneck]

That makes sense to me. Looking at things a bit more, I see now that the bump-*-pr commands already do that as of a few years ago. That approach should work well.

The fetch and --cache each have a lower likelihood of errors because they default to the name if the cask is loaded from the API which happens when loading from the installed cask JSON. Technically you could still see the same problem with casks installed from third-party taps though.