Teach download strategies to take a SoftwareSpec

Now that a URL, version, and the (for lack of a better term) "specs"
associated with said URL (e.g. the VCS revision, or a download strategy
hint) are neatly bundled up in a SoftwareSpec object, it doesn't make
sense to pass them individually to download strategy constructors. These
constructors now take only the formula name and a SoftwareSpec as
parameters.

This allows us to move mirror handling out out of Formula#fetch and into
the download strategies themselves. While doing so, we adjust the mirror
implementation a bit; mirrors now assume the same "specs" as their
owner's URL. They are still only useable by the CurlDownloadStrategy,
but this provides a basis for extending mirror support to other
strategies.

Signed-off-by: Jack Nagel <jacknagel@gmail.com>

Library/Homebrew/cmd/create.rb

@@ -89,7 +89,10 @@ class FormulaCreator
 
     unless ARGV.include? "--no-fetch" and version
       strategy = DownloadStrategyDetector.new(url).detect
-      @sha1 = strategy.new(url, name, version, nil).fetch.sha1 if strategy == CurlDownloadStrategy
+      spec = SoftwareSpec.new
+      spec.url(url)
+      spec.version(version)
+      @sha1 = strategy.new(name, spec).fetch.sha1 if strategy == CurlDownloadStrategy
     end
 
     path.write ERB.new(template, nil, '>').result(binding)

Library/Homebrew/download_strategy.rb

@@ -1,9 +1,12 @@
 class AbstractDownloadStrategy
-  def initialize url, name, version, specs
-    @url=url
-    case specs when Hash
-      @spec = specs.keys.first # only use first spec
-      @ref = specs.values.first
+  def initialize name, package
+    @url = package.url
+    @specs = package.specs
+
+    case @specs
+    when Hash
+      @spec = @specs.keys.first # only use first spec
+      @ref = @specs.values.first
     end
   end
 
@@ -31,9 +34,10 @@ end
 class CurlDownloadStrategy < AbstractDownloadStrategy
   attr_reader :tarball_path
 
-  def initialize url, name, version, specs
+  def initialize name, package
     super
-    @unique_token="#{name}-#{version}" unless name.to_s.empty? or name == '__UNKNOWN__'
+    @mirrors = package.mirrors
+    @unique_token = "#{name}-#{package.version}" unless name.to_s.empty? or name == '__UNKNOWN__'
     if @unique_token
       @tarball_path=HOMEBREW_CACHE+(@unique_token+ext)
     else
@@ -67,6 +71,11 @@ class CurlDownloadStrategy < AbstractDownloadStrategy
       puts "Already downloaded: #{@tarball_path}"
     end
     return @tarball_path # thus performs checksum verification
+  rescue CurlDownloadStrategyError
+    raise if @mirrors.empty?
+    puts "Trying a mirror..."
+    @url = @mirrors.shift
+    retry
   end
 
   def stage
@@ -180,15 +189,15 @@ end
 
 # This strategy extracts our binary packages.
 class CurlBottleDownloadStrategy < CurlDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
-    @tarball_path = HOMEBREW_CACHE/"#{name}-#{version}#{ext}"
+    @tarball_path = HOMEBREW_CACHE/"#{name}-#{package.version}#{ext}"
 
     unless @tarball_path.exist?
       # Stop people redownloading bottles just because I (Mike) was stupid.
-      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{version}-bottle.tar.gz"
-      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{version}.#{MacOS.cat}.bottle-bottle.tar.gz" unless old_bottle_path.exist?
-      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{version}-7.#{MacOS.cat}.bottle.tar.gz" unless old_bottle_path.exist? or name != "imagemagick"
+      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{package.version}-bottle.tar.gz"
+      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{package.version}.#{MacOS.cat}.bottle-bottle.tar.gz" unless old_bottle_path.exist?
+      old_bottle_path = HOMEBREW_CACHE/"#{name}-#{package.version}-7.#{MacOS.cat}.bottle.tar.gz" unless old_bottle_path.exist? or name != "imagemagick"
       FileUtils.mv old_bottle_path, @tarball_path if old_bottle_path.exist?
     end
   end
@@ -199,7 +208,7 @@ class CurlBottleDownloadStrategy < CurlDownloadStrategy
 end
 
 class SubversionDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--svn" unless name.to_s.empty? or name == '__UNKNOWN__'
     @unique_token += "-HEAD" if ARGV.include? '--HEAD'
@@ -304,7 +313,7 @@ class UnsafeSubversionDownloadStrategy < SubversionDownloadStrategy
 end
 
 class GitDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--git" unless name.to_s.empty? or name == '__UNKNOWN__'
     @clone=HOMEBREW_CACHE+@unique_token
@@ -397,7 +406,7 @@ class GitDownloadStrategy < AbstractDownloadStrategy
 end
 
 class CVSDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--cvs" unless name.to_s.empty? or name == '__UNKNOWN__'
     @co=HOMEBREW_CACHE+@unique_token
@@ -447,7 +456,7 @@ private
 end
 
 class MercurialDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--hg" unless name.to_s.empty? or name == '__UNKNOWN__'
     @clone=HOMEBREW_CACHE+@unique_token
@@ -488,7 +497,7 @@ class MercurialDownloadStrategy < AbstractDownloadStrategy
 end
 
 class BazaarDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--bzr" unless name.to_s.empty? or name == '__UNKNOWN__'
     @clone=HOMEBREW_CACHE+@unique_token
@@ -531,7 +540,7 @@ class BazaarDownloadStrategy < AbstractDownloadStrategy
 end
 
 class FossilDownloadStrategy < AbstractDownloadStrategy
-  def initialize url, name, version, specs
+  def initialize name, package
     super
     @unique_token="#{name}--fossil" unless name.to_s.empty? or name == '__UNKNOWN__'
     @clone=HOMEBREW_CACHE+@unique_token

Library/Homebrew/formula.rb

@@ -58,7 +58,7 @@ class Formula
 
     # If we got an explicit path, use that, else determine from the name
     @path = path.nil? ? self.class.path(name) : Pathname.new(path)
-    @downloader = download_strategy.new(@active_spec.url, name, @active_spec.version, @active_spec.specs)
+    @downloader = download_strategy.new(name, @active_spec)
   end
 
   # Derive specs from class ivars
@@ -488,28 +488,10 @@ public
 
   # For brew-fetch and others.
   def fetch
-    downloader = @downloader
-    mirror_list = case @active_spec
-    when @stable, @devel then @active_spec.mirrors
-    else []
-    end
-
     # Ensure the cache exists
     HOMEBREW_CACHE.mkpath
 
-    # TODO teach download strategies to take a SoftwareSpec
-    # object, and move mirror handling into CurlDownloadStrategy
-    begin
-      fetched = downloader.fetch
-    rescue CurlDownloadStrategyError => e
-      raise e if mirror_list.empty?
-      puts "Trying a mirror..."
-      url, specs = mirror_list.shift.values_at :url, :specs
-      downloader = download_strategy.new url, name, version, specs
-      retry
-    end
-
-    return fetched, downloader
+    return @downloader.fetch, @downloader
   end
 
   # For FormulaInstaller.
@@ -631,9 +613,9 @@ private
       @stable.version(val)
     end
 
-    def mirror val, specs=nil
+    def mirror val
       @stable ||= SoftwareSpec.new
-      @stable.mirror(val, specs)
+      @stable.mirror(val)
     end
 
     def dependencies

Library/Homebrew/formula_support.rb

@@ -60,9 +60,9 @@ class SoftwareSpec
     return @version
   end
 
-  def mirror val, specs=nil
+  def mirror val
     @mirrors ||= []
-    @mirrors << { :url => val, :specs => specs }
+    @mirrors << val
   end
 end