Make bottle implementation more generic

Library/Homebrew/bottles.rb

@@ -3,12 +3,9 @@ require 'os/mac'
 require 'extend/ARGV'
 require 'bottle_version'
 
-def bottle_filename f, options={}
+def bottle_filename options={}
   options = { :tag => bottle_tag }.merge(options)
-  name = f.name.downcase
+  "#{options[:name]}-#{options[:version]}#{bottle_native_suffix(options)}"
-  version = PkgVersion.new(f.stable.version, f.revision)
-  options[:revision] ||= f.bottle.revision.to_i if f.bottle
-  "#{name}-#{version}#{bottle_native_suffix(options)}"
 end
 
 def install_bottle? f, options={:warn=>false}
@@ -16,7 +13,8 @@ def install_bottle? f, options={:warn=>false}
   return true if ARGV.force_bottle?
   return false unless f.pour_bottle?
   return false unless f.default_build?
-  return false unless bottle_current?(f)
+  return false unless f.bottle
+
   if f.bottle.cellar != :any && f.bottle.cellar != HOMEBREW_CELLAR.to_s
     if options[:warn]
       opoo "Building source; cellar of #{f}'s bottle is #{f.bottle.cellar}"
@@ -33,10 +31,6 @@ def built_as_bottle? f
   tab.built_as_bottle
 end
 
-def bottle_current? f
-  f.bottle and f.bottle.url and not f.bottle.checksum.empty?
-end
-
 def bottle_file_outdated? f, file
   filename = file.basename.to_s
   return nil unless f and f.bottle and f.bottle.url \
@@ -66,8 +60,8 @@ def bottle_regex
   Pathname::BOTTLE_EXTNAME_RX
 end
 
-def bottle_url f, tag=bottle_tag
+def bottle_url(root_url, filename_options={})
-  "#{f.bottle.root_url}/#{bottle_filename(f, :tag => tag)}"
+  "#{root_url}/#{bottle_filename(filename_options)}"
 end
 
 def bottle_tag

Library/Homebrew/formula.rb

@@ -18,7 +18,7 @@ class Formula
   extend BuildEnvironmentDSL
 
   attr_reader :name, :path, :homepage, :build
-  attr_reader :stable, :bottle, :devel, :head, :active_spec
+  attr_reader :stable, :devel, :head, :active_spec
   attr_reader :pkg_version, :revision
 
   # The current working directory during builds and tests.
@@ -41,31 +41,11 @@ class Formula
     set_spec :stable
     set_spec :devel
     set_spec :head
-    set_spec :bottle do |bottle|
-      # Ensure the bottle URL is set. If it does not have a checksum,
-      # then a bottle is not available for the current platform.
-      # TODO: push this down into Bottle; we can pass the formula instance
-      # into a validation method on the bottle instance.
-      unless bottle.checksum.nil? || bottle.checksum.empty?
-        @bottle = bottle
-        bottle.url ||= bottle_url(self, bottle.current_tag)
-        bottle.version = PkgVersion.new(stable.version, revision)
-      end
-    end
 
     @active_spec = determine_active_spec
     validate_attributes :url, :name, :version
     @build = determine_build_options
-
+    @pkg_version = PkgVersion.new(version, revision)
-    # TODO: @pkg_version is already set for bottles, since constructing it
-    # requires passing in the active_spec version. This should be fixed by
-    # making the bottle an attribute of SoftwareSpec rather than a separate
-    # spec itself.
-    if active_spec == bottle
-      @pkg_version = bottle.version
-    else
-      @pkg_version = PkgVersion.new(version, revision)
-    end
 
     @pin = FormulaPin.new(self)
 
@@ -74,21 +54,16 @@ class Formula
 
   def set_spec(name)
     spec = self.class.send(name)
-    if block_given? && yield(spec) || spec.url
+    if spec.url
       spec.owner = self
       instance_variable_set("@#{name}", spec)
     end
   end
 
-  def select_bottle?
-    !ARGV.build_bottle? && install_bottle?(self)
-  end
-
   def determine_active_spec
     case
     when head && ARGV.build_head?        then head    # --HEAD
     when devel && ARGV.build_devel?      then devel   # --devel
-    when bottle && select_bottle?        then bottle  # bottle available
     when stable                          then stable
     when devel && stable.nil?            then devel   # devel-only
     when head && stable.nil?             then head    # head-only
@@ -115,6 +90,10 @@ class Formula
     build
   end
 
+  def bottle
+    Bottle.new(self, active_spec.bottle_specification) if active_spec.bottled?
+  end
+
   def url;      active_spec.url;     end
   def version;  active_spec.version; end
   def mirrors;  active_spec.mirrors; end
@@ -699,7 +678,7 @@ class Formula
     attr_rw :homepage, :plist_startup, :plist_manual, :revision
 
     def specs
-      @specs ||= [stable, devel, head, bottle].freeze
+      @specs ||= [stable, devel, head].freeze
     end
 
     def url val, specs={}
@@ -722,6 +701,10 @@ class Formula
       EOS
     end
 
+    def bottle *, &block
+      stable.bottle(&block)
+    end
+
     def build
       stable.build
     end
@@ -732,13 +715,6 @@ class Formula
       @stable.instance_eval(&block)
     end
 
-    def bottle *, &block
-      @bottle ||= Bottle.new
-      return @bottle unless block_given?
-      @bottle.instance_eval(&block)
-      @bottle.version = @stable.version
-    end
-
     def devel &block
       @devel ||= SoftwareSpec.new
       return @devel unless block_given?

Library/Homebrew/software_spec.rb

@@ -12,6 +12,7 @@ class SoftwareSpec
   attr_reader :name
   attr_reader :build, :resources, :owner
   attr_reader :dependency_collector
+  attr_reader :bottle_specification
 
   def_delegators :@resource, :stage, :fetch, :verify_download_integrity
   def_delegators :@resource, :cached_download, :clear_cache
@@ -23,6 +24,7 @@ class SoftwareSpec
     @resources = {}
     @build = BuildOptions.new(ARGV.options_only)
     @dependency_collector = DependencyCollector.new
+    @bottle_specification = BottleSpecification.new
   end
 
   def owner= owner
@@ -41,6 +43,14 @@ class SoftwareSpec
     dependency_collector.add(@resource)
   end
 
+  def bottled?
+    bottle_specification.fully_specified?
+  end
+
+  def bottle &block
+    bottle_specification.instance_eval(&block)
+  end
+
   def resource? name
     resources.has_key?(name)
   end
@@ -87,20 +97,47 @@ class HeadSoftwareSpec < SoftwareSpec
   end
 end
 
-class Bottle < SoftwareSpec
+class Bottle
-  attr_rw :root_url, :prefix, :cellar, :revision
+  extend Forwardable
-  attr_accessor :current_tag
 
-  def_delegators :@resource, :version=, :url=
+  attr_reader :resource, :prefix, :cellar, :revision
+
+  def_delegators :resource, :url, :fetch, :verify_download_integrity
+  def_delegators :resource, :downloader, :cached_download, :clear_cache
+
+  def initialize(f, spec)
+    @resource = Resource.new
+    @resource.owner = f
+    @resource.url = bottle_url(
+      spec.root_url,
+      :name => f.name,
+      :version => f.pkg_version,
+      :revision => spec.revision,
+      :tag => spec.current_tag
+    )
+    @resource.version = f.pkg_version
+    @resource.checksum = spec.checksum
+    @prefix = spec.prefix
+    @cellar = spec.cellar
+    @revision = spec.revision
+  end
+end
+
+class BottleSpecification
+  attr_rw :root_url, :prefix, :cellar, :revision
+  attr_reader :current_tag, :checksum
 
   def initialize
-    super
     @revision = 0
     @prefix = '/usr/local'
     @cellar = '/usr/local/Cellar'
     @root_url = 'https://downloads.sf.net/project/machomebrew/Bottles'
   end
 
+  def fully_specified?
+    checksum && !checksum.empty?
+  end
+
   # Checksum methods in the DSL's bottle block optionally take
   # a Hash, which indicates the platform the checksum applies on.
   Checksum::TYPES.each do |cksum|
@@ -115,7 +152,7 @@ class Bottle < SoftwareSpec
         end
 
         cksum, current_tag = @#{cksum}.fetch_bottle_for(bottle_tag)
-        @resource.checksum = cksum if cksum
+        @checksum = cksum if cksum
         @current_tag = current_tag if cksum
       end
     EOS

Library/Homebrew/test/test_formula.rb

@@ -182,7 +182,6 @@ class FormulaTests < Test::Unit::TestCase
     assert_equal f.stable, f.active_spec
 
     assert_instance_of SoftwareSpec, f.stable
-    assert_instance_of Bottle, f.bottle
     assert_instance_of SoftwareSpec, f.devel
     assert_instance_of HeadSoftwareSpec, f.head
   end
@@ -216,7 +215,7 @@ class FormulaTests < Test::Unit::TestCase
   def test_class_specs_are_always_initialized
     f = formula { url 'foo-1.0' }
 
-    %w{stable devel head bottle}.each do |spec|
+    %w{stable devel head}.each do |spec|
       assert_kind_of SoftwareSpec, f.class.send(spec)
     end
   end
@@ -224,7 +223,7 @@ class FormulaTests < Test::Unit::TestCase
   def test_incomplete_instance_specs_are_not_accessible
     f = formula { url 'foo-1.0' }
 
-    %w{devel head bottle}.each { |spec| assert_nil f.send(spec) }
+    %w{devel head}.each { |spec| assert_nil f.send(spec) }
   end
 
   def test_honors_attributes_declared_before_specs
@@ -234,7 +233,7 @@ class FormulaTests < Test::Unit::TestCase
       devel { url 'foo-1.1' }
     end
 
-    %w{stable devel head bottle}.each do |spec|
+    %w{stable devel head}.each do |spec|
       assert_equal 'foo', f.class.send(spec).deps.first.name
     end
   end

Library/Homebrew/test/test_formula_spec_selection.rb

@@ -34,17 +34,6 @@ class FormulaSpecSelectionTests < Test::Unit::TestCase
     assert_spec_selected :devel
   end
 
-  def test_selects_bottle_when_available
-    formula do
-      def install_bottle?(*); true; end
-
-      url 'foo-1.0'
-      bottle { sha1 TEST_SHA1 => bottle_tag }
-    end
-
-    assert_spec_selected :bottle
-  end
-
   def test_selects_stable_by_default
     formula do
       url 'foo-1.0'
@@ -119,14 +108,4 @@ class FormulaSpecSelectionTests < Test::Unit::TestCase
     assert_spec_unset :devel
     assert_spec_selected :stable
   end
-
-  def test_incomplete_bottle_not_set
-    formula do
-      url 'foo-1.0'
-      bottle { sha1 TEST_SHA1 => :some_nonexistent_thing }
-    end
-
-    assert_spec_unset :bottle
-    assert_spec_selected :stable
-  end
 end

Library/Homebrew/test/test_software_spec.rb

@@ -98,9 +98,9 @@ class HeadSoftwareSpecTests < Test::Unit::TestCase
   end
 end
 
-class BottleTests < Test::Unit::TestCase
+class BottleSpecificationTests < Test::Unit::TestCase
   def setup
-    @spec = Bottle.new
+    @spec = BottleSpecification.new
   end
 
   def test_checksum_setters