From cf08b71bf8dc94eaaeb1b0cde68b97a7e02ca129 Mon Sep 17 00:00:00 2001 From: Jack Nagel Date: Wed, 23 Jan 2013 00:26:28 -0600 Subject: [PATCH] FormulaInstaller: construct new ARGV from an Options collection The array of options that is passed to the spawned build process is a combination of the current ARGV, options passed in by a dependent formula, and an existing install receipt. The objects that are interacting here each expect the resulting collection to have certain properties, and the expectations are not consistent. Clear up this confusing mess by only dealing with Options collections. This keeps our representation of options uniform across the codebase. We can remove BuildOptions dependency on HomebrewArgvExtension, which allows us to pass any Array-like collection to Tab.create. The only other site inside of FormulaInstaller that uses the array is the #exec call, and there it is splatted and thus we can substitute our Options collection there as well. --- Library/Homebrew/build_options.rb | 16 +++---- Library/Homebrew/dependencies.rb | 2 +- Library/Homebrew/formula.rb | 2 +- Library/Homebrew/formula_installer.rb | 27 ++++++----- Library/Homebrew/options.rb | 34 ++++++++++++- Library/Homebrew/tab.rb | 4 +- Library/Homebrew/test/test_build_options.rb | 2 +- Library/Homebrew/test/test_options.rb | 53 +++++++++++++++++++++ 8 files changed, 112 insertions(+), 28 deletions(-) diff --git a/Library/Homebrew/build_options.rb b/Library/Homebrew/build_options.rb index 1718bc4a0f..24c49931f8 100644 --- a/Library/Homebrew/build_options.rb +++ b/Library/Homebrew/build_options.rb @@ -7,7 +7,7 @@ class BuildOptions include Enumerable def initialize args - @args = Array.new(args).extend(HomebrewArgvExtension) + @args = Options.coerce(args) @options = Options.new end @@ -37,7 +37,7 @@ class BuildOptions end def include? name - @args.include? '--' + name + args.include? '--' + name end def with? name @@ -55,11 +55,11 @@ class BuildOptions end def head? - @args.flag? '--HEAD' + args.include? '--HEAD' end def devel? - @args.include? '--devel' + args.include? '--devel' end def stable? @@ -68,21 +68,21 @@ class BuildOptions # True if the user requested a universal build. def universal? - @args.include?('--universal') && has_option?('universal') + args.include?('--universal') && has_option?('universal') end # Request a 32-bit only build. # This is needed for some use-cases though we prefer to build Universal # when a 32-bit version is needed. def build_32_bit? - @args.include?('--32-bit') && has_option?('32-bit') + args.include?('--32-bit') && has_option?('32-bit') end def used_options - Options.new((as_flags & @args.options_only).map { |o| Option.new(o) }) + Options.new(@options & @args) end def unused_options - Options.new((as_flags - @args.options_only).map { |o| Option.new(o) }) + Options.new(@options - @args) end end diff --git a/Library/Homebrew/dependencies.rb b/Library/Homebrew/dependencies.rb index 404a349032..561294fb1a 100644 --- a/Library/Homebrew/dependencies.rb +++ b/Library/Homebrew/dependencies.rb @@ -138,7 +138,7 @@ module Dependable end def options - Options.new((tags - RESERVED_TAGS).map { |o| Option.new(o) }) + Options.coerce(tags - RESERVED_TAGS) end end diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb index e5b5d58fcc..c877adb842 100644 --- a/Library/Homebrew/formula.rb +++ b/Library/Homebrew/formula.rb @@ -690,7 +690,7 @@ private end def build - @build ||= BuildOptions.new(ARGV) + @build ||= BuildOptions.new(ARGV.options_only) end def url val=nil, specs=nil diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index 1e44b18e4d..7e44182414 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -17,7 +17,7 @@ class FormulaInstaller @f = ff @show_header = false @ignore_deps = ARGV.ignore_deps? || ARGV.interactive? - @options = [] + @options = Options.new @@attempted ||= Set.new @@ -247,6 +247,17 @@ class FormulaInstaller @build_time ||= Time.now - @start_time unless pour_bottle? or ARGV.interactive? or @start_time.nil? end + def build_argv + @build_argv ||= begin + opts = Options.coerce(ARGV.options_only) + unless opts.include? '--fresh' + opts.concat(options) # from a dependent formula + opts.concat((tab.used_options rescue [])) # from a previous install + end + opts << '--build-from-source' # don't download bottle + end + end + def build FileUtils.rm Dir["#{HOMEBREW_LOGS}/#{f}/*"] @@ -261,16 +272,6 @@ class FormulaInstaller # I'm guessing this is not a good way to do this, but I'm no UNIX guru ENV['HOMEBREW_ERROR_PIPE'] = write.to_i.to_s - args = ARGV.clone - args.concat(tab.used_options.as_flags) unless tab.nil? or args.include? '--fresh' - # FIXME: enforce the download of the non-bottled package - # in the spawned Ruby process. - args << '--build-from-source' - # Add any options that were passed into this FormulaInstaller instance. - # Usually this represents options being passed by a dependent formula. - args.concat options - args.uniq! - fork do begin read.close @@ -281,7 +282,7 @@ class FormulaInstaller '-rbuild', '--', f.path, - *args.options_only + *build_argv rescue Exception => e Marshal.dump(e, write) write.close @@ -300,7 +301,7 @@ class FormulaInstaller raise "Empty installation" if Dir["#{f.prefix}/*"].empty? - Tab.create(f, args).write # INSTALL_RECEIPT.json + Tab.create(f, build_argv).write # INSTALL_RECEIPT.json rescue Exception => e ignore_interrupts do diff --git a/Library/Homebrew/options.rb b/Library/Homebrew/options.rb index 10abc1fae7..e10d9483e5 100644 --- a/Library/Homebrew/options.rb +++ b/Library/Homebrew/options.rb @@ -4,9 +4,8 @@ class Option attr_reader :name, :description, :flag def initialize(name, description=nil) - @name = name.to_s[/^(?:--)?(.+)$/, 1] + @name, @flag = split_name(name) @description = description.to_s - @flag = "--#{@name}" end def to_s @@ -29,6 +28,19 @@ class Option def hash name.hash end + + private + + def split_name(name) + case name + when /^-[a-zA-Z]$/ + [name[1..1], name] + when /^--(.+)$/ + [$1, name] + else + [name, "--#{name}"] + end + end end class Options @@ -55,6 +67,10 @@ class Options Options.new(@options - o) end + def &(o) + Options.new(@options & o) + end + def *(arg) @options.to_a * arg end @@ -71,8 +87,22 @@ class Options any? { |opt| opt == o || opt.name == o || opt.flag == o } end + def concat(o) + o.each { |opt| @options << opt } + self + end + def to_a @options.to_a end alias_method :to_ary, :to_a + + def self.coerce(arg) + case arg + when self then arg + when Option then new << arg + when Array then new(arg.map { |a| Option.new(a.to_s) }) + else raise TypeError, "Cannot convert #{arg.inspect} to Options" + end + end end diff --git a/Library/Homebrew/tab.rb b/Library/Homebrew/tab.rb index 20f7b729b4..15b0b59ea9 100644 --- a/Library/Homebrew/tab.rb +++ b/Library/Homebrew/tab.rb @@ -77,11 +77,11 @@ class Tab < OpenStruct end def used_options - Options.new(super.map { |o| Option.new(o) }) + Options.coerce(super) end def unused_options - Options.new(super.map { |o| Option.new(o) }) + Options.coerce(super) end def options diff --git a/Library/Homebrew/test/test_build_options.rb b/Library/Homebrew/test/test_build_options.rb index cc73b28b19..daf5ef96b2 100644 --- a/Library/Homebrew/test/test_build_options.rb +++ b/Library/Homebrew/test/test_build_options.rb @@ -3,7 +3,7 @@ require 'build_options' class BuildOptionsTests < Test::Unit::TestCase def setup - args = %w{--with-foo --with-bar --without-qux}.extend(HomebrewArgvExtension) + args = %w{--with-foo --with-bar --without-qux} @build = BuildOptions.new(args) @build.add("with-foo") @build.add("with-bar") diff --git a/Library/Homebrew/test/test_options.rb b/Library/Homebrew/test/test_options.rb index 065c00a826..78edb408cc 100644 --- a/Library/Homebrew/test/test_options.rb +++ b/Library/Homebrew/test/test_options.rb @@ -38,6 +38,12 @@ class OptionTests < Test::Unit::TestCase assert_empty @option.description assert_equal "foo", Option.new("foo", "foo").description end + + def test_preserves_short_options + option = Option.new("-d") + assert_equal "-d", option.flag + assert_equal "d", option.name + end end class OptionsTests < Test::Unit::TestCase @@ -87,4 +93,51 @@ class OptionsTests < Test::Unit::TestCase @options << option assert_equal [option], @options.to_ary end + + def test_concat_array + option = Option.new("foo") + @options.concat([option]) + assert @options.include?(option) + assert_equal [option], @options.to_a + end + + def test_concat_options + option = Option.new("foo") + opts = Options.new + opts << option + @options.concat(opts) + assert @options.include?(option) + assert_equal [option], @options.to_a + end + + def test_concat_returns_self + assert_same @options, (@options.concat([])) + end + + def test_intersection + foo, bar, baz = %w{foo bar baz}.map { |o| Option.new(o) } + options = Options.new << foo << bar + @options << foo << baz + assert_equal [foo], (@options & options).to_a + end + + def test_coerce_with_options + assert_same @options, Options.coerce(@options) + end + + def test_coerce_with_option + option = Option.new("foo") + assert_equal option, Options.coerce(option).to_a.first + end + + def test_coerce_with_array + array = %w{--foo --bar} + option1 = Option.new("foo") + option2 = Option.new("bar") + assert_equal [option1, option2].sort, Options.coerce(array).to_a.sort + end + + def test_coerce_raises_for_inappropriate_types + assert_raises(TypeError) { Options.coerce(1) } + end end