Use IO.popen to start a process

RuboCop rightfully complains about using open() to spawn a process. This
uses the way simpler IO.popen() method.

Author: ekohl

lib/puppet/util/execution.rb

@@ -57,31 +57,19 @@ def initialize(value, exitstatus)
   # @see Kernel#open for `mode` values
   # @api public
   def self.execpipe(command, failonfail = true)
-    # Paste together an array with spaces.  We used to paste directly
-    # together, no spaces, which made for odd invocations; the user had to
-    # include whitespace between arguments.
-    #
-    # Having two spaces is really not a big drama, since this passes to the
-    # shell anyhow, while no spaces makes for a small developer cost every
-    # time this is invoked. --daniel 2012-02-13
-    command_str = command.respond_to?(:join) ? command.join(' ') : command
-
     if respond_to? :debug
-      debug "Executing '#{command_str}'"
+      debug "Executing #{command.inspect}"
     else
-      Puppet.debug { "Executing '#{command_str}'" }
+      Puppet.debug { "Executing #{command.inspect}" }
     end
 
-    # force the run of the command with
-    # the user/system locale to "C" (via environment variables LANG and LC_*)
-    # it enables to have non localized output for some commands and therefore
-    # a predictable output
-    english_env = ENV.to_hash.merge({ 'LANG' => 'C', 'LC_ALL' => 'C' })
-    output = Puppet::Util.withenv(english_env) do
-      # We are intentionally using 'pipe' with open to launch a process
-      open("| #{command_str} 2>&1") do |pipe| # rubocop:disable Security/Open
+    begin
+      output = IO.popen({ 'LANG' => 'C', 'LC_ALL' => 'C' }, command, err: :out) do |pipe|
         yield pipe
       end
+    rescue Errno::ENOENT => e
+      output = e.message
+      raise Puppet::ExecutionFailure, output if failonfail
     end
 
     if failonfail && exitstatus != 0

spec/unit/provider/package/pacman_spec.rb

@@ -403,21 +403,21 @@
     let(:groups) { [['foo package1'], ['foo package2'], ['bar package3'], ['bar package4'], ['baz package5']] }
 
     it 'should raise an error on non-zero pacman exit without a filter' do
-      expect(executor).to receive(:open).with('| /usr/bin/pacman --sync -gg 2>&1').and_return('error!')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['/usr/bin/pacman', '--sync', '-gg'], err: :out).and_return('error!')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(1)
       expect { described_class.get_installed_groups(installed_packages) }.to raise_error(Puppet::ExecutionFailure, 'error!')
     end
 
     it 'should return empty groups on non-zero pacman exit with a filter' do
-      expect(executor).to receive(:open).with('| /usr/bin/pacman --sync -gg git 2>&1').and_return('')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['/usr/bin/pacman', '--sync', '-gg', 'git'], err: :out).and_return('')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(1)
       expect(described_class.get_installed_groups(installed_packages, 'git')).to eq({})
     end
 
     it 'should return empty groups on empty pacman output' do
       pipe = double()
       expect(pipe).to receive(:each_line)
-      expect(executor).to receive(:open).with('| /usr/bin/pacman --sync -gg 2>&1').and_yield(pipe).and_return('')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['/usr/bin/pacman', '--sync', '-gg'], err: :out).and_yield(pipe).and_return('')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       expect(described_class.get_installed_groups(installed_packages)).to eq({})
     end
@@ -427,7 +427,7 @@
       pipe_expectation = receive(:each_line)
       groups.each { |group| pipe_expectation = pipe_expectation.and_yield(*group) }
       expect(pipe).to pipe_expectation
-      expect(executor).to receive(:open).with('| /usr/bin/pacman --sync -gg 2>&1').and_yield(pipe).and_return('')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['/usr/bin/pacman', '--sync', '-gg'], err: :out).and_yield(pipe).and_return('')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       expect(described_class.get_installed_groups(installed_packages)).to eq({'foo' => 'package1 1.0, package2 2.0'})
     end

spec/unit/util/execution_spec.rb

@@ -909,43 +909,44 @@ def expect_cwd_to_be(cwd)
 
   describe "#execpipe" do
     it "should execute a string as a string" do
-      expect(Puppet::Util::Execution).to receive(:open).with('| echo hello 2>&1').and_return('hello')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'echo hello', err: :out).and_return('hello')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       expect(Puppet::Util::Execution.execpipe('echo hello')).to eq('hello')
     end
 
     it "should print meaningful debug message for string argument" do
       Puppet[:log_level] = 'debug'
-      expect(Puppet).to receive(:send_log).with(:debug, "Executing 'echo hello'")
-      expect(Puppet::Util::Execution).to receive(:open).with('| echo hello 2>&1').and_return('hello')
+      expect(Puppet).to receive(:send_log).with(:debug, 'Executing "echo hello"')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'echo hello', err: :out).and_return('hello')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       Puppet::Util::Execution.execpipe('echo hello')
     end
 
     it "should print meaningful debug message for array argument" do
       Puppet[:log_level] = 'debug'
-      expect(Puppet).to receive(:send_log).with(:debug, "Executing 'echo hello'")
-      expect(Puppet::Util::Execution).to receive(:open).with('| echo hello 2>&1').and_return('hello')
+      expect(Puppet).to receive(:send_log).with(:debug, 'Executing ["echo", "hello"]')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['echo', 'hello'], err: :out).and_return('hello')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       Puppet::Util::Execution.execpipe(['echo','hello'])
     end
 
-    it "should execute an array by pasting together with spaces" do
-      expect(Puppet::Util::Execution).to receive(:open).with('| echo hello 2>&1').and_return('hello')
+    it "should execute an array" do
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['echo', 'hello'], err: :out).and_return('hello')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(0)
       expect(Puppet::Util::Execution.execpipe(['echo', 'hello'])).to eq('hello')
     end
 
     it "should fail if asked to fail, and the child does" do
-      allow(Puppet::Util::Execution).to receive(:open).with('| echo hello 2>&1').and_return('error message')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, ['echo', 'hello'], err: :out).and_return('error message')
       expect(Puppet::Util::Execution).to receive(:exitstatus).and_return(1)
       expect {
         Puppet::Util::Execution.execpipe('echo hello')
       }.to raise_error Puppet::ExecutionFailure, /error message/
     end
 
     it "should not fail if asked not to fail, and the child does" do
-      allow(Puppet::Util::Execution).to receive(:open).and_return('error message')
+      expect(IO).to receive(:popen).with({ 'LANG' => 'C', 'LC_ALL' => 'C' }, 'echo hello', err: :out).and_return('error message')
+      expect(Puppet::Util::Execution).not_to receive(:exitstatus)
       expect(Puppet::Util::Execution.execpipe('echo hello', false)).to eq('error message')
     end
   end