From 3b4ee58c491f164c2751c1c0f941568c35fb0ac6 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Mon, 26 Jun 2017 07:30:28 +0200 Subject: [PATCH 1/4] Refactor using `Forwardable` and `DelegateClass`. --- .../Homebrew/cask/lib/hbc/artifact/base.rb | 10 +- Library/Homebrew/cask/lib/hbc/cask.rb | 2 - Library/Homebrew/cask/lib/hbc/checkable.rb | 14 +-- Library/Homebrew/cask/lib/hbc/installer.rb | 19 +-- Library/Homebrew/cask/lib/hbc/url.rb | 2 - Library/Homebrew/cask/lib/hbc/utils.rb | 8 +- Library/Homebrew/caveats.rb | 6 +- Library/Homebrew/checksum.rb | 7 +- Library/Homebrew/cmd/info.rb | 4 +- Library/Homebrew/cmd/update-report.rb | 6 +- Library/Homebrew/debrew.rb | 11 +- Library/Homebrew/dependencies.rb | 59 +++------ Library/Homebrew/dependency.rb | 5 +- Library/Homebrew/download_strategy.rb | 5 +- Library/Homebrew/extend/predicable.rb | 9 ++ Library/Homebrew/formula.rb | 115 +++++------------- Library/Homebrew/formula_installer.rb | 11 +- Library/Homebrew/formula_support.rb | 2 +- Library/Homebrew/global.rb | 2 + Library/Homebrew/resource.rb | 1 - Library/Homebrew/software_spec.rb | 1 - 21 files changed, 96 insertions(+), 203 deletions(-) create mode 100644 Library/Homebrew/extend/predicable.rb diff --git a/Library/Homebrew/cask/lib/hbc/artifact/base.rb b/Library/Homebrew/cask/lib/hbc/artifact/base.rb index 2d9330b138..ae15552a4e 100644 --- a/Library/Homebrew/cask/lib/hbc/artifact/base.rb +++ b/Library/Homebrew/cask/lib/hbc/artifact/base.rb @@ -1,6 +1,8 @@ module Hbc module Artifact class Base + extend Predicable + def self.artifact_name @artifact_name ||= name.sub(/^.*:/, "").gsub(/(.)([A-Z])/, '\1_\2').downcase end @@ -65,13 +67,7 @@ module Hbc {} end - def verbose? - @verbose - end - - def force? - @force - end + attr_predicate :force?, :verbose? def initialize(cask, command: SystemCommand, force: false, verbose: false) @cask = cask diff --git a/Library/Homebrew/cask/lib/hbc/cask.rb b/Library/Homebrew/cask/lib/hbc/cask.rb index e1cdb5deab..4a410f4c7d 100644 --- a/Library/Homebrew/cask/lib/hbc/cask.rb +++ b/Library/Homebrew/cask/lib/hbc/cask.rb @@ -1,5 +1,3 @@ -require "forwardable" - require "hbc/dsl" require "hbc/metadata" diff --git a/Library/Homebrew/cask/lib/hbc/checkable.rb b/Library/Homebrew/cask/lib/hbc/checkable.rb index 03f0526292..604dd3f89c 100644 --- a/Library/Homebrew/cask/lib/hbc/checkable.rb +++ b/Library/Homebrew/cask/lib/hbc/checkable.rb @@ -1,29 +1,27 @@ module Hbc module Checkable def errors - Array(@errors) + @errors ||= [] end def warnings - Array(@warnings) + @warnings ||= [] end def add_error(message) - @errors ||= [] - @errors << message + errors << message end def add_warning(message) - @warnings ||= [] - @warnings << message + warnings << message end def errors? - Array(@errors).any? + errors.any? end def warnings? - Array(@warnings).any? + warnings.any? end def result diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index aa6d600f7f..0477cbd22f 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -6,6 +6,7 @@ require "hbc/verify" module Hbc class Installer + extend Predicable # TODO: it is unwise for Hbc::Staged to be a module, when we are # dealing with both staged and unstaged Casks here. This should # either be a class which is only sometimes instantiated, or there @@ -27,21 +28,7 @@ module Hbc @reinstall = false end - def skip_cask_deps? - @skip_cask_deps - end - - def force? - @force - end - - def binaries? - @binaries - end - - def verbose? - @verbose - end + attr_predicate :binaries?, :force?, :skip_cask_deps?, :require_sha?, :verbose? def self.print_caveats(cask) odebug "Printing caveats" @@ -75,7 +62,7 @@ module Hbc odebug "Hbc::Installer#fetch" satisfy_dependencies - verify_has_sha if @require_sha && !force? + verify_has_sha if require_sha? && !force? download verify end diff --git a/Library/Homebrew/cask/lib/hbc/url.rb b/Library/Homebrew/cask/lib/hbc/url.rb index 07b41fb803..15da2ced21 100644 --- a/Library/Homebrew/cask/lib/hbc/url.rb +++ b/Library/Homebrew/cask/lib/hbc/url.rb @@ -1,5 +1,3 @@ -require "forwardable" - module Hbc class URL FAKE_USER_AGENT = "Mozilla/5.0 (Macintosh; Intel Mac OS X 10) https://caskroom.github.io".freeze diff --git a/Library/Homebrew/cask/lib/hbc/utils.rb b/Library/Homebrew/cask/lib/hbc/utils.rb index 59e85aaeb3..f23ce059c0 100644 --- a/Library/Homebrew/cask/lib/hbc/utils.rb +++ b/Library/Homebrew/cask/lib/hbc/utils.rb @@ -14,14 +14,14 @@ class Object end class Buffer < StringIO + extend Predicable + + attr_predicate :tty? + def initialize(tty = false) super() @tty = tty end - - def tty? - @tty - end end # global methods diff --git a/Library/Homebrew/caveats.rb b/Library/Homebrew/caveats.rb index ee09063fd9..2d0ceeb481 100644 --- a/Library/Homebrew/caveats.rb +++ b/Library/Homebrew/caveats.rb @@ -1,4 +1,6 @@ class Caveats + extend Forwardable + attr_reader :f def initialize(f) @@ -25,9 +27,7 @@ class Caveats caveats.compact.join("\n") end - def empty? - caveats.empty? - end + delegate [:empty?, :to_s] => :caveats private diff --git a/Library/Homebrew/checksum.rb b/Library/Homebrew/checksum.rb index 1b095ea328..506885fce3 100644 --- a/Library/Homebrew/checksum.rb +++ b/Library/Homebrew/checksum.rb @@ -1,6 +1,7 @@ class Checksum + extend Forwardable + attr_reader :hash_type, :hexdigest - alias to_s hexdigest TYPES = [:sha256].freeze @@ -9,9 +10,7 @@ class Checksum @hexdigest = hexdigest end - def empty? - hexdigest.empty? - end + delegate [:empty?, :to_s] => :@hexdigest def ==(other) hash_type == other.hash_type && hexdigest == other.hexdigest diff --git a/Library/Homebrew/cmd/info.rb b/Library/Homebrew/cmd/info.rb index 98200a0b4a..731156f955 100644 --- a/Library/Homebrew/cmd/info.rb +++ b/Library/Homebrew/cmd/info.rb @@ -169,8 +169,8 @@ module Homebrew Homebrew.dump_options_for_formula f end - c = Caveats.new(f) - ohai "Caveats", c.caveats unless c.empty? + caveats = Caveats.new(f) + ohai "Caveats", caveats.to_s unless caveats.empty? end def decorate_dependencies(dependencies) diff --git a/Library/Homebrew/cmd/update-report.rb b/Library/Homebrew/cmd/update-report.rb index aa1fd244d0..374bbbd739 100644 --- a/Library/Homebrew/cmd/update-report.rb +++ b/Library/Homebrew/cmd/update-report.rb @@ -547,6 +547,8 @@ class Reporter end class ReporterHub + extend Forwardable + attr_reader :reporters def initialize @@ -564,9 +566,7 @@ class ReporterHub @hash.update(report) { |_key, oldval, newval| oldval.concat(newval) } end - def empty? - @hash.empty? - end + delegate :empty? => :@hash def dump # Key Legend: Added (A), Copied (C), Deleted (D), Modified (M), Renamed (R) diff --git a/Library/Homebrew/debrew.rb b/Library/Homebrew/debrew.rb index 7ff2c03605..c2662c9eee 100644 --- a/Library/Homebrew/debrew.rb +++ b/Library/Homebrew/debrew.rb @@ -74,18 +74,13 @@ module Debrew end end - class << self - alias original_raise raise - end - @active = false @debugged_exceptions = Set.new - def self.active? - @active - end - class << self + extend Predicable + alias original_raise raise + attr_predicate :active? attr_reader :debugged_exceptions end diff --git a/Library/Homebrew/dependencies.rb b/Library/Homebrew/dependencies.rb index 951db078df..0ed0502d81 100644 --- a/Library/Homebrew/dependencies.rb +++ b/Library/Homebrew/dependencies.rb @@ -1,28 +1,11 @@ -class Dependencies - include Enumerable +require "delegate" - def initialize - @deps = [] +class Dependencies < DelegateClass(Array) + def initialize(*args) + super(args) end - def each(*args, &block) - @deps.each(*args, &block) - end - - def <<(o) - @deps << o - self - end - - def empty? - @deps.empty? - end - - def *(arg) - @deps * arg - end - - alias to_ary to_a + alias eql? == def optional select(&:optional?) @@ -44,40 +27,28 @@ class Dependencies build + required + recommended end - attr_reader :deps - protected :deps - - def ==(other) - deps == other.deps - end - alias eql? == - def inspect - "#<#{self.class.name}: #{to_a.inspect}>" + "#<#{self.class.name}: #{to_a}>" end end -class Requirements - include Enumerable - - def initialize - @reqs = Set.new - end - - def each(*args, &block) - @reqs.each(*args, &block) +class Requirements < DelegateClass(Set) + def initialize(*args) + super(Set.new(args)) end def <<(other) if other.is_a?(Comparable) - @reqs.grep(other.class) do |req| + grep(other.class) do |req| return self if req > other - @reqs.delete(req) + delete(req) end end - @reqs << other + super self end - alias to_ary to_a + def inspect + "#<#{self.class.name}: {#{to_a.join(", ")}}>" + end end diff --git a/Library/Homebrew/dependency.rb b/Library/Homebrew/dependency.rb index 4a452440a7..d7d5ec59c0 100644 --- a/Library/Homebrew/dependency.rb +++ b/Library/Homebrew/dependency.rb @@ -2,6 +2,7 @@ require "dependable" # A dependency on another Homebrew formula. class Dependency + extend Forwardable include Dependable attr_reader :name, :tags, :env_proc, :option_names @@ -34,9 +35,7 @@ class Dependency formula end - def installed? - to_formula.installed? - end + delegate installed?: :to_formula def satisfied?(inherited_options) installed? && missing_options(inherited_options).empty? diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 78f6c1e28c..7173347143 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -3,6 +3,7 @@ require "rexml/document" require "time" class AbstractDownloadStrategy + extend Forwardable include FileUtils attr_reader :meta, :name, :version, :resource @@ -181,9 +182,7 @@ class VCSDownloadStrategy < AbstractDownloadStrategy @clone end - def head? - version.head? - end + delegate head?: :version # Return last commit's unique identifier for the repository. # Return most recent modified timestamp unless overridden. diff --git a/Library/Homebrew/extend/predicable.rb b/Library/Homebrew/extend/predicable.rb new file mode 100644 index 0000000000..d02fbb89be --- /dev/null +++ b/Library/Homebrew/extend/predicable.rb @@ -0,0 +1,9 @@ +module Predicable + def attr_predicate(*attrs) + attrs.each do |attr| + define_method attr do + instance_variable_get("@#{attr.to_s.sub(/\?$/, "")}") == true + end + end + end +end diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index 66993a348b..0d000496b9 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -45,6 +45,7 @@ class Formula include Utils::Inreplace include Utils::Shell extend Enumerable + extend Forwardable # @!method inreplace(paths, before = nil, after = nil) # Actually implemented in {Utils::Inreplace.inreplace}. @@ -314,37 +315,14 @@ class Formula active_spec == head end - # @private - def bottle_unneeded? - active_spec.bottle_unneeded? - end - - # @private - def bottle_disabled? - active_spec.bottle_disabled? - end - - # @private - def bottle_disable_reason - active_spec.bottle_disable_reason - end - - # Does the currently active {SoftwareSpec} have any bottle? - # @private - def bottle_defined? - active_spec.bottle_defined? - end - - # Does the currently active {SoftwareSpec} have an installable bottle? - # @private - def bottled? - active_spec.bottled? - end - - # @private - def bottle_specification - active_spec.bottle_specification - end + delegate [ + :bottle_unneeded?, + :bottle_disabled?, + :bottle_disable_reason, + :bottle_defined?, + :bottled?, + :bottle_specification, + ] => :active_spec # The Bottle object for the currently active {SoftwareSpec}. # @private @@ -353,24 +331,21 @@ class Formula end # The description of the software. + # @method desc # @see .desc - def desc - self.class.desc - end + delegate desc: :"self.class" # The homepage for the software. + # @method homepage # @see .homepage - def homepage - self.class.homepage - end + delegate homepage: :"self.class" # The version for the currently active {SoftwareSpec}. # The version is autodetected from the URL and/or tag so only needs to be # declared if it cannot be autodetected correctly. + # @method version # @see .version - def version - active_spec.version - end + delegate version: :active_spec def update_head_version return unless head? @@ -394,9 +369,8 @@ class Formula # Additional downloads can be defined as {#resource}s. # {Resource#stage} will create a temporary directory and yield to a block. # 
resource("additional_files").stage { bin.install "my/extra/tool" }
- def resource(name) - active_spec.resource(name) - end + # @method resource + delegate resource: :active_spec # An old name for the formula def oldname @@ -416,68 +390,39 @@ class Formula end # The {Resource}s for the currently active {SoftwareSpec}. - def resources - active_spec.resources.values - end + # @method resources + def_delegator :"active_spec.resources", :values, :resources # The {Dependency}s for the currently active {SoftwareSpec}. - # @private - def deps - active_spec.deps - end + delegate deps: :active_spec # The {Requirement}s for the currently active {SoftwareSpec}. - # @private - def requirements - active_spec.requirements - end + delegate requirements: :active_spec # The cached download for the currently active {SoftwareSpec}. - # @private - def cached_download - active_spec.cached_download - end + delegate cached_download: :active_spec # Deletes the download for the currently active {SoftwareSpec}. - # @private - def clear_cache - active_spec.clear_cache - end + delegate clear_cache: :active_spec # The list of patches for the currently active {SoftwareSpec}. - # @private - def patchlist - active_spec.patches - end + def_delegator :active_spec, :patches, :patchlist # The options for the currently active {SoftwareSpec}. - # @private - def options - active_spec.options - end + delegate options: :active_spec # The deprecated options for the currently active {SoftwareSpec}. - # @private - def deprecated_options - active_spec.deprecated_options - end + delegate deprecated_options: :active_spec # The deprecated option flags for the currently active {SoftwareSpec}. - # @private - def deprecated_flags - active_spec.deprecated_flags - end + delegate deprecated_flags: :active_spec # If a named option is defined for the currently active {SoftwareSpec}. - def option_defined?(name) - active_spec.option_defined?(name) - end + # @method option_defined? + delegate option_defined?: :active_spec # All the {.fails_with} for the currently active {SoftwareSpec}. - # @private - def compiler_failures - active_spec.compiler_failures - end + delegate compiler_failures: :active_spec # If this {Formula} is installed. # This is actually just a check for if the {#installed_prefix} directory diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index a365c967ad..27786e77ed 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -17,6 +17,7 @@ require "development_tools" class FormulaInstaller include FormulaCellarChecks + extend Predicable def self.mode_attr_accessor(*names) attr_accessor(*names) @@ -559,11 +560,11 @@ class FormulaInstaller audit_installed if ARGV.homebrew_developer? && !formula.keg_only? - c = Caveats.new(formula) + caveats = Caveats.new(formula) - return if c.empty? + return if caveats.empty? @show_summary_heading = true - ohai "Caveats", c.caveats + ohai "Caveats", caveats.to_s end def finish @@ -879,9 +880,7 @@ class FormulaInstaller private - def hold_locks? - @hold_locks || false - end + attr_predicate :hold_locks? def lock return unless (@@locked ||= []).empty? diff --git a/Library/Homebrew/formula_support.rb b/Library/Homebrew/formula_support.rb index 4d963a55e1..a4534291a2 100644 --- a/Library/Homebrew/formula_support.rb +++ b/Library/Homebrew/formula_support.rb @@ -81,7 +81,7 @@ class BottleDisableReason end def to_s - if @type == :unneeded + if unneeded? "This formula doesn't require compiling." else @reason diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index f36b018745..9f79781b41 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -1,4 +1,6 @@ +require "forwardable" require "extend/module" +require "extend/predicable" require "extend/fileutils" require "extend/pathname" require "extend/git_repository" diff --git a/Library/Homebrew/resource.rb b/Library/Homebrew/resource.rb index c0e9dbadae..1e55b69c55 100644 --- a/Library/Homebrew/resource.rb +++ b/Library/Homebrew/resource.rb @@ -1,7 +1,6 @@ require "download_strategy" require "checksum" require "version" -require "forwardable" # Resource is the fundamental representation of an external resource. The # primary formula download, along with other declared resources, are instances diff --git a/Library/Homebrew/software_spec.rb b/Library/Homebrew/software_spec.rb index eaae724a42..dd6cea8b2c 100644 --- a/Library/Homebrew/software_spec.rb +++ b/Library/Homebrew/software_spec.rb @@ -1,4 +1,3 @@ -require "forwardable" require "resource" require "checksum" require "version" From 2703646b1008e8fc0a922e4677d9b2aadcea4b4d Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Tue, 27 Jun 2017 11:55:23 +0200 Subject: [PATCH 2/4] Remove `utf8_inspect` method. --- Library/Homebrew/cask/lib/hbc/download_strategy.rb | 2 +- Library/Homebrew/cask/lib/hbc/system_command.rb | 6 +++--- Library/Homebrew/cask/lib/hbc/url_checker.rb | 2 +- Library/Homebrew/cask/lib/hbc/utils.rb | 9 --------- 4 files changed, 5 insertions(+), 14 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/download_strategy.rb b/Library/Homebrew/cask/lib/hbc/download_strategy.rb index 5b32b4840c..28ae704ee0 100644 --- a/Library/Homebrew/cask/lib/hbc/download_strategy.rb +++ b/Library/Homebrew/cask/lib/hbc/download_strategy.rb @@ -100,7 +100,7 @@ module Hbc end def _fetch - odebug "Calling curl with args #{cask_curl_args.utf8_inspect}" + odebug "Calling curl with args #{cask_curl_args}" curl(*cask_curl_args) end diff --git a/Library/Homebrew/cask/lib/hbc/system_command.rb b/Library/Homebrew/cask/lib/hbc/system_command.rb index 6414a9e807..901617b71b 100644 --- a/Library/Homebrew/cask/lib/hbc/system_command.rb +++ b/Library/Homebrew/cask/lib/hbc/system_command.rb @@ -20,7 +20,7 @@ module Hbc def run! @processed_output = { stdout: "", stderr: "" } - odebug "Executing: #{expanded_command.utf8_inspect}" + odebug "Executing: #{expanded_command}" each_output_line do |type, line| case type @@ -62,7 +62,7 @@ module Hbc def assert_success return if processed_status && processed_status.success? - raise CaskCommandFailedError.new(command.utf8_inspect, processed_output[:stdout], processed_output[:stderr], processed_status) + raise CaskCommandFailedError.new(command, processed_output[:stdout], processed_output[:stderr], processed_status) end def expanded_command @@ -162,7 +162,7 @@ module Hbc raise CaskError, <<-EOS Empty result parsing plist output from command. command was: - #{command.utf8_inspect} + #{command} output we attempted to parse: #{output} EOS diff --git a/Library/Homebrew/cask/lib/hbc/url_checker.rb b/Library/Homebrew/cask/lib/hbc/url_checker.rb index 60a15ca2fe..7076b13c58 100644 --- a/Library/Homebrew/cask/lib/hbc/url_checker.rb +++ b/Library/Homebrew/cask/lib/hbc/url_checker.rb @@ -38,7 +38,7 @@ module Hbc def _check_response_status ok = OK_RESPONSES[cask.url.scheme] return if ok.include?(@response_status) - add_error "unexpected http response, expecting #{ok.map(&:utf8_inspect).join(" or ")}, got #{@response_status.utf8_inspect}" + add_error "unexpected http response, expecting #{ok.map(&:to_s).join(" or ")}, got #{@response_status}" end def _get_data_from_request diff --git a/Library/Homebrew/cask/lib/hbc/utils.rb b/Library/Homebrew/cask/lib/hbc/utils.rb index f23ce059c0..22f826e748 100644 --- a/Library/Homebrew/cask/lib/hbc/utils.rb +++ b/Library/Homebrew/cask/lib/hbc/utils.rb @@ -4,15 +4,6 @@ require "stringio" BUG_REPORTS_URL = "https://github.com/caskroom/homebrew-cask#reporting-bugs".freeze -# monkeypatch Object - not a great idea -class Object - def utf8_inspect - return inspect unless defined?(Encoding) - return map(&:utf8_inspect) if respond_to?(:map) - inspect.force_encoding("UTF-8").sub(/\A"(.*)"\Z/, '\1') - end -end - class Buffer < StringIO extend Predicable From 96271aaa89c47cf4f0ff10e8e9a165c1ac2cdb73 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 28 Jun 2017 09:25:14 +0200 Subject: [PATCH 3/4] Use `attr_reader` in `CaskLoader`. --- Library/Homebrew/cask/lib/hbc/cask_loader.rb | 43 +++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cask_loader.rb b/Library/Homebrew/cask/lib/hbc/cask_loader.rb index 43fd9080da..8cd010ef6b 100644 --- a/Library/Homebrew/cask/lib/hbc/cask_loader.rb +++ b/Library/Homebrew/cask/lib/hbc/cask_loader.rb @@ -1,12 +1,14 @@ module Hbc module CaskLoader class FromContentLoader + attr_reader :content + def initialize(content) @content = content end def load - instance_eval(@content.force_encoding("UTF-8"), __FILE__, __LINE__) + instance_eval(content.force_encoding("UTF-8"), __FILE__, __LINE__) end private @@ -18,25 +20,25 @@ module Hbc class FromPathLoader < FromContentLoader def self.can_load?(ref) - path = Pathname.new(ref) + path = Pathname(ref) path.extname == ".rb" && path.expand_path.exist? end attr_reader :token, :path def initialize(path) - path = Pathname.new(path).expand_path + path = Pathname(path).expand_path @token = path.basename(".rb").to_s @path = path end def load - raise CaskUnavailableError.new(@token, "'#{@path}' does not exist.") unless @path.exist? - raise CaskUnavailableError.new(@token, "'#{@path}' is not readable.") unless @path.readable? - raise CaskUnavailableError.new(@token, "'#{@path}' is not a file.") unless @path.file? + raise CaskUnavailableError.new(token, "'#{path}' does not exist.") unless path.exist? + raise CaskUnavailableError.new(token, "'#{path}' is not readable.") unless path.readable? + raise CaskUnavailableError.new(token, "'#{path}' is not a file.") unless path.file? - @content = IO.read(@path) + @content = IO.read(path) super end @@ -44,11 +46,11 @@ module Hbc private def cask(header_token, &block) - if @token != header_token - raise CaskTokenMismatchError.new(@token, header_token) + if token != header_token + raise CaskTokenMismatchError.new(token, header_token) end - Cask.new(header_token, sourcefile_path: @path, &block) + Cask.new(header_token, sourcefile_path: path, &block) end end @@ -65,14 +67,13 @@ module Hbc end def load - Hbc.cache.mkpath - FileUtils.rm_f @path + path.dirname.mkpath begin - ohai "Downloading #{@url}." - curl @url, "-o", @path + ohai "Downloading #{url}." + curl url, "-o", path rescue ErrorDuringExecution - raise CaskUnavailableError.new(@token, "Failed to download #{Formatter.url(@url)}.") + raise CaskUnavailableError.new(token, "Failed to download #{Formatter.url(url)}.") end super @@ -84,15 +85,17 @@ module Hbc ref.to_s.match?(HOMEBREW_TAP_CASK_REGEX) end + attr_reader :tap + def initialize(tapped_name) - user, repo, token = tapped_name.split("/", 3).map(&:downcase) + user, repo, token = tapped_name.split("/", 3) @tap = Tap.fetch(user, repo) super @tap.cask_dir/"#{token}.rb" end def load - @tap.install unless @tap.installed? + tap.install unless tap.installed? super end @@ -104,12 +107,12 @@ module Hbc end def initialize(ref) - @token = File.basename(ref, ".rb") - super CaskLoader.default_path(@token) + token = File.basename(ref, ".rb") + super CaskLoader.default_path(token) end def load - raise CaskUnavailableError.new(@token, "No Cask with this name exists.") + raise CaskUnavailableError.new(token, "No Cask with this name exists.") end end From 6a1fa87191bfef31ff1b2d47d3ebf281398a210f Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 28 Jun 2017 17:53:59 +0200 Subject: [PATCH 4/4] Refactor Formula/Cask dependencies. --- Library/Homebrew/cask/lib/hbc/cask.rb | 9 +++ .../cask/lib/hbc/cask_dependencies.rb | 37 +++++----- Library/Homebrew/cask/lib/hbc/dsl.rb | 4 +- .../Homebrew/cask/lib/hbc/dsl/depends_on.rb | 4 +- Library/Homebrew/cask/lib/hbc/exceptions.rb | 12 +++- Library/Homebrew/cask/lib/hbc/installer.rb | 67 ++++++++++++------- Library/Homebrew/test/cask/depends_on_spec.rb | 2 +- 7 files changed, 83 insertions(+), 52 deletions(-) diff --git a/Library/Homebrew/cask/lib/hbc/cask.rb b/Library/Homebrew/cask/lib/hbc/cask.rb index 4a410f4c7d..672d189545 100644 --- a/Library/Homebrew/cask/lib/hbc/cask.rb +++ b/Library/Homebrew/cask/lib/hbc/cask.rb @@ -71,6 +71,15 @@ module Hbc @token end + def hash + token.hash + end + + def eql?(other) + token == other.token + end + alias == eql? + def dumpcask odebug "Cask instance dumps in YAML:" odebug "Cask instance toplevel:", to_yaml diff --git a/Library/Homebrew/cask/lib/hbc/cask_dependencies.rb b/Library/Homebrew/cask/lib/hbc/cask_dependencies.rb index 8bba5df8e7..0edda074e3 100644 --- a/Library/Homebrew/cask/lib/hbc/cask_dependencies.rb +++ b/Library/Homebrew/cask/lib/hbc/cask_dependencies.rb @@ -1,35 +1,36 @@ +require "delegate" + require "hbc/topological_hash" module Hbc - class CaskDependencies - attr_reader :cask, :graph, :sorted + class CaskDependencies < DelegateClass(Array) + attr_reader :cask, :graph def initialize(cask) @cask = cask @graph = graph_dependencies - @sorted = sort + super(sort) end - def graph_dependencies - deps_in = ->(csk) { csk.depends_on ? csk.depends_on.cask || [] : [] } - walk = lambda do |acc, deps| - deps.each do |dep| - next if acc.key?(dep) - succs = deps_in.call CaskLoader.load(dep) - acc[dep] = succs - walk.call(acc, succs) - end - acc - end + private - graphed = walk.call({}, @cask.depends_on.cask) - TopologicalHash[graphed] + def graph_dependencies(cask = self.cask, acc = TopologicalHash.new) + return acc if acc.key?(cask) + deps = cask.depends_on.cask.map(&CaskLoader.public_method(:load)) + acc[cask] = deps + deps.each do |dep| + graph_dependencies(dep, acc) + end + acc end def sort - @graph.tsort + raise CaskSelfReferencingDependencyError, cask.token if graph[cask].include?(cask) + graph.tsort - [cask] rescue TSort::Cyclic - raise CaskCyclicCaskDependencyError, @cask.token + strongly_connected_components = graph.strongly_connected_components.sort_by(&:count) + cyclic_dependencies = strongly_connected_components.last - [cask] + raise CaskCyclicDependencyError.new(cask.token, cyclic_dependencies.join(", ")) end end end diff --git a/Library/Homebrew/cask/lib/hbc/dsl.rb b/Library/Homebrew/cask/lib/hbc/dsl.rb index 92245e8fb2..112ceb943f 100644 --- a/Library/Homebrew/cask/lib/hbc/dsl.rb +++ b/Library/Homebrew/cask/lib/hbc/dsl.rb @@ -211,10 +211,10 @@ module Hbc # depends_on uses a load method so that multiple stanzas can be merged def depends_on(*args) - return @depends_on if args.empty? @depends_on ||= DSL::DependsOn.new + return @depends_on if args.empty? begin - @depends_on.load(*args) unless args.empty? + @depends_on.load(*args) rescue RuntimeError => e raise CaskInvalidError.new(token, e) end diff --git a/Library/Homebrew/cask/lib/hbc/dsl/depends_on.rb b/Library/Homebrew/cask/lib/hbc/dsl/depends_on.rb index a8c1a1b738..bdef4239a5 100644 --- a/Library/Homebrew/cask/lib/hbc/dsl/depends_on.rb +++ b/Library/Homebrew/cask/lib/hbc/dsl/depends_on.rb @@ -24,6 +24,8 @@ module Hbc def initialize @pairs ||= {} + @cask ||= [] + @formula ||= [] end def load(pairs = {}) @@ -53,12 +55,10 @@ module Hbc end def formula=(*args) - @formula ||= [] @formula.concat(args) end def cask=(*args) - @cask ||= [] @cask.concat(args) end diff --git a/Library/Homebrew/cask/lib/hbc/exceptions.rb b/Library/Homebrew/cask/lib/hbc/exceptions.rb index d9e1b07dbf..1a246be65a 100644 --- a/Library/Homebrew/cask/lib/hbc/exceptions.rb +++ b/Library/Homebrew/cask/lib/hbc/exceptions.rb @@ -77,9 +77,15 @@ module Hbc end end - class CaskCyclicCaskDependencyError < AbstractCaskErrorWithToken + class CaskCyclicDependencyError < AbstractCaskErrorWithToken def to_s - "Cask '#{token}' includes cyclic dependencies on other Casks and could not be installed." + "Cask '#{token}' includes cyclic dependencies on other Casks" << (reason.empty? ? "." : ": #{reason}") + end + end + + class CaskSelfReferencingDependencyError < CaskCyclicDependencyError + def to_s + "Cask '#{token}' depends on itself." end end @@ -91,7 +97,7 @@ module Hbc class CaskInvalidError < AbstractCaskErrorWithToken def to_s - "Cask '#{token}' definition is invalid" << (reason.empty? ? ".": ": #{reason}") + "Cask '#{token}' definition is invalid" << (reason.empty? ? "." : ": #{reason}") end end diff --git a/Library/Homebrew/cask/lib/hbc/installer.rb b/Library/Homebrew/cask/lib/hbc/installer.rb index 0477cbd22f..252205a3bf 100644 --- a/Library/Homebrew/cask/lib/hbc/installer.rb +++ b/Library/Homebrew/cask/lib/hbc/installer.rb @@ -1,5 +1,7 @@ require "rubygems" +require "formula_installer" + require "hbc/cask_dependencies" require "hbc/staged" require "hbc/verify" @@ -197,7 +199,6 @@ module Hbc x11_dependencies formula_dependencies cask_dependencies unless skip_cask_deps? - puts "complete" end def macos_dependencies @@ -234,36 +235,50 @@ module Hbc end def formula_dependencies - return unless @cask.depends_on.formula && !@cask.depends_on.formula.empty? - ohai "Installing Formula dependencies from Homebrew" - @cask.depends_on.formula.each do |dep_name| - print "#{dep_name} ... " - installed = @command.run(HOMEBREW_BREW_FILE, - args: ["list", "--versions", dep_name], - print_stderr: false).stdout.include?(dep_name) - if installed - puts "already installed" - else - @command.run!(HOMEBREW_BREW_FILE, - args: ["install", dep_name]) - puts "done" + formulae = @cask.depends_on.formula.map { |f| Formula[f] } + return if formulae.empty? + + if formulae.all?(&:any_version_installed?) + puts "All Formula dependencies satisfied." + return + end + + not_installed = formulae.reject(&:any_version_installed?) + + ohai "Installing Formula dependencies: #{not_installed.map(&:to_s).join(", ")}" + not_installed.each do |formula| + begin + old_argv = ARGV.dup + ARGV.replace([]) + FormulaInstaller.new(formula).tap do |fi| + fi.installed_as_dependency = true + fi.installed_on_request = false + fi.show_header = true + fi.verbose = verbose? + fi.prelude + fi.install + fi.finish + end + ensure + ARGV.replace(old_argv) end end end def cask_dependencies - return unless @cask.depends_on.cask && !@cask.depends_on.cask.empty? - ohai "Installing Cask dependencies: #{@cask.depends_on.cask.join(", ")}" - deps = CaskDependencies.new(@cask) - deps.sorted.each do |dep_token| - puts "#{dep_token} ..." - dep = CaskLoader.load(dep_token) - if dep.installed? - puts "already installed" - else - Installer.new(dep, binaries: binaries?, verbose: verbose?, skip_cask_deps: true, force: false).install - puts "done" - end + return if @cask.depends_on.cask.empty? + casks = CaskDependencies.new(@cask) + + if casks.all?(&:installed?) + puts "All Cask dependencies satisfied." + return + end + + not_installed = casks.reject(&:installed?) + + ohai "Installing Cask dependencies: #{not_installed.map(&:to_s).join(", ")}" + not_installed.each do |cask| + Installer.new(cask, binaries: binaries?, verbose: verbose?, skip_cask_deps: true, force: false).install end end diff --git a/Library/Homebrew/test/cask/depends_on_spec.rb b/Library/Homebrew/test/cask/depends_on_spec.rb index 0299c243a6..4b78820cee 100644 --- a/Library/Homebrew/test/cask/depends_on_spec.rb +++ b/Library/Homebrew/test/cask/depends_on_spec.rb @@ -12,7 +12,7 @@ describe "Satisfy Dependencies and Requirements", :cask do describe "depends_on cask" do context "when depends_on cask is cyclic" do let(:cask) { Hbc::CaskLoader.load_from_file(TEST_FIXTURE_DIR/"cask/Casks/with-depends-on-cask-cyclic.rb") } - it { is_expected.to raise_error(Hbc::CaskCyclicCaskDependencyError) } + it { is_expected.to raise_error(Hbc::CaskCyclicDependencyError) } end context do