Improved formula attribute validation

The initializer for Formula does a number of validations, but it does
them in a weird order, and some attributes aren't validated under
certain circumstances. This became even more of a mess when most
software package attributes were moved into the SoftwareSpec class.

This commit removes the last vestiges of storing these attributes as
instance variables. In particular, it eliminates #set_instance_variable
and #validate_variable, replacing them with methods that operate on
SoftwareSpec instances, and generate more useful errors.

Doing these validations unconditionally in the initializer means we bail
out much earlier if the formula has invalid attributes or is not fully
specified, and no longer need to validate in #prefix.

Technically we don't need to validate in #brew either, but we continue
to do so anyway as a safety measure, and because we cannot enforce calls
to super in subclasses.

Library/Homebrew/exceptions.rb

@@ -22,6 +22,17 @@ class NoSuchKegError < RuntimeError
   end
 end
 
+class FormulaValidationError < StandardError
+  attr_reader :attr
+
+  def initialize(attr, value)
+    @attr = attr
+    msg = "invalid attribute: #{attr}"
+    msg << " (#{value.inspect})" unless value.empty?
+    super msg
+  end
+end
+
 class FormulaUnavailableError < RuntimeError
   attr_reader :name
   attr_accessor :dependent

Library/Homebrew/formula.rb

@@ -24,44 +24,31 @@ class Formula
 
   # Homebrew determines the name
   def initialize name='__UNKNOWN__', path=nil
-    set_instance_variable :homepage
-    set_instance_variable :stable
-    set_instance_variable :bottle
-    set_instance_variable :devel
-    set_instance_variable :head
-
     @name = name
-    validate_variable :name
-
-    # If a checksum or version was set in the DSL, but no stable URL
-    # was defined, make @stable nil and save callers some trouble
-    @stable = nil if @stable and @stable.url.nil?
-
-    # Ensure the bottle URL is set. If it does not have a checksum,
-    # then a bottle is not available for the current platform.
-    if @bottle and not (@bottle.checksum.nil? or @bottle.checksum.empty?)
-      @bottle.url ||= bottle_url(self)
-      if @bottle.cat_without_underscores
-        @bottle.url.gsub!(MacOS.cat.to_s, MacOS.cat_without_underscores.to_s)
-      end
-    else
-      @bottle = nil
-    end
-
-    @active_spec = if @head and ARGV.build_head? then @head # --HEAD
-      elsif @devel and ARGV.build_devel? then @devel        # --devel
-      elsif @bottle and install_bottle?(self) then @bottle  # bottle available
-      elsif @stable.nil? and @head then @head               # head-only
-      else @stable                                          # default
-      end
-
-    @version = @active_spec.version
-    validate_variable :version if @version
-
-    raise "No url provided for formula #{name}" if @active_spec.url.nil?
-
     # If we got an explicit path, use that, else determine from the name
     @path = path.nil? ? self.class.path(name) : Pathname.new(path)
+    @homepage = self.class.homepage
+
+    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)
+        if bottle.cat_without_underscores
+          bottle.url.gsub!(MacOS.cat.to_s,
+                           MacOS.cat_without_underscores.to_s)
+        end
+      end
+    end
+
+    @active_spec = determine_active_spec
+    validate_attributes :url, :name, :version
     @downloader = download_strategy.new(name, @active_spec)
 
     # Combine DSL `option` and `def options`
@@ -73,10 +60,36 @@ class Formula
     @pin = FormulaPin.new(self)
   end
 
-  def url;      @active_spec.url;     end
-  def version;  @active_spec.version; end
-  def specs;    @active_spec.specs;   end
-  def mirrors;  @active_spec.mirrors; end
+  def set_spec(name)
+    spec = self.class.send(name)
+    return if spec.nil?
+    if block_given? && yield(spec) || !spec.url.nil?
+      instance_variable_set("@#{name}", spec)
+    end
+  end
+
+  def determine_active_spec
+    case
+    when @head && ARGV.build_head?        then @head    # --HEAD
+    when @devel && ARGV.build_devel?      then @devel   # --devel
+    when @bottle && install_bottle?(self) then @bottle  # bottle available
+    when @stable.nil? && @head            then @head    # head-only
+    else @stable
+    end
+  end
+
+  def validate_attributes(*attrs)
+    attrs.each do |attr|
+      if (value = send(attr).to_s).empty? || value =~ /\s/
+        raise FormulaValidationError.new(attr, value)
+      end
+    end
+  end
+
+  def url;      active_spec.url;     end
+  def version;  active_spec.version; end
+  def specs;    active_spec.specs;   end
+  def mirrors;  active_spec.mirrors; end
 
   # if the dir is there, but it's empty we consider it not installed
   def installed?
@@ -100,21 +113,21 @@ class Formula
   end
 
   def linked_keg
-    HOMEBREW_REPOSITORY/'Library/LinkedKegs'/@name
+    HOMEBREW_REPOSITORY/'Library/LinkedKegs'/name
   end
 
   def installed_prefix
-    devel_prefix = unless @devel.nil?
-      HOMEBREW_CELLAR/@name/@devel.version
+    devel_prefix = unless devel.nil?
+      HOMEBREW_CELLAR/name/devel.version
     end
 
-    head_prefix = unless @head.nil?
-      HOMEBREW_CELLAR/@name/@head.version
+    head_prefix = unless head.nil?
+      HOMEBREW_CELLAR/name/head.version
     end
 
-    if @active_spec == @head || @head and head_prefix.directory?
+    if active_spec == head || head and head_prefix.directory?
       head_prefix
-    elsif @active_spec == @devel || @devel and devel_prefix.directory?
+    elsif active_spec == devel || devel and devel_prefix.directory?
       devel_prefix
     else
       prefix
@@ -127,9 +140,7 @@ class Formula
   end
 
   def prefix
-    validate_variable :name
-    validate_variable :version
-    HOMEBREW_CELLAR/@name/@version
+    HOMEBREW_CELLAR/name/version
   end
   def rack; prefix.parent end
 
@@ -174,11 +185,11 @@ class Formula
   def opt_prefix; HOMEBREW_PREFIX/:opt/name end
 
   def download_strategy
-    @active_spec.download_strategy
+    active_spec.download_strategy
   end
 
   def cached_download
-    @downloader.cached_location
+    downloader.cached_location
   end
 
   # Can be overridden to selectively disable bottles from formulae.
@@ -237,8 +248,7 @@ class Formula
 
   # yields self with current working directory set to the uncompressed tarball
   def brew
-    validate_variable :name
-    validate_variable :version
+    validate_attributes :name, :version
 
     stage do
       begin
@@ -599,12 +609,12 @@ class Formula
   def fetch
     # Ensure the cache exists
     HOMEBREW_CACHE.mkpath
-    return @downloader.fetch, @downloader
+    return downloader.fetch, downloader
   end
 
   # For FormulaInstaller.
   def verify_download_integrity fn
-    @active_spec.verify_download_integrity(fn)
+    active_spec.verify_download_integrity(fn)
   end
 
   def test
@@ -655,17 +665,6 @@ class Formula
     end
   end
 
-  def validate_variable name
-    v = instance_variable_get("@#{name}")
-    raise "Invalid @#{name}" if v.to_s.empty? or v.to_s =~ /\s/
-  end
-
-  def set_instance_variable(type)
-    return if instance_variable_defined? "@#{type}"
-    class_value = self.class.send(type)
-    instance_variable_set("@#{type}", class_value) if class_value
-  end
-
   def self.method_added method
     case method
     when :brew

Library/Homebrew/formula_specialties.rb

@@ -10,10 +10,9 @@ end
 # See flac.rb for an example
 class GithubGistFormula < ScriptFileFormula
   def initialize name='__UNKNOWN__', path=nil
-    super name, path
-    @stable.version(File.basename(File.dirname(url))[0,6])
-    @version = @active_spec.version
-    validate_variable :version
+    url = self.class.stable.url
+    self.class.stable.version(File.basename(File.dirname(url))[0,6])
+    super
   end
 end
 

Library/Homebrew/test/test_formula.rb

@@ -1,10 +1,6 @@
 require 'testing_env'
 require 'test/testball'
 
-class MostlyAbstractFormula < Formula
-  url ''
-end
-
 class FormulaTests < Test::Unit::TestCase
   include VersionAssertions
 
@@ -58,19 +54,6 @@ class FormulaTests < Test::Unit::TestCase
     assert_equal 'FooBar', Formula.class_s('foo_bar')
   end
 
-  def test_cant_override_brew
-    assert_raises(RuntimeError) do
-      Class.new(Formula) { def brew; end }
-    end
-  end
-
-  def test_abstract_formula
-    f=MostlyAbstractFormula.new
-    assert_equal '__UNKNOWN__', f.name
-    assert_raises(RuntimeError) { f.prefix }
-    shutup { assert_raises(RuntimeError) { f.brew } }
-  end
-
   def test_mirror_support
     f = Class.new(Formula) do
       url "file:///#{TEST_FOLDER}/bad_url/testball-0.1.tbz"
@@ -286,7 +269,7 @@ class FormulaTests < Test::Unit::TestCase
       f << %{
         require 'formula'
         class #{Formula.class_s(foobar)} < Formula
-          url ''
+          url 'foo-1.0'
           def initialize(*args)
             @homepage = 'http://example.com/'
             super

Library/Homebrew/test/test_formula_validation.rb

@@ -0,0 +1,69 @@
+require 'testing_env'
+require 'formula'
+
+class FormulaValidationTests < Test::Unit::TestCase
+  def formula(*args, &block)
+    Class.new(Formula, &block).new(*args)
+  end
+
+  def assert_invalid(attr, &block)
+    e = assert_raises(FormulaValidationError, &block)
+    assert_equal attr, e.attr
+  end
+
+  def test_cant_override_brew
+    assert_raises(RuntimeError) do
+      Class.new(Formula) { def brew; end }
+    end
+  end
+
+  def test_validates_name
+    assert_invalid :name do
+      formula "name with spaces" do
+        url "foo"
+        version "1.0"
+      end
+    end
+  end
+
+  def test_validates_url
+    assert_invalid :url do
+      formula do
+        url ""
+        version "1"
+      end
+    end
+  end
+
+  def test_validates_version
+    assert_invalid :version do
+      formula do
+        url "foo"
+        version "version with spaces"
+      end
+    end
+
+    assert_invalid :version do
+      formula do
+        url "foo"
+        version ""
+      end
+    end
+  end
+
+  def test_validates_when_initialize_overridden
+    assert_invalid :name do
+      formula do
+        def initialize; end
+      end.brew {}
+    end
+  end
+
+  def test_head_only_valid
+    assert_nothing_raised do
+      formula do
+        head "foo"
+      end
+    end
+  end
+end

Library/Homebrew/test/test_versions.rb

@@ -50,14 +50,6 @@ class VersionParsingTests < Test::Unit::TestCase
     assert_version_nil 'foo'
   end
 
-  def test_bad_version
-    assert_raises(RuntimeError) do
-      Class.new(TestBall) do
-        version "versions can't have spaces"
-      end.new
-    end
-  end
-
   def test_version_all_dots
     assert_version_detected '1.14', 'http://example.com/foo.bar.la.1.14.zip'
   end