Eliminate some indirection in evaluating requirement env blocks

2 files changed, 34 insertions, 17 deletions

diff --git a/Library/Homebrew/requirement.rb b/Library/Homebrew/requirement.rb
index 796dc4f3b..8c79f30c6 100644
--- a/Library/Homebrew/requirement.rb
+++ b/Library/Homebrew/requirement.rb
@@ -24,11 +24,8 @@ class Requirement
   # Overriding #satisfied? is deprecated.
   # Pass a block or boolean to the satisfy DSL method instead.
   def satisfied?
-    result = self.class.satisfy.yielder do |proc|
-      instance_eval(&proc)
-    end
-
-    infer_env_modification(result)
+    result = self.class.satisfy.yielder { |p| instance_eval(&p) }
+    @satisfied_result = result
     !!result
   end
 
@@ -51,13 +48,31 @@ class Requirement
   # Note: #satisfied? should be called before invoking this method
   # as the env modifications may depend on its side effects.
   def modify_build_environment
-    env.modify_build_environment(self)
+    instance_eval(&env_proc) if env_proc
+
+    # XXX If the satisfy block returns a Pathname, then make sure that it
+    # remains available on the PATH. This makes requirements like
+    #   satisfy { which("executable") }
+    # work, even under superenv where "executable" wouldn't normally be on the
+    # PATH.
+    # This is undocumented magic and it should be removed, but we need to add
+    # a way to declare path-based requirements that work with superenv first.
+    if Pathname === @satisfied_result
+      parent = @satisfied_result.parent
+      unless ENV["PATH"].split(File::PATH_SEPARATOR).include?(parent.to_s)
+        ENV.append_path("PATH", parent)
+      end
+    end
   end
 
   def env
     self.class.env
   end
 
+  def env_proc
+    self.class.env_proc
+  end
+
   def eql?(other)
     instance_of?(other.class) && name == other.name && tags == other.tags
   end
@@ -85,17 +100,6 @@ class Requirement
     klass.downcase
   end
 
-  def infer_env_modification(o)
-    case o
-    when Pathname
-      self.class.env do
-        unless ENV["PATH"].split(File::PATH_SEPARATOR).include?(o.parent.to_s)
-          ENV.append_path("PATH", o.parent)
-        end
-      end
-    end
-  end
-
   def which(cmd)
     super(cmd, ORIGINAL_PATHS.join(File::PATH_SEPARATOR))
   end
@@ -103,6 +107,7 @@ class Requirement
   class << self
     include BuildEnvironmentDSL
 
+    attr_reader :env_proc
     attr_rw :fatal, :default_formula
     # build is deprecated, use `depends_on <requirement> => :build` instead
     attr_rw :build
@@ -110,6 +115,14 @@ class Requirement
     def satisfy(options={}, &block)
       @satisfied ||= Requirement::Satisfier.new(options, &block)
     end
+
+    def env(*settings, &block)
+      if block_given?
+        @env_proc = block
+      else
+        super
+      end
+    end
   end
 
   class Satisfier
diff --git a/Library/Homebrew/test/test_requirement.rb b/Library/Homebrew/test/test_requirement.rb
index 1c57cc3eb..a3fd117eb 100644
--- a/Library/Homebrew/test/test_requirement.rb
+++ b/Library/Homebrew/test/test_requirement.rb
@@ -123,6 +123,10 @@ class RequirementTests < Homebrew::TestCase
     end
   end
 
+  def test_modify_build_environment_without_env_proc
+    assert_nil Class.new(Requirement).new.modify_build_environment
+  end
+
   def test_eql
     a, b = Requirement.new, Requirement.new
     assert_eql a, b