From 0fcc1150c56483ae6844bd4d194fae3e16cbfc89 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Wed, 29 Sep 2010 23:38:07 -0700 Subject: [PATCH] Revert "Instead of using Kernel#system, use custom piped solution" This reverts commit 171f4184c0a698c4a6b05767accb6002ffcd18a0. --- CHANGELOG.md | 2 -- lib/vagrant/hosts/bsd.rb | 17 ++++++---------- lib/vagrant/hosts/linux.rb | 17 ++++++---------- lib/vagrant/util.rb | 1 - lib/vagrant/util/sh.rb | 33 -------------------------------- test/vagrant/hosts/bsd_test.rb | 18 +++++++---------- test/vagrant/hosts/linux_test.rb | 18 +++++++---------- test/vagrant/util/sh_test.rb | 22 --------------------- 8 files changed, 26 insertions(+), 102 deletions(-) delete mode 100644 lib/vagrant/util/sh.rb delete mode 100644 test/vagrant/util/sh_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index f10baf631..dd95d7e06 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/vagrant/hosts/bsd.rb b/lib/vagrant/hosts/bsd.rb index 1fee8f48b..9bf3f2310 100644 --- a/lib/vagrant/hosts/bsd.rb +++ b/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") - - # Sometimes the status is nil for some reason. In that case, force a retry - raise TypeError.new("Bad status code") if !status - status.success? + system("which nfsd > /dev/null 2>&1") 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 diff --git a/lib/vagrant/hosts/linux.rb b/lib/vagrant/hosts/linux.rb index 12523f298..68466491e 100644 --- a/lib/vagrant/hosts/linux.rb +++ b/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") - - # Sometimes the status is nil for some reason. In that case, force a retry - raise TypeError.new("Bad status code") if !status - status.success? + system("cat /proc/filesystems | grep nfsd > /dev/null 2>&1") 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 diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index 847bb16e5..bcfb27841 100644 --- a/lib/vagrant/util.rb +++ b/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 diff --git a/lib/vagrant/util/sh.rb b/lib/vagrant/util/sh.rb deleted file mode 100644 index 23181e2b4..000000000 --- a/lib/vagrant/util/sh.rb +++ /dev/null @@ -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 diff --git a/test/vagrant/hosts/bsd_test.rb b/test/vagrant/hosts/bsd_test.rb index 1539e8ba4..8e73d15c8 100644 --- a/test/vagrant/hosts/bsd_test.rb +++ b/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(:sh).returns(result) + @instance.expects(:system).returns(true) assert @instance.nfs? end should "not support NFS if nfsd is not found" do - result = @instance.sh("sdalkfjasdlkfj") - @instance.expects(:sh).returns(result) + @instance.expects(:system).returns(false) 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(:sh).once.in_sequence(seq).returns(result) + @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo")) + @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(:sh).with("sudo nfsd restart") + @instance.expects(:system).times(output.split("\n").length) + @instance.expects(:system).with("sudo nfsd restart") @instance.nfs_export(@ip, @folders) end end diff --git a/test/vagrant/hosts/linux_test.rb b/test/vagrant/hosts/linux_test.rb index c04a1ed7a..5c1ff9a85 100644 --- a/test/vagrant/hosts/linux_test.rb +++ b/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(:sh).returns(result) + @instance.expects(:system).returns(true) assert @instance.nfs? end should "not support NFS if nfsd is not found" do - result = @instance.sh("adosifjssdlkfj") - @instance.expects(:sh).returns(result) + @instance.expects(:system).returns(false) 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(:sh).once.in_sequence(seq).returns(result) + @instance.expects(:system).in_sequence(seq).raises(TypeError.new("foo")) + @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(:sh).with("sudo /etc/init.d/nfs-kernel-server restart") + @instance.expects(:system).times(output.split("\n").length) + @instance.expects(:system).with("sudo /etc/init.d/nfs-kernel-server restart") @instance.nfs_export(@ip, @folders) end end diff --git a/test/vagrant/util/sh_test.rb b/test/vagrant/util/sh_test.rb deleted file mode 100644 index 82e9c9f98..000000000 --- a/test/vagrant/util/sh_test.rb +++ /dev/null @@ -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