From 2e26afe55675e0ffea472c5214000e711f0489eb Mon Sep 17 00:00:00 2001 From: Jack Nagel Date: Sat, 13 Apr 2013 17:40:12 -0500 Subject: [PATCH] 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 | 11 ++ Library/Homebrew/formula.rb | 131 +++++++++--------- Library/Homebrew/formula_specialties.rb | 7 +- Library/Homebrew/test/test_formula.rb | 19 +-- .../Homebrew/test/test_formula_validation.rb | 69 +++++++++ Library/Homebrew/test/test_versions.rb | 8 -- 6 files changed, 149 insertions(+), 96 deletions(-) create mode 100644 Library/Homebrew/test/test_formula_validation.rb diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index d57f5ddc3b..b5299c0384 100644 --- a/Library/Homebrew/exceptions.rb +++ b/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 diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index a6305eaaff..eddacf08dd 100644 --- a/Library/Homebrew/formula.rb +++ b/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 diff --git a/Library/Homebrew/formula_specialties.rb b/Library/Homebrew/formula_specialties.rb index 4e63440a6f..daf9dd6d01 100644 --- a/Library/Homebrew/formula_specialties.rb +++ b/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 diff --git a/Library/Homebrew/test/test_formula.rb b/Library/Homebrew/test/test_formula.rb index 30ef41589d..434dd3ac12 100644 --- a/Library/Homebrew/test/test_formula.rb +++ b/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 diff --git a/Library/Homebrew/test/test_formula_validation.rb b/Library/Homebrew/test/test_formula_validation.rb new file mode 100644 index 0000000000..be4c38c641 --- /dev/null +++ b/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 diff --git a/Library/Homebrew/test/test_versions.rb b/Library/Homebrew/test/test_versions.rb index d503f4acec..bf943637ec 100644 --- a/Library/Homebrew/test/test_versions.rb +++ b/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