Fix for #4299 -- Don't require which

We already had an internal implementation of which hiding under an assumed
name (Puppet::Util.binary); this commit calls it out of hiding and uses it
consisantly.

13 files changed, 25 insertions, 66 deletions

diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index b8437fe29..93b01c779 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -637,7 +637,7 @@ module Puppet
     setdefaults(
     :tagmail,
     :tagmap => ["$confdir/tagmail.conf", "The mapping between reporting tags and email addresses."],
-    :sendmail => [%x{which sendmail 2>/dev/null}.chomp, "Where to find the sendmail binary with which to send email."],
+    :sendmail => [which('sendmail') || '', "Where to find the sendmail binary with which to send email."],
 
     :reportfrom => ["report@" + [Facter["hostname"].value, Facter["domain"].value].join("."), "The 'from' email address for the reports."],
     :smtpserver => ["none", "The server through which to send email reports."]
diff --git a/lib/puppet/provider.rb b/lib/puppet/provider.rb
index af792b623..4456feb4e 100644
--- a/lib/puppet/provider.rb
+++ b/lib/puppet/provider.rb
@@ -12,7 +12,7 @@ class Puppet::Provider
   Puppet::Util.logmethods(self, true)
 
   class << self
-    # Include the util module so we have access to things like 'binary'
+    # Include the util module so we have access to things like 'which'
     include Puppet::Util, Puppet::Util::Docs
     include Puppet::Util::Logging
     attr_accessor :name
@@ -43,7 +43,7 @@ class Puppet::Provider
       raise Puppet::DevError, "No command #{name} defined for provider #{self.name}"
     end
 
-    binary(command)
+    which(command)
   end
 
   # Define commands that are not optional.
diff --git a/lib/puppet/provider/confine/exists.rb b/lib/puppet/provider/confine/exists.rb
index 085118b2a..09f94dfd9 100644
--- a/lib/puppet/provider/confine/exists.rb
+++ b/lib/puppet/provider/confine/exists.rb
@@ -6,10 +6,7 @@ class Puppet::Provider::Confine::Exists < Puppet::Provider::Confine
   end
 
   def pass?(value)
-    if for_binary?
-      return false unless value = binary(value)
-    end
-    value and FileTest.exist?(value)
+    value && (for_binary? ? which(value) : FileTest.exist?(value))
   end
 
   def message(value)
diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index dd57ec231..606888c75 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -553,13 +553,7 @@ module Puppet
       if self[:path]
         if Puppet.features.posix? and !File.exists?(exe)
           withenv :PATH => self[:path].join(File::PATH_SEPARATOR) do
-            path = %x{which #{exe}}.chomp
-            if path == ""
-              raise ArgumentError,
-                "Could not find command '#{exe}'"
-            else
-              exe = path
-            end
+            exe = which(exe) || raise(ArgumentError,"Could not find command '#{exe}'")
           end
         elsif Puppet.features.microsoft_windows? and !File.exists?(exe)
           self[:path].each do |path|
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 1a5acaf22..850d147e2 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -198,7 +198,7 @@ module Util
     end
   end
 
-  def binary(bin)
+  def which(bin)
     if bin =~ /^\//
       return bin if FileTest.file? bin and FileTest.executable? bin
     else
@@ -209,7 +209,7 @@ module Util
     end
     nil
   end
-  module_function :binary
+  module_function :which
 
   # Execute the provided command in a pipe, yielding the pipe object.
   def execpipe(command, failonfail = true)
@@ -378,15 +378,10 @@ module Util
 
   def memory
     unless defined?(@pmap)
-      pmap = %x{which pmap 2>/dev/null}.chomp
-      if $CHILD_STATUS != 0 or pmap =~ /^no/
-        @pmap = nil
-      else
-        @pmap = pmap
-      end
+      @pmap = which('pmap')
     end
     if @pmap
-      return %x{pmap #{Process.pid}| grep total}.chomp.sub(/^\s*total\s+/, '').sub(/K$/, '').to_i
+      %x{#{@pmap} #{Process.pid}| grep total}.chomp.sub(/^\s*total\s+/, '').sub(/K$/, '').to_i
     else
       0
     end
diff --git a/lib/puppet/util/command_line.rb b/lib/puppet/util/command_line.rb
index 2a97ee069..3562a3dc0 100644
--- a/lib/puppet/util/command_line.rb
+++ b/lib/puppet/util/command_line.rb
@@ -62,7 +62,7 @@ module Puppet
           external_command = "puppet-#{subcommand_name}"
 
           require 'puppet/util'
-          path_to_subcommand = Puppet::Util.binary( external_command )
+          path_to_subcommand = Puppet::Util.which( external_command )
           return false unless path_to_subcommand
 
           system( path_to_subcommand, *args )
diff --git a/lib/puppet/util/reference.rb b/lib/puppet/util/reference.rb
index ab201cde4..95efeb1c1 100644
--- a/lib/puppet/util/reference.rb
+++ b/lib/puppet/util/reference.rb
@@ -39,14 +39,7 @@ class Puppet::Util::Reference
     Puppet::Util.secure_open("/tmp/puppetdoc.txt", "w") do |f|
       f.puts text
     end
-    rst2latex = %x{which rst2latex}
-    if $CHILD_STATUS != 0 or rst2latex =~ /no /
-      rst2latex = %x{which rst2latex.py}
-    end
-    if $CHILD_STATUS != 0 or rst2latex =~ /no /
-      raise "Could not find rst2latex"
-    end
-    rst2latex.chomp!
+    rst2latex = which('rst2latex') || which('rst2latex.py') || raise("Could not find rst2latex")
     cmd = %{#{rst2latex} /tmp/puppetdoc.txt > /tmp/puppetdoc.tex}
     Puppet::Util.secure_open("/tmp/puppetdoc.tex","w") do |f|
       # If we get here without an error, /tmp/puppetdoc.tex isn't a tricky cracker's symlink
diff --git a/spec/integration/ssl/certificate_authority_spec.rb b/spec/integration/ssl/certificate_authority_spec.rb
index fca17b405..67ff6f215 100755
--- a/spec/integration/ssl/certificate_authority_spec.rb
+++ b/spec/integration/ssl/certificate_authority_spec.rb
@@ -121,9 +121,7 @@ describe Puppet::SSL::CertificateAuthority do
     it "should save valid certificates" do
       @ca.sign("luke.madstop.com")
 
-      ssl = %x{which openssl}
-
-      unless ssl
+      unless ssl = Puppet::Util::which('openssl')
         pending "No ssl available"
       else
         ca_cert = Puppet[:cacert]
diff --git a/spec/unit/provider/confine/exists_spec.rb b/spec/unit/provider/confine/exists_spec.rb
index c3958e317..f039208b8 100755
--- a/spec/unit/provider/confine/exists_spec.rb
+++ b/spec/unit/provider/confine/exists_spec.rb
@@ -39,25 +39,18 @@ describe Puppet::Provider::Confine::Exists do
 
   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
+    it "should use its 'which' method to look up the full path of the file" do
+      @confine.expects(:which).returns nil
       @confine.pass?("/my/file")
     end
 
-    it "should return false if no binary can be found" do
-      @confine.expects(:binary).with("/my/file").returns nil
+    it "should return false if no executable can be found" do
+      @confine.expects(:which).with("/my/file").returns nil
       @confine.pass?("/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.pass?("/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
+    it "should return true if the executable can be found" do
+      @confine.expects(:which).with("/my/file").returns "/my/file"
       @confine.pass?("/my/file").should be_true
     end
   end
diff --git a/spec/unit/util/command_line_spec.rb b/spec/unit/util/command_line_spec.rb
index a83ad968d..7ba965249 100644
--- a/spec/unit/util/command_line_spec.rb
+++ b/spec/unit/util/command_line_spec.rb
@@ -86,7 +86,7 @@ describe Puppet::Util::CommandLine do
   describe "when the subcommand is not implemented" do
     it "should find and invoke an executable with a hyphenated name" do
       commandline = Puppet::Util::CommandLine.new("puppet", ['whatever', 'argument'], @tty)
-      Puppet::Util.expects(:binary).with('puppet-whatever').returns('/dev/null/puppet-whatever')
+      Puppet::Util.expects(:which).with('puppet-whatever').returns('/dev/null/puppet-whatever')
       commandline.expects(:system).with('/dev/null/puppet-whatever', 'argument')
 
       commandline.execute
@@ -95,7 +95,7 @@ describe Puppet::Util::CommandLine do
     describe "and an external implementation cannot be found" do
       it "should abort and show the usage message" do
         commandline = Puppet::Util::CommandLine.new("puppet", ['whatever', 'argument'], @tty)
-        Puppet::Util.expects(:binary).with('puppet-whatever').returns(nil)
+        Puppet::Util.expects(:which).with('puppet-whatever').returns(nil)
         commandline.expects(:system).never
 
         commandline.expects(:usage_message).returns("the usage message")
diff --git a/test/other/provider.rb b/test/other/provider.rb
index 052d7a0d0..a539ee5a7 100755
--- a/test/other/provider.rb
+++ b/test/other/provider.rb
@@ -70,24 +70,13 @@ class TestImpl < Test::Unit::TestCase
       child = @type.provide("child", :parent => parent.name) {}
     }
 
-    assert_nothing_raised {
-      child.commands :which => "which"
-    }
-
-    assert(child.command(:which), "Did not find 'which' command")
-
-    assert(child.command(:which) =~ /^\//,
-        "Command did not become fully qualified")
-    assert(FileTest.exists?(child.command(:which)),
-                "Did not find actual 'which' binary")
-
     assert_raise(Puppet::DevError) do
       child.command(:nosuchcommand)
     end
 
     # Now create a parent command
     assert_nothing_raised {
-      parent.commands :sh => Puppet::Util.binary('sh')
+      parent.commands :sh => Puppet::Util.which('sh')
     }
 
     assert(parent.command(:sh), "Did not find 'sh' command")
diff --git a/test/ral/manager/type.rb b/test/ral/manager/type.rb
index 7df643005..145877722 100755
--- a/test/ral/manager/type.rb
+++ b/test/ral/manager/type.rb
@@ -276,7 +276,7 @@ class TestType < Test::Unit::TestCase
   def test_isomorphic_names
     catalog = mk_catalog
     # First do execs, since they're not isomorphic.
-    echo = Puppet::Util.binary "echo"
+    echo = Puppet::Util.which "echo"
     exec1 = exec2 = nil
     assert_nothing_raised do
 
diff --git a/test/ral/providers/provider.rb b/test/ral/providers/provider.rb
index cb0b2a19e..f46e03f82 100755
--- a/test/ral/providers/provider.rb
+++ b/test/ral/providers/provider.rb
@@ -9,7 +9,7 @@ class TestProvider < Test::Unit::TestCase
   include PuppetTest
 
   def echo
-    echo = Puppet::Util.binary("echo")
+    echo = Puppet::Util.which("echo")
 
     raise "Could not find 'echo' binary; cannot complete test" unless echo
 
@@ -95,7 +95,7 @@ class TestProvider < Test::Unit::TestCase
 
     provider.commands :testing => "/no/such/path"
 
-    provider.stubs(:binary).returns "/no/such/path"
+    provider.stubs(:which).returns "/no/such/path"
 
     provider.command(:testing)
     assert_equal("/no/such/path", provider.command(:testing), "Did not return correct binary path")
@@ -187,7 +187,7 @@ class TestProvider < Test::Unit::TestCase
 
     dir = tstdir
     file = File.join(dir, "mycmd")
-    sh = Puppet::Util.binary("sh")
+    sh = Puppet::Util.which("sh")
     File.open(file, "w") { |f|
       f.puts %{#!#{sh}
       echo A Failure >&2