Moving all confine code out of the Provider class, and fixing #1197.

I created a Confiner module for the Provider class methods,
and then I enhanced the interface between it and the Confine
class to make sure binary paths are searched for fresh each time.

This fixes #1197, which was a result of binary paths being
searched for at startup, rather than at execution.

8 files changed, 274 insertions, 151 deletions

diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index e0de9d61d..c02e15029 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -7,6 +7,8 @@ class Puppet::Provider
 
     require 'puppet/provider/confiner'
 
+    extend Puppet::Provider::Confiner
+
     Puppet::Util.logmethods(self, true)
 
     class << self
@@ -42,31 +44,16 @@ class Puppet::Provider
                 [name, self.name]
         end
 
-        if command == :missing
-            return nil
-        end
-
-        command
+        return binary(command)
     end
 
     # Define commands that are not optional.
     def self.commands(hash)
         optional_commands(hash) do |name, path|
-            confine :exists => path
+            confine :exists => path, :for_binary => true
         end
     end
 
-    def self.confine(hash)
-        confiner.confine(hash)
-    end
-
-    def self.confiner
-        unless defined?(@confiner)
-            @confiner = Puppet::Provider::Confiner.new
-        end
-        @confiner
-    end
-
     # Is the provided feature a declared feature?
     def self.declared_feature?(name)
         defined?(@declared_features) and @declared_features.include?(name)
@@ -111,7 +98,6 @@ class Puppet::Provider
     def self.initvars
         @defaults = {}
         @commands = {}
-        @origcommands = {}
     end
 
     # The method for returning a list of provider instances.  Note that it returns providers, preferably with values already
@@ -180,16 +166,7 @@ class Puppet::Provider
     def self.optional_commands(hash)
         hash.each do |name, path|
             name = symbolize(name)
-            @origcommands[name] = path
-
-            # Try to find the full path (or verify already-full paths); otherwise
-            # store that the command is missing so we know it's defined but absent.
-            if tmp = binary(path)
-                path = tmp
-                @commands[name] = path
-            else
-                @commands[name] = :missing
-            end
+            @commands[name] = path
 
             if block_given?
                 yield(name, path)
@@ -208,12 +185,6 @@ class Puppet::Provider
         @source
     end
 
-    # Check whether this implementation is suitable for our platform.
-    def self.suitable?(short = true)
-        return confiner.valid?  if short
-        return confiner.result
-    end
-
     # Does this provider support the specified parameter?
     def self.supports_parameter?(param)
         if param.is_a?(Class)
@@ -252,8 +223,8 @@ class Puppet::Provider
     end
 
     dochook(:commands) do
-        if @origcommands.length > 0
-            return "  Required binaries: " + @origcommands.collect do |n, c|
+        if @commands.length > 0
+            return "  Required binaries: " + @commands.collect do |n, c|
                 "``#{c}``"
             end.join(", ") + "."
         end
diff --git a/lib/puppet/provider/confine.rb b/lib/puppet/provider/confine.rb
index e55ba02ec..227c923e6 100644
--- a/lib/puppet/provider/confine.rb
+++ b/lib/puppet/provider/confine.rb
@@ -1,9 +1,22 @@
 # The class that handles testing whether our providers
 # actually work or not.
+require 'puppet/util'
+
 class Puppet::Provider::Confine
+    include Puppet::Util
+
     attr_reader :test, :values, :fact
 
+    # Mark that this confine is used for testing binary existence.
+    attr_accessor :for_binary
+    def for_binary?
+        for_binary
+    end
+
     def exists?(value)
+        if for_binary?
+            return false unless value = binary(value)
+        end
         value and FileTest.exist?(value)
     end
 
@@ -57,11 +70,11 @@ class Puppet::Provider::Confine
         values.each do |value|
             unless send(@method, value)
                 msg = case test
-                when :false:  "false value"
-                when :true:  "true value"
-                when :exists:  "file %s does not exist" % value
-                when :facter:  "facter value '%s' for '%s' not in required list '%s'" % [value, @fact, values.join(",")]
-                end
+                      when :false:  "false value when expecting true"
+                      when :true:  "true value when expecting false"
+                      when :exists:  "file %s does not exist" % value
+                      when :facter:  "facter value '%s' for '%s' not in required list '%s'" % [value, @fact, values.join(",")]
+                      end
                 Puppet.debug msg
                 return false
             end
diff --git a/lib/puppet/provider/confine_collection.rb b/lib/puppet/provider/confine_collection.rb
new file mode 100644
index 000000000..f38035521
--- /dev/null
+++ b/lib/puppet/provider/confine_collection.rb
@@ -0,0 +1,48 @@
+# Manage a collection of confines, returning a boolean or
+# helpful information.
+require 'puppet/provider/confine'
+
+class Puppet::Provider::ConfineCollection
+    def confine(hash)
+        if hash.include?(:for_binary)
+            for_binary = true
+            hash.delete(:for_binary)
+        else
+            for_binary = false
+        end
+        hash.each do |test, values|
+            @confines << Puppet::Provider::Confine.new(test, values)
+            @confines[-1].for_binary = true if for_binary
+        end
+    end
+
+    def initialize
+        @confines = []
+    end
+
+    # Return a hash of the whole confine set, used for the Provider
+    # reference.
+    def result
+        defaults = {
+            :false => 0,
+            :true => 0,
+            :exists => [],
+            :facter => {}
+        }
+        missing = Hash.new { |hash, key| hash[key] = defaults[key] }
+        @confines.each do |confine|
+            case confine.test
+            when :false: missing[confine.test] += confine.result.find_all { |v| v == false }.length
+            when :true: missing[confine.test] += confine.result.find_all { |v| v == true }.length
+            when :exists: confine.result.zip(confine.values).each { |val, f| missing[:exists] << f unless val }
+            when :facter: missing[:facter][confine.fact] = confine.values if confine.result.include?(false)
+            end
+        end
+
+        missing
+    end
+
+    def valid?
+        ! @confines.detect { |c| ! c.valid? }
+    end
+end
diff --git a/lib/puppet/provider/confiner.rb b/lib/puppet/provider/confiner.rb
index b88f1f01b..3e406873b 100644
--- a/lib/puppet/provider/confiner.rb
+++ b/lib/puppet/provider/confiner.rb
@@ -1,41 +1,20 @@
-# Manage a collection of confines, returning a boolean or
-# helpful information.
-require 'puppet/provider/confine'
+require 'puppet/provider/confine_collection'
 
-class Puppet::Provider::Confiner
+module Puppet::Provider::Confiner
     def confine(hash)
-        hash.each do |test, values|
-            @confines << Puppet::Provider::Confine.new(test, values)
-        end
-    end
-
-    def initialize
-        @confines = []
+        confine_collection.confine(hash)
     end
 
-    # Return a hash of the whole confine set, used for the Provider
-    # reference.
-    def result
-        defaults = {
-            :false => 0,
-            :true => 0,
-            :exists => [],
-            :facter => {}
-        }
-        missing = Hash.new { |hash, key| hash[key] = defaults[key] }
-        @confines.each do |confine|
-            case confine.test
-            when :false: missing[confine.test] += confine.result.find_all { |v| v == false }.length
-            when :true: missing[confine.test] += confine.result.find_all { |v| v == true }.length
-            when :exists: confine.result.zip(confine.values).each { |val, f| missing[:exists] << f unless val }
-            when :facter: missing[:facter][confine.fact] = confine.values if confine.result.include?(false)
-            end
+    def confine_collection
+        unless defined?(@confine_collection)
+            @confine_collection = Puppet::Provider::ConfineCollection.new
         end
-
-        missing
+        @confine_collection
     end
 
-    def valid?
-        ! @confines.detect { |c| ! c.valid? }
+    # Check whether this implementation is suitable for our platform.
+    def suitable?(short = true)
+        return confine_collection.valid? if short
+        return confine_collection.result
     end
 end
diff --git a/spec/unit/provider/confine.rb b/spec/unit/provider/confine.rb
index 0e87ccdfb..bb2e751d6 100755
--- a/spec/unit/provider/confine.rb
+++ b/spec/unit/provider/confine.rb
@@ -25,6 +25,24 @@ describe Puppet::Provider::Confine do
         Puppet::Provider::Confine.new(:foo, :bar).should respond_to(:fact)
     end
 
+    it "should be possible to mark the confine as a binary test" do
+        Puppet::Provider::Confine.new(:foo, :bar).should respond_to(:for_binary=)
+    end
+
+    it "should have a boolean method to indicate it's a binary confine" do
+        Puppet::Provider::Confine.new(:foo, :bar).should respond_to(:for_binary?)
+    end
+
+    it "should indicate it's a boolean confine if it has been marked that way" do
+        confine = Puppet::Provider::Confine.new(:foo, :bar)
+        confine.for_binary = true
+        confine.should be_for_binary
+    end
+
+    it "should have a method for returning a binary's path" do
+        Puppet::Provider::Confine.new(:foo, :bar).private_methods.should be_include("binary")
+    end
+
     describe "when testing values" do
         before { @confine = Puppet::Provider::Confine.new("eh", "foo") }
 
@@ -92,6 +110,31 @@ describe Puppet::Provider::Confine do
                 Puppet.expects(:debug).with { |l| l.include?("true") }
                 @confine.valid?
             end
+
+            describe "and the confine is for binaries" do
+                before { @confine.stubs(:for_binary).returns true }
+                it "should use its 'binary' method to look up the full path of the file" do
+                    @confine.expects(:binary).returns nil
+                    @confine.exists?("/my/file")
+                end
+
+                it "should return false if no binary can be found" do
+                    @confine.expects(:binary).with("/my/file").returns nil
+                    @confine.exists?("/my/file").should be_false
+                end
+
+                it "should return true if the binary can be found and the file exists" do
+                    @confine.expects(:binary).with("/my/file").returns "/my/file"
+                    FileTest.expects(:exist?).with("/my/file").returns true
+                    @confine.exists?("/my/file").should be_true
+                end
+
+                it "should return false if the binary can be found but the file does not exist" do
+                    @confine.expects(:binary).with("/my/file").returns "/my/file"
+                    FileTest.expects(:exist?).with("/my/file").returns true
+                    @confine.exists?("/my/file").should be_true
+                end
+            end
         end
 
         describe "and the test is not 'true', 'false', or 'exists'" do
diff --git a/spec/unit/provider/confine_collection.rb b/spec/unit/provider/confine_collection.rb
new file mode 100755
index 000000000..3430d604f
--- /dev/null
+++ b/spec/unit/provider/confine_collection.rb
@@ -0,0 +1,113 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+require 'puppet/provider/confine_collection'
+
+describe Puppet::Provider::ConfineCollection do
+    it "should be able to add confines" do
+        Puppet::Provider::ConfineCollection.new.should respond_to(:confine)
+    end
+
+    it "should create a Confine instance for every confine call" do
+        Puppet::Provider::Confine.expects(:new).with(:foo, :bar).returns "eh"
+        Puppet::Provider::Confine.expects(:new).with(:baz, :bee).returns "eh"
+        Puppet::Provider::ConfineCollection.new.confine :foo => :bar, :baz => :bee
+    end
+
+    it "should mark each confine as a binary confine if :for_binary => true is included" do
+        confine = mock 'confine'
+        confine.expects(:for_binary=).with true
+        Puppet::Provider::Confine.expects(:new).with(:foo, :bar).returns confine
+        Puppet::Provider::ConfineCollection.new.confine :foo => :bar, :for_binary => true
+    end
+
+    it "should be valid if no confines are present" do
+        Puppet::Provider::ConfineCollection.new.should be_valid
+    end
+
+    it "should be valid if all confines are valid" do
+        c1 = mock 'c1', :valid? => true
+        c2 = mock 'c2', :valid? => true
+
+        Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
+
+        confiner = Puppet::Provider::ConfineCollection.new
+        confiner.confine :foo => :bar, :baz => :bee
+
+        confiner.should be_valid
+    end
+
+    it "should not be valid if any confines are valid" do
+        c1 = mock 'c1', :valid? => true
+        c2 = mock 'c2', :valid? => false
+
+        Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
+
+        confiner = Puppet::Provider::ConfineCollection.new
+        confiner.confine :foo => :bar, :baz => :bee
+
+        confiner.should_not be_valid
+    end
+
+    describe "when providing a complete result" do
+        before do
+            @confiner = Puppet::Provider::ConfineCollection.new
+        end
+
+        it "should return a hash" do
+            @confiner.result.should be_instance_of(Hash)
+        end
+
+        it "should return an empty hash if the confiner is valid" do
+            @confiner.result.should == {}
+        end
+
+        it "should contain the number of incorrectly false values" do
+            c1 = stub 'c1', :result => [true, false, true], :test => :true
+            c2 = stub 'c2', :result => [false, true, false], :test => :true
+
+            Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
+
+            confiner = Puppet::Provider::ConfineCollection.new
+            confiner.confine :foo => :bar, :baz => :bee
+
+            confiner.result[:true].should == 3
+        end
+
+        it "should contain the number of incorrectly true values" do
+            c1 = stub 'c1', :result => [true, false, true], :test => :false
+            c2 = stub 'c2', :result => [false, true, false], :test => :false
+
+            Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
+
+            confiner = Puppet::Provider::ConfineCollection.new
+            confiner.confine :foo => :bar, :baz => :bee
+
+            confiner.result[:false].should == 3
+        end
+
+        it "should contain the missing files" do
+            FileTest.stubs(:exist?).returns true
+            FileTest.expects(:exist?).with("/two").returns false
+            FileTest.expects(:exist?).with("/four").returns false
+
+            confiner = Puppet::Provider::ConfineCollection.new
+            confiner.confine :exists => %w{/one /two}
+            confiner.confine :exists => %w{/three /four}
+
+            confiner.result[:exists].should == %w{/two /four}
+        end
+
+        it "should contain a hash of facts and the allowed values" do
+            Facter.expects(:value).with(:foo).returns "yay"
+            Facter.expects(:value).with(:bar).returns "boo"
+            confiner = Puppet::Provider::ConfineCollection.new
+            confiner.confine :foo => "yes", :bar => "boo"
+
+            result = confiner.result
+            result[:facter][:foo].should == %w{yes}
+            result[:facter][:bar].should be_nil
+        end
+    end
+end
diff --git a/spec/unit/provider/confiner.rb b/spec/unit/provider/confiner.rb
index b80255c95..38fffc102 100755
--- a/spec/unit/provider/confiner.rb
+++ b/spec/unit/provider/confiner.rb
@@ -5,102 +5,58 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 require 'puppet/provider/confiner'
 
 describe Puppet::Provider::Confiner do
-    it "should be able to add confines" do
-        Puppet::Provider::Confiner.new.should respond_to(:confine)
+    before do
+        @object = Object.new
+        @object.extend(Puppet::Provider::Confiner)
     end
 
-    it "should create a Confine instance for every confine call" do
-        Puppet::Provider::Confine.expects(:new).with(:foo, :bar).returns "eh"
-        Puppet::Provider::Confine.expects(:new).with(:baz, :bee).returns "eh"
-        Puppet::Provider::Confiner.new.confine :foo => :bar, :baz => :bee
+    it "should have a method for defining confines" do
+        @object.should respond_to(:confine)
     end
 
-    it "should be valid if no confines are present" do
-        Puppet::Provider::Confiner.new.should be_valid
+    it "should have a method for returning its confine collection" do
+        @object.should respond_to(:confine_collection)
     end
 
-    it "should be valid if all confines are valid" do
-        c1 = mock 'c1', :valid? => true
-        c2 = mock 'c2', :valid? => true
-
-        Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
-
-        confiner = Puppet::Provider::Confiner.new
-        confiner.confine :foo => :bar, :baz => :bee
-
-        confiner.should be_valid
+    it "should have a method for testing suitability" do
+        @object.should respond_to(:suitable?)
     end
 
-    it "should not be valid if any confines are valid" do
-        c1 = mock 'c1', :valid? => true
-        c2 = mock 'c2', :valid? => false
-
-        Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
+    it "should delegate its confine method to its confine collection" do
+        coll = mock 'collection'
+        @object.stubs(:confine_collection).returns coll
+        coll.expects(:confine).with(:foo => :bar, :bee => :baz)
+        @object.confine(:foo => :bar, :bee => :baz)
+    end
 
-        confiner = Puppet::Provider::Confiner.new
-        confiner.confine :foo => :bar, :baz => :bee
+    it "should create a new confine collection if one does not exist" do
+        Puppet::Provider::ConfineCollection.expects(:new).returns "mycoll"
+        @object.confine_collection.should == "mycoll"
+    end
 
-        confiner.should_not be_valid
+    it "should reuse the confine collection" do
+        @object.confine_collection.should equal(@object.confine_collection)
     end
 
-    describe "when providing a complete result" do
+    describe "when testing suitability" do
         before do
-            @confiner = Puppet::Provider::Confiner.new
+            @coll = mock 'collection'
+            @object.stubs(:confine_collection).returns @coll
         end
 
-        it "should return a hash" do
-            @confiner.result.should be_instance_of(Hash)
+        it "should return true if the confine collection is valid" do
+            @coll.expects(:valid?).returns true
+            @object.should be_suitable
         end
 
-        it "should return an empty hash if the confiner is valid" do
-            @confiner.result.should == {}
-        end
-
-        it "should contain the number of incorrectly false values" do
-            c1 = stub 'c1', :result => [true, false, true], :test => :true
-            c2 = stub 'c2', :result => [false, true, false], :test => :true
-
-            Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
-
-            confiner = Puppet::Provider::Confiner.new
-            confiner.confine :foo => :bar, :baz => :bee
-
-            confiner.result[:true].should == 3
+        it "should return false if the confine collection is invalid" do
+            @coll.expects(:valid?).returns false
+            @object.should_not be_suitable
         end
 
-        it "should contain the number of incorrectly true values" do
-            c1 = stub 'c1', :result => [true, false, true], :test => :false
-            c2 = stub 'c2', :result => [false, true, false], :test => :false
-
-            Puppet::Provider::Confine.expects(:new).times(2).returns(c1).then.returns(c2)
-
-            confiner = Puppet::Provider::Confiner.new
-            confiner.confine :foo => :bar, :baz => :bee
-
-            confiner.result[:false].should == 3
-        end
-
-        it "should contain the missing files" do
-            FileTest.stubs(:exist?).returns true
-            FileTest.expects(:exist?).with("/two").returns false
-            FileTest.expects(:exist?).with("/four").returns false
-
-            confiner = Puppet::Provider::Confiner.new
-            confiner.confine :exists => %w{/one /two}
-            confiner.confine :exists => %w{/three /four}
-
-            confiner.result[:exists].should == %w{/two /four}
-        end
-
-        it "should contain a hash of facts and the allowed values" do
-            Facter.expects(:value).with(:foo).returns "yay"
-            Facter.expects(:value).with(:bar).returns "boo"
-            confiner = Puppet::Provider::Confiner.new
-            confiner.confine :foo => "yes", :bar => "boo"
-
-            result = confiner.result
-            result[:facter][:foo].should == %w{yes}
-            result[:facter][:bar].should be_nil
+        it "should return the result of the confine collection if a long result is asked for" do
+            @coll.expects(:result).returns "myresult"
+            @object.suitable?(false).should == "myresult"
         end
     end
 end
diff --git a/test/ral/providers/provider.rb b/test/ral/providers/provider.rb
index f284e9125..70f606a37 100755
--- a/test/ral/providers/provider.rb
+++ b/test/ral/providers/provider.rb
@@ -97,7 +97,7 @@ class TestProvider < Test::Unit::TestCase
 
         provider.commands :testing => "/no/such/path"
 
-        provider.expects(:binary).returns "/no/such/path"
+        provider.stubs(:binary).returns "/no/such/path"
 
         provider.command(:testing)
         assert_equal("/no/such/path", provider.command(:testing), "Did not return correct binary path")