Casks can fail to load when source file path is nil

apainintheneck commented 6 months ago

`brew doctor` output

Please note that these warnings are just used to help the Homebrew maintainers
with debugging if you file an issue. If everything you use Homebrew for is
working fine: please don't worry or file an issue; just ignore this. Thanks!

Warning: Some installed formulae are deprecated or disabled.
You should find replacements for the following formulae:
  llvm@11

Verification

[X] My "brew doctor output" above says Your system is ready to brew. and am still able to reproduce my issue.
[X] I ran brew update twice and am still able to reproduce my issue.
[X] This issue's title and/or description do not reference a single formula e.g. brew install wget. If they do, open an issue at https://github.com/Homebrew/homebrew-core/issues/new/choose instead.

`brew config` output

HOMEBREW_VERSION: 4.2.20-104-gdfbf269
ORIGIN: https://github.com/Homebrew/brew
HEAD: dfbf26910d514220e6453f6efdd5d9fb4d36c145
Last commit: 11 hours ago
Core tap HEAD: 5c9c84e8d43c9ab364371cef3f9b7473d91d954b
Core tap last commit: 2 hours ago
Core tap JSON: 04 May 01:07 UTC
Core cask tap JSON: 04 May 01:07 UTC
HOMEBREW_PREFIX: /usr/local
HOMEBREW_AUTOREMOVE: set
HOMEBREW_CASK_OPTS: []
HOMEBREW_DEVELOPER: set
HOMEBREW_DISPLAY: /private/tmp/com.apple.launchd.kTNdVTmWzn/org.macosforge.xquartz:0
HOMEBREW_EDITOR: nano
HOMEBREW_MAKE_JOBS: 4
HOMEBREW_SORBET_RUNTIME: set
Homebrew Ruby: 3.1.4 => /usr/local/Homebrew/Library/Homebrew/vendor/portable-ruby/3.1.4/bin/ruby
CPU: quad-core 64-bit ivybridge
Clang: 12.0.0 build 1200
Git: 2.24.3 => /Applications/Xcode.app/Contents/Developer/usr/bin/git
Curl: 7.64.1 => /usr/bin/curl
macOS: 10.15.7-x86_64
CLT: 12.0.0.32.29
Xcode: 12.4

What were you trying to do (and why)?

I was trying to reproduce this cask loading issue: https://github.com/orgs/Homebrew/discussions/4910#discussioncomment-8750211

CC: @johnmcdowell

What happened (include all command output)?

$ brew bump --cask iterm2 --verbose --debug
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/bin/bundle clean
/usr/local/Homebrew/Library/Homebrew/brew.rb (Cask::CaskLoader::FromInstalledPathLoader): loading iterm2
Error: Parameter 'ref': Expected type T.any(Cask::Cask, Pathname, String, URI::Generic), got type NilClass
Caller: /usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:564
Definition: /usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:235
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/configuration.rb:296:in `call_validation_error_handler_default'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/configuration.rb:303:in `call_validation_error_handler'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:300:in `report_error'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:136:in `block in validate_call_skip_block_type'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/signature.rb:213:in `each_args_value_type'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:133:in `validate_call_skip_block_type'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:109:in `block in create_validator_slow_skip_block_type'
/usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:564:in `block in for'
/usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:563:in `each'
/usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:563:in `for'
/usr/local/Homebrew/Library/Homebrew/cask/cask_loader.rb:514:in `load'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:356:in `block (2 levels) in retrieve_versions_by_arch'
/usr/local/Homebrew/Library/Homebrew/simulate_system.rb:27:in `with'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:348:in `block in retrieve_versions_by_arch'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:347:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:347:in `retrieve_versions_by_arch'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:443:in `retrieve_and_display_info_and_open_pr'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:159:in `block in handle_formula_and_casks'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:145:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:145:in `each_with_index'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:145:in `handle_formula_and_casks'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:103:in `block in run'
/usr/local/Homebrew/Library/Homebrew/extend/kernel.rb:520:in `with_env'
/usr/local/Homebrew/Library/Homebrew/api.rb:196:in `with_no_api_env'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/bump.rb:68:in `run'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `bind_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/call_validation.rb:270:in `validate_call'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/3.1.0/gems/sorbet-runtime-0.5.11368/lib/types/private/methods/_methods.rb:277:in `block in _on_method_added'
/usr/local/Homebrew/Library/Homebrew/brew.rb:92:in `<main>'

What did you expect to happen?

It shouldn't cause a type error.

Step-by-step reproduction instructions (by running `brew` commands)

1. Install a cask from `homebrew/cask`.
2. Untap the `homebrew/cask` repo.
3. Run `brew bump --cask INSTALLED_CASK_NAME`.

apainintheneck commented 6 months ago

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 commented 6 months ago

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 bump-cask-pr`~

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

https://github.com/Homebrew/brew/blob/dfbf26910d514220e6453f6efdd5d9fb4d36c145/Library/Homebrew/dev-cmd/bump-cask-pr.rb#L129

`brew bump`

https://github.com/Homebrew/brew/blob/dfbf26910d514220e6453f6efdd5d9fb4d36c145/Library/Homebrew/dev-cmd/bump.rb#L356

`brew livecheck`

https://github.com/Homebrew/brew/blob/dfbf26910d514220e6453f6efdd5d9fb4d36c145/Library/Homebrew/livecheck/strategy/extract_plist.rb#L105

apainintheneck commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

Homebrew / brew