Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More CaskLoader improvements. #16621

Merged
merged 10 commits into from
Feb 9, 2024
2 changes: 1 addition & 1 deletion Library/Homebrew/api/cask.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def download_and_cache_data!
end
private :download_and_cache_data!

sig { returns(Hash) }
sig { returns(T::Hash[String, Hash]) }
def all_casks
unless cache.key?("casks")
json_updated = download_and_cache_data!
Expand Down
71 changes: 27 additions & 44 deletions Library/Homebrew/cask/cask_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,14 @@ class FromPathLoader < AbstractContentLoader
.returns(T.nilable(T.attached_class))
}
def self.try_new(ref, warn: false)
ref = Pathname(ref) if ref.is_a?(String)
return unless ref.is_a?(Pathname)

path = ref
path = case ref
when String
Pathname(ref)
when Pathname
ref
else
return
end

return if %w[.rb .json].exclude?(path.extname)
return unless path.expand_path.exist?
Expand All @@ -110,9 +114,8 @@ def initialize(path, token: T.unsafe(nil))
path = Pathname(path).expand_path

@token = path.basename(path.extname).to_s

@path = path
@tap = Homebrew::API.tap_from_source_download(path)
@tap = Tap.from_path(path) || Homebrew::API.tap_from_source_download(path)
Comment on lines 117 to +118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might even be a touch faster here. The Homebrew::API.tap_from_source_download method is quite slow when it gets called a lot in commands like brew readall. This change in logic means it will get called less in that scenario though I plan on opening a PR soon to move from Pathname#relative_path_from in that method to a regex to speed things up anyway.

end

def load(config:)
Expand Down Expand Up @@ -191,27 +194,8 @@ def load(config:)
end
end

# Loads a cask from a tap path.
class FromTapPathLoader < FromPathLoader
sig {
params(ref: T.any(String, Pathname, Cask, URI::Generic), warn: T::Boolean)
.returns(T.nilable(T.attached_class))
}
def self.try_new(ref, warn: false)
return unless (loader = super)

loader unless Tap.from_path(ref).nil?
end

sig { params(path: T.any(Pathname, String)).void }
def initialize(path)
super(path)
@tap = Tap.from_path(path)
end
end

# Loads a cask from a specific tap.
class FromTapLoader < FromTapPathLoader
class FromTapLoader < FromPathLoader
sig {
params(ref: T.any(String, Pathname, Cask, URI::Generic), warn: T::Boolean)
.returns(T.nilable(T.attached_class))
Expand All @@ -225,9 +209,9 @@ def self.try_new(ref, warn: false)
new("#{tap}/#{token}")
end

sig { params(tapped_name: String).void }
def initialize(tapped_name)
user, repo, token = tapped_name.split("/", 3)
sig { params(tapped_token: String).void }
def initialize(tapped_token)
user, repo, token = tapped_token.split("/", 3)
tap = Tap.fetch(user, repo)
cask = CaskLoader.find_cask_in_tap(token, tap)
super cask
Expand All @@ -249,7 +233,7 @@ class FromDefaultTapLoader < FromTapLoader
def self.try_new(ref, warn: false)
ref = ref.to_s

return unless (match = ref.match(HOMEBREW_MAIN_TAP_CASK_REGEX))
return unless (match = ref.match(HOMEBREW_DEFAULT_TAP_CASK_REGEX))

token = match[:token]

Expand All @@ -261,7 +245,7 @@ def self.try_new(ref, warn: false)
end

# Loads a cask from the default tap path.
class FromDefaultTapPathLoader < FromTapPathLoader
class FromDefaultTapPathLoader < FromPathLoader
sig {
params(ref: T.any(String, Pathname, Cask, URI::Generic), warn: T::Boolean)
.returns(T.nilable(T.attached_class))
Expand Down Expand Up @@ -297,7 +281,11 @@ def load(config:)
class FromAPILoader
include ILoader

attr_reader :token, :path
sig { returns(String) }
attr_reader :token

sig { returns(Pathname) }
attr_reader :path

sig { returns(T.nilable(Hash)) }
attr_reader :from_json
Expand All @@ -309,12 +297,8 @@ class FromAPILoader
def self.try_new(ref, warn: false)
return if Homebrew::EnvConfig.no_install_from_api?
return unless ref.is_a?(String)

return unless (match = ref.match(HOMEBREW_MAIN_TAP_CASK_REGEX))

token = match[:token]

return unless Homebrew::API::Cask.all_casks.key?(token)
return unless (token = ref[HOMEBREW_DEFAULT_TAP_CASK_REGEX, :token])
return if !Homebrew::API::Cask.all_casks.key?(token) && !Homebrew::API::Cask.all_renames.key?(token)

ref = "#{CoreCaskTap.instance}/#{token}"

Expand All @@ -330,7 +314,7 @@ def initialize(token, from_json: T.unsafe(nil))
end

def load(config:)
json_cask = from_json || Homebrew::API::Cask.all_casks[token]
json_cask = from_json || Homebrew::API::Cask.all_casks.fetch(token)

cask_options = {
loaded_from_api: true,
Expand Down Expand Up @@ -480,13 +464,13 @@ def from_h_gsubs(value, appdir)

# Loader which tries loading casks from tap paths, failing
# if the same token exists in multiple taps.
class FromAmbiguousTapPathLoader < FromTapPathLoader
class FromAmbiguousTapPathLoader < FromPathLoader
def self.try_new(ref, warn: false)
case (possible_tap_casks = CaskLoader.tap_paths(ref, warn: warn)).count
case (possible_tap_casks = CaskLoader.tap_paths(ref)).count
when 1
new(possible_tap_casks.first)
when 2..Float::INFINITY
loaders = possible_tap_casks.map(&FromTapPathLoader.method(:new))
loaders = possible_tap_casks.map(&FromPathLoader.method(:new))
raise TapCaskAmbiguityError.new(ref, loaders)
end
end
Expand Down Expand Up @@ -568,7 +552,6 @@ def self.for(ref, need_path: false, warn: true)
FromURILoader,
FromAPILoader,
FromTapLoader,
FromTapPathLoader,
FromPathLoader,
FromDefaultTapPathLoader,
FromAmbiguousTapPathLoader,
Expand All @@ -587,7 +570,7 @@ def self.default_path(token)
find_cask_in_tap(token.to_s.downcase, CoreCaskTap.instance)
end

def self.tap_paths(token, warn: true)
def self.tap_paths(token)
token = token.to_s.downcase

Tap.map { |tap| find_cask_in_tap(token, tap) }.select(&:exist?)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/cask/caskroom.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ def self.casks(config: nil)
CaskLoader.load(token, config: config)
rescue TapCaskAmbiguityError
tap_path = CaskLoader.tap_paths(token).first
CaskLoader::FromTapPathLoader.new(tap_path).load(config: config)
CaskLoader::FromPathLoader.new(tap_path).load(config: config)
rescue
# Don't blow up because of a single unavailable cask.
nil
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/cli/named_args.rb
Original file line number Diff line number Diff line change
Expand Up @@ -256,11 +256,11 @@ def to_paths(only: parent&.only_formula_or_cask, recurse_tap: false)
paths = []

if formula_path.exist? ||
(!CoreTap.instance.installed? && Homebrew::API::Formula.all_formulae.key?(path.basename))
(!CoreTap.instance.installed? && Homebrew::API::Formula.all_formulae.key?(path.basename.to_s))
paths << formula_path
end
if cask_path.exist? ||
(!CoreCaskTap.instance.installed? && Homebrew::API::Cask.all_casks.key?(path.basename))
(!CoreCaskTap.instance.installed? && Homebrew::API::Cask.all_casks.key?(path.basename.to_s))
Comment on lines -259 to +263
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this somehow got coerced to strings before. Otherwise, I'm not really sure how this ever worked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's because we have a Pathname#to_str monkey-patch.

paths << cask_path
end

Expand Down
108 changes: 108 additions & 0 deletions Library/Homebrew/sorbet/rbi/upstream.rbi
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,111 @@

# This file contains temporary definitions for fixes that have
# been submitted upstream to https://github.com/sorbet/sorbet.

# https://github.com/sorbet/sorbet/pull/7678
class String
sig do
params(
arg0: Integer,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: T.any(T::Range[Integer], Regexp),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: T.any(String, Symbol),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: String,
)
.returns(T.nilable(String))
end
def [](arg0, arg1=T.unsafe(nil)); end

sig do
params(
arg0: Integer,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: T.any(T::Range[Integer], Regexp),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: T.any(String, Symbol),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: String,
)
.returns(T.nilable(String))
end
def slice!(arg0, arg1=T.unsafe(nil)); end

sig do
params(
arg0: Integer,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: T.any(T::Range[Integer], Regexp),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: Integer,
)
.returns(T.nilable(String))
end
sig do
params(
arg0: Regexp,
arg1: T.any(String, Symbol),
)
.returns(T.nilable(String))
end
sig do
params(
arg0: String,
)
.returns(T.nilable(String))
end
def slice(arg0, arg1=T.unsafe(nil)); end
end
6 changes: 4 additions & 2 deletions Library/Homebrew/tap_constants.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
HOMEBREW_TAP_FORMULA_REGEX = T.let(%r{^([\w-]+)/([\w-]+)/([\w+-.@]+)$}, Regexp)
# Match taps' casks, e.g. `someuser/sometap/somecask`
HOMEBREW_TAP_CASK_REGEX = T.let(%r{^([\w-]+)/([\w-]+)/([a-z0-9\-_]+)$}, Regexp)
# Match main cask taps' casks, e.g. `homebrew/cask/somecask` or `somecask`
HOMEBREW_MAIN_TAP_CASK_REGEX = T.let(%r{^(?:[Hh]omebrew/(?:homebrew-)?cask/)?(?<token>[a-z0-9\-_]+)$}, Regexp)
# Match default cask taps' casks, e.g. `homebrew/cask/somecask` or `somecask`
HOMEBREW_DEFAULT_TAP_CASK_REGEX = T.let(
%r{^(?:[Hh]omebrew/(?:homebrew-)?cask/)?(?<token>[a-z0-9\-_]+)$}, Regexp
)
# Match taps' directory paths, e.g. `HOMEBREW_LIBRARY/Taps/someuser/sometap`
HOMEBREW_TAP_DIR_REGEX = T.let(
%r{#{Regexp.escape(HOMEBREW_LIBRARY.to_s)}/Taps/(?<user>[\w-]+)/(?<repo>[\w-]+)}, Regexp
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/test/cask/cask_loader_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@

it "warns when using the short token" do
expect do
expect(described_class.for("version-newest")).to be_a Cask::CaskLoader::FromTapPathLoader
expect(described_class.for("version-newest")).to be_a Cask::CaskLoader::FromPathLoader
end.to output(/version-newest was renamed to version-latest/).to_stderr
end

it "warns when using the full token" do
expect do
expect(described_class.for("homebrew/cask/version-newest")).to be_a Cask::CaskLoader::FromTapPathLoader
expect(described_class.for("homebrew/cask/version-newest")).to be_a Cask::CaskLoader::FromPathLoader
end.to output(/version-newest was renamed to version-latest/).to_stderr
end
end
Expand Down
Loading