Revert "Instead of using Kernel#system, use custom piped solution"

This reverts commit 171f4184c0.

CHANGELOG.md

@@ -2,8 +2,6 @@
 
   - SSH gives proper error message if VM is not running. [GH-167]
   - Fix some issues with undefined constants in command errors.
-  - Replaced `Kernel#system` calls with custom `Vagrant::Util::Sh` method to
-    fix strange error issues.
 
 ## 0.6.1, 0.6.2, 0.6.3 (September 27, 2010)
 

lib/vagrant/hosts/bsd.rb

@@ -4,15 +4,10 @@ module Vagrant
     class BSD < Base
       include Util
       include Util::Retryable
-      include Util::Sh
 
       def nfs?
         retryable(:tries => 10, :on => TypeError) do
-          _, status = sh("which nfsd")
+          system("which nfsd > /dev/null 2>&1")
-
-          # Sometimes the status is nil for some reason. In that case, force a retry
-          raise TypeError.new("Bad status code") if !status
-          status.success?
         end
       end
 
@@ -30,22 +25,22 @@ module Vagrant
         output.split("\n").each do |line|
           # This should only ask for administrative permission once, even
           # though its executed in multiple subshells.
-          sh(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"])
+          system(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"])
         end
 
         # We run restart here instead of "update" just in case nfsd
         # is not starting
-        sh("sudo nfsd restart")
+        system("sudo nfsd restart")
       end
 
       def nfs_cleanup
         return if !File.exist?("/etc/exports")
-        _, status = sh("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}'")
+        system("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}' > /dev/null 2>&1")
 
-        if status.success?
+        if $?.to_i == 0
           # Use sed to just strip out the block of code which was inserted
           # by Vagrant
-          sh("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -i bak /etc/exports")
+          system("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -i bak /etc/exports")
         end
       end
     end

lib/vagrant/hosts/linux.rb

@@ -4,16 +4,11 @@ module Vagrant
     class Linux < Base
       include Util
       include Util::Retryable
-      include Util::Sh
 
       def nfs?
         retryable(:tries => 10, :on => TypeError) do
           # Check procfs to see if NFSd is a supported filesystem
-          _, status = sh("cat /proc/filesystems | grep nfsd")
+          system("cat /proc/filesystems | grep nfsd > /dev/null 2>&1")
-
-          # Sometimes the status is nil for some reason. In that case, force a retry
-          raise TypeError.new("Bad status code") if !status
-          status.success?
         end
       end
 
@@ -29,22 +24,22 @@ module Vagrant
         output.split("\n").each do |line|
           # This should only ask for administrative permission once, even
           # though its executed in multiple subshells.
-          sh(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"])
+          system(%Q[sudo su root -c "echo '#{line}' >> /etc/exports"])
         end
 
         # We run restart here instead of "update" just in case nfsd
         # is not starting
-        sh("sudo /etc/init.d/nfs-kernel-server restart")
+        system("sudo /etc/init.d/nfs-kernel-server restart")
       end
 
       def nfs_cleanup
         return if !File.exist?("/etc/exports")
-        _, status = sh("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}'")
+        system("cat /etc/exports | grep 'VAGRANT-BEGIN: #{env.vm.uuid}' > /dev/null 2>&1")
 
-        if status.success?
+        if $?.to_i == 0
           # Use sed to just strip out the block of code which was inserted
           # by Vagrant
-          sh("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -ibak /etc/exports")
+          system("sudo sed -e '/^# VAGRANT-BEGIN: #{env.vm.uuid}/,/^# VAGRANT-END: #{env.vm.uuid}/ d' -ibak /etc/exports")
         end
       end
     end

lib/vagrant/util.rb

@@ -7,7 +7,6 @@ module Vagrant
     autoload :Platform,                  'vagrant/util/platform'
     autoload :ResourceLogger,            'vagrant/util/resource_logger'
     autoload :Retryable,                 'vagrant/util/retryable'
-    autoload :Sh,                        'vagrant/util/sh'
     autoload :StackedProcRunner,         'vagrant/util/stacked_proc_runner'
     autoload :TemplateRenderer,          'vagrant/util/template_renderer'
   end

lib/vagrant/util/sh.rb

@@ -1,33 +0,0 @@
-module Vagrant
-  module Util
-    module Sh
-      # Runs the given shell command, collecting STDERR and STDOUT
-      # into a single string and returning it. After this method,
-      # `$?` will be set to a `Process::Status` object.
-      #
-      # @param [String] command Command to run
-      # @return [Array] An array of `[output, status]` where status is `Process::Status`
-      def sh(command, *args)
-        # Use a pipe to execute the given command
-        rd, wr = IO.pipe
-        pid = fork do
-          rd.close
-          $stdout.reopen wr
-          $stderr.reopen wr
-          exec(command, *args) rescue nil
-          exit! 1 # Should never reach this point
-        end
-
-        # Close our end of the writer pipe, read the output until
-        # its over, and wait for the process to end.
-        wr.close
-        out = ""
-        out << rd.read until rd.eof?
-        _, status = Process.wait2(-1, Process::WNOHANG)
-
-        # Finally, return the result
-        [out, status]
-      end
-    end
-  end
-end

test/vagrant/hosts/bsd_test.rb

@@ -9,30 +9,26 @@ class BSDHostTest < Test::Unit::TestCase
 
   context "supporting nfs check" do
     should "support NFS" do
-      result = @instance.sh("echo")
+      @instance.expects(:system).returns(true)
-      @instance.expects(:sh).returns(result)
       assert @instance.nfs?
     end
 
     should "not support NFS if nfsd is not found" do
-      result = @instance.sh("sdalkfjasdlkfj")
+      @instance.expects(:system).returns(false)
-      @instance.expects(:sh).returns(result)
       assert !@instance.nfs?
     end
 
     should "retry until a boolean is returned" do
-      result = @instance.sh("echo")
-
       seq = sequence("seq")
-      @instance.expects(:sh).once.in_sequence(seq).raises(TypeError.new("foo"))
+      @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo"))
-      @instance.expects(:sh).once.in_sequence(seq).returns(result)
+      @instance.expects(:system).in_sequence(seq).returns(true)
       assert @instance.nfs?
     end
   end
 
   context "nfs export" do
     setup do
-      @instance.stubs(:sh)
+      @instance.stubs(:system)
 
       @ip = "foo"
       @folders = "bar"
@@ -45,8 +41,8 @@ class BSDHostTest < Test::Unit::TestCase
                                                             :ip => @ip,
                                                             :folders => @folders).returns(output)
 
-      @instance.expects(:sh).times(output.split("\n").length)
+      @instance.expects(:system).times(output.split("\n").length)
-      @instance.expects(:sh).with("sudo nfsd restart")
+      @instance.expects(:system).with("sudo nfsd restart")
       @instance.nfs_export(@ip, @folders)
     end
   end

test/vagrant/hosts/linux_test.rb

@@ -10,30 +10,26 @@ class LinuxHostTest < Test::Unit::TestCase
 
   context "supporting nfs check" do
     should "support NFS" do
-      result = @instance.sh("echo")
+      @instance.expects(:system).returns(true)
-      @instance.expects(:sh).returns(result)
       assert @instance.nfs?
     end
 
     should "not support NFS if nfsd is not found" do
-      result = @instance.sh("adosifjssdlkfj")
+      @instance.expects(:system).returns(false)
-      @instance.expects(:sh).returns(result)
       assert !@instance.nfs?
     end
 
     should "retry until a boolean is returned" do
-      result = @instance.sh("echo")
-
       seq = sequence("seq")
-      @instance.expects(:sh).once.in_sequence(seq).raises(TypeError.new("foo"))
+      @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo"))
-      @instance.expects(:sh).once.in_sequence(seq).returns(result)
+      @instance.expects(:system).in_sequence(seq).returns(true)
       assert @instance.nfs?
     end
   end
 
   context "nfs export" do
     setup do
-      @instance.stubs(:sh)
+      @instance.stubs(:system)
 
       @ip = "foo"
       @folders = "bar"
@@ -46,8 +42,8 @@ class LinuxHostTest < Test::Unit::TestCase
                                                             :ip => @ip,
                                                             :folders => @folders).returns(output)
 
-      @instance.expects(:sh).times(output.split("\n").length)
+      @instance.expects(:system).times(output.split("\n").length)
-      @instance.expects(:sh).with("sudo /etc/init.d/nfs-kernel-server restart")
+      @instance.expects(:system).with("sudo /etc/init.d/nfs-kernel-server restart")
       @instance.nfs_export(@ip, @folders)
     end
   end

test/vagrant/util/sh_test.rb

@@ -1,22 +0,0 @@
-require "test_helper"
-
-class ShUtilTest < Test::Unit::TestCase
-  setup do
-    @klass = Class.new do
-      extend Vagrant::Util::Sh
-    end
-  end
-
-  should "execute and return the output" do
-    out, _ = @klass.sh("echo 'hello'")
-    assert_equal "hello\n", out
-  end
-
-  should "populate the exit status variable" do
-    _, status = @klass.sh("echo")
-    assert status.success?
-
-    _, status = @klass.sh("sdklfjslkfj")
-    assert !status.success?
-  end
-end