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

Refactor CaskLoader::for. #16609

Merged
merged 6 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
238 changes: 181 additions & 57 deletions Library/Homebrew/cask/cask_loader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,10 @@ def cask(header_token, **options, &block)

# Loads a cask from a string.
class FromContentLoader < AbstractContentLoader
def self.can_load?(ref)
def self.try_new(ref, warn: false)
Comment on lines 52 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this approach but I am 50/50 on the name. I don't have any better ideas though.

Would this approach make it easier to streamline formula loading as well? The current #can_load? style logic doesn't really work well for formulae since we need a bunch of workarounds and extra checks when deciding what to load.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I hope we can use the same pattern for formulae as well.

return false unless ref.respond_to?(:to_str)

content = ref.to_str
content = T.unsafe(ref).to_str
Comment on lines 54 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit surprised that Sorbet can't figure this one out. sigh

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, unfortunate, but it does make sense: https://sorbet.org/docs/flow-sensitive#what-about-respond_to

You cannot deduce a type by its methods alone. We could just use is_a?(String) here though, because anything else doesn't make sense anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way is fine for me. Thanks for explaining it to me.


# Cache compiled regex
@regex ||= begin
Expand All @@ -63,13 +63,16 @@ def self.can_load?(ref)
/\A\s*cask(?:#{curly.source}|#{do_end.source})\s*\Z/m
end

content.match?(@regex)
return unless content.match?(@regex)

new(content)
end

def initialize(content, tap: nil)
sig { params(content: String, tap: Tap).void }
def initialize(content, tap: T.unsafe(nil))
super()

@content = content.force_encoding("UTF-8")
@content = content.dup.force_encoding("UTF-8")
Comment on lines -72 to +75
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt the String#dup is really necessary here. Are we relying on the original string encoding anywhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Some tests were failing here because they pass in string literals. It is surprising that this is necessary only now though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right, we were only testing can_load? before, not the constructor.

@tap = tap
end

Expand All @@ -82,14 +85,26 @@ def load(config:)

# Loads a cask from a path.
class FromPathLoader < AbstractContentLoader
def self.can_load?(ref)
path = Pathname(ref)
%w[.rb .json].include?(path.extname) && path.expand_path.exist?
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)
ref = Pathname(ref) if ref.is_a?(String)
return unless ref.is_a?(Pathname)

path = ref

return if %w[.rb .json].exclude?(path.extname)
return unless path.expand_path.exist?

new(path)
end

attr_reader :token, :path

def initialize(path, token: nil)
sig { params(path: T.any(Pathname, String), token: String).void }
def initialize(path, token: T.unsafe(nil))
super()

path = Pathname(path).expand_path
Expand Down Expand Up @@ -134,19 +149,24 @@ def cask(header_token, **options, &block)

# Loads a cask from a URI.
class FromURILoader < FromPathLoader
def self.can_load?(ref)
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)
# Cache compiled regex
@uri_regex ||= begin
uri_regex = ::URI::DEFAULT_PARSER.make_regexp
Regexp.new("\\A#{uri_regex.source}\\Z", uri_regex.options)
end

return false unless ref.to_s.match?(@uri_regex)
uri = ref.to_s
return unless uri.match?(@uri_regex)

uri = URI(ref)
return false unless uri.path
uri = URI(uri)
return unless uri.path

true
new(uri)
end

attr_reader :url
Expand All @@ -173,10 +193,17 @@ def load(config:)

# Loads a cask from a tap path.
class FromTapPathLoader < FromPathLoader
def self.can_load?(ref)
super && !Tap.from_path(ref).nil?
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)
Expand All @@ -185,10 +212,20 @@ def initialize(path)

# Loads a cask from a specific tap.
class FromTapLoader < FromTapPathLoader
def self.can_load?(ref)
ref.to_s.match?(HOMEBREW_TAP_CASK_REGEX)
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)
ref = ref.to_s

return unless ref.match?(HOMEBREW_TAP_CASK_REGEX)

token, tap, = CaskLoader.tap_cask_token_type(ref, warn: warn)
new("#{tap}/#{token}")
end

sig { params(tapped_name: String).void }
def initialize(tapped_name)
user, repo, token = tapped_name.split("/", 3)
tap = Tap.fetch(user, repo)
Expand All @@ -203,24 +240,52 @@ def load(config:)
end
end

# Loads a cask from the default tap path.
class FromDefaultTapPathLoader < FromTapPathLoader
def self.can_load?(ref)
super CaskLoader.default_path(ref)
# Load a cask from the default tap, using either a full or short token.
class FromDefaultTapLoader < FromTapLoader
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)
ref = ref.to_s

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

token = match[:token]

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

token, tap, = CaskLoader.tap_cask_token_type(ref, warn: warn)
new("#{tap}/#{token}")
end
end

def initialize(ref)
# Loads a cask from the default tap path.
class FromDefaultTapPathLoader < FromTapPathLoader
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)
super CaskLoader.default_path(ref)
end
end

# Loads a cask from an existing {Cask} instance.
class FromInstanceLoader
include ILoader
def self.can_load?(ref)
ref.is_a?(Cask)

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 ref.is_a?(Cask)

new(ref)
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
end

sig { params(cask: Cask).void }
def initialize(cask)
@cask = cask
end
Expand All @@ -233,25 +298,41 @@ def load(config:)
# Loads a cask from the JSON API.
class FromAPILoader
include ILoader

attr_reader :token, :path

def self.can_load?(ref)
return false if Homebrew::EnvConfig.no_install_from_api?
return false unless ref.is_a?(String)
return false unless ref.match?(HOMEBREW_MAIN_TAP_CASK_REGEX)
sig { returns(T.nilable(Hash)) }
attr_reader :from_json

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 if Homebrew::EnvConfig.no_install_from_api?
return unless ref.is_a?(String)

token = ref.sub(%r{^homebrew/(?:homebrew-)?cask/}i, "")
Homebrew::API::Cask.all_casks.key?(token)
return unless (match = ref.match(HOMEBREW_MAIN_TAP_CASK_REGEX))

token = match[:token]

return unless Homebrew::API::Cask.all_casks.key?(token)

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

token, tap, = CaskLoader.tap_cask_token_type(ref, warn: warn)
new("#{tap}/#{token}")
end

def initialize(token, from_json: nil)
sig { params(token: String, from_json: Hash).void }
def initialize(token, from_json: T.unsafe(nil))
@token = token.sub(%r{^homebrew/(?:homebrew-)?cask/}i, "")
@path = CaskLoader.default_path(@token)
@from_json = from_json
end

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

cask_options = {
loaded_from_api: true,
Expand Down Expand Up @@ -399,10 +480,41 @@ def from_h_gsubs(value, appdir)
end
end

# Loader which tries loading casks from tap paths, failing
# if the same token exists in multiple taps.
class FromAmbiguousTapPathLoader < FromTapPathLoader
def self.try_new(ref, warn: false)
case (possible_tap_casks = CaskLoader.tap_paths(ref, warn: warn)).count
when 1
new(possible_tap_casks.first)
when 2..Float::INFINITY
loaders = possible_tap_casks.map(&FromTapPathLoader.method(:new))
raise TapCaskAmbiguityError.new(ref, loaders)
end
end
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
end

# Loader which loads a cask from the installed cask file.
class FromInstalledPathLoader < FromPathLoader
def self.try_new(ref, warn: false)
possible_installed_cask = Cask.new(ref)
return unless (installed_caskfile = possible_installed_cask.installed_caskfile)

new(installed_caskfile)
end
end

# Pseudo-loader which raises an error when trying to load the corresponding cask.
class NullLoader < FromPathLoader
def self.can_load?(*)
true
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 if ref.is_a?(Cask)
return if ref.is_a?(URI::Generic)

new(ref)
apainintheneck marked this conversation as resolved.
Show resolved Hide resolved
end

sig { params(ref: T.any(String, Pathname)).void }
Expand All @@ -424,6 +536,33 @@ def self.load(ref, config: nil, warn: true)
self.for(ref, warn: warn).load(config: config)
end

def self.tap_cask_token_type(tapped_token, warn:)
user, repo, token = tapped_token.split("/", 3).map(&:downcase)
tap = Tap.fetch(user, repo)
type = nil

if (new_token = tap.cask_renames[token].presence)
old_token = token
token = new_token
new_token = tap.core_cask_tap? ? token : "#{tap}/#{token}"
type = :rename
elsif (new_tap_name = tap.tap_migrations[token].presence)
new_tap_user, new_tap_repo, = new_tap_name.split("/")
new_tap_name = "#{new_tap_user}/#{new_tap_repo}"
new_tap = Tap.fetch(new_tap_name)
new_tap.ensure_installed!
apainintheneck marked this conversation as resolved.
Show resolved Hide resolved
new_tapped_token = "#{new_tap_name}/#{token}"
token, tap, = tap_cask_token_type(new_tapped_token, warn: false)
old_token = tapped_token
new_token = new_tap.core_cask_tap? ? token : new_tapped_token
type = :migration
end

opoo "Cask #{old_token} was renamed to #{new_token}." if warn && old_token && new_token

[token, tap, type]
end

def self.for(ref, need_path: false, warn: true)
[
FromInstanceLoader,
Expand All @@ -434,26 +573,15 @@ def self.for(ref, need_path: false, warn: true)
FromTapPathLoader,
FromPathLoader,
FromDefaultTapPathLoader,
FromAmbiguousTapPathLoader,
FromDefaultTapLoader,
FromInstalledPathLoader,
NullLoader,
].each do |loader_class|
if loader_class.can_load?(ref)
$stderr.puts "#{$PROGRAM_NAME} (#{loader_class}): loading #{ref}" if debug?
return loader_class.new(ref)
if (loader = loader_class.try_new(ref, warn: warn))
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
return loader
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
end
end

case (possible_tap_casks = tap_paths(ref, warn: warn)).count
when 1
return FromTapPathLoader.new(possible_tap_casks.first)
when 2..Float::INFINITY
loaders = possible_tap_casks.map(&FromTapPathLoader.method(:new))

raise TapCaskAmbiguityError.new(ref, loaders)
end

possible_installed_cask = Cask.new(ref)
return FromPathLoader.new(possible_installed_cask.installed_caskfile) if possible_installed_cask.installed?

NullLoader.new(ref)
end

def self.default_path(token)
Expand All @@ -463,11 +591,7 @@ def self.default_path(token)
def self.tap_paths(token, warn: true)
token = token.to_s.downcase

Tap.map do |tap|
new_token = tap.cask_renames[token]
opoo "Cask #{token} was renamed to #{new_token}." if new_token && warn
find_cask_in_tap(new_token || token, tap)
end.select(&:exist?)
Tap.map { |tap| find_cask_in_tap(token, tap) }.select(&:exist?)
end

def self.find_cask_in_tap(token, tap)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/tap_constants.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
# 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/)?[a-z0-9\-_]+$}, Regexp)
HOMEBREW_MAIN_TAP_CASK_REGEX = T.let(%r{^(?<tap>[Hh]omebrew/(?:homebrew-)?cask/)?(?<token>[a-z0-9\-_]+)$}, Regexp)
reitermarkus marked this conversation as resolved.
Show resolved Hide resolved
# 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