Merge pull request #135 from chef/lcg/cleanup1

remove largely useless Utils class

6 files changed, 140 insertions, 133 deletions

diff --git a/.travis.yml b/.travis.yml
index f77d573..28638b9 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -10,5 +10,6 @@ branches:
     - master
 before_install:
   - gem update bundler
+  - gem update --system
 
 script: bundle exec rake
diff --git a/Rakefile b/Rakefile
index 03c1c26..06a7620 100644
--- a/Rakefile
+++ b/Rakefile
@@ -13,4 +13,4 @@ RSpec::Core::RakeTask.new(:spec) do |t|
   t.pattern = FileList["spec/**/*_spec.rb"]
 end
 
-task default: [:style, :spec]
+task default: [:spec, :style]
diff --git a/appveyor.yml b/appveyor.yml
index e49c59e..2276bde 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -22,6 +22,7 @@ install:
   - echo %PATH%
   - ruby --version
   - gem --version
+  - gem update --system
   - gem install bundler --quiet --no-ri --no-rdoc
   - bundler --version
 
diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb
index 0c66c2e..094e9b3 100644
--- a/lib/mixlib/shellout/windows.rb
+++ b/lib/mixlib/shellout/windows.rb
@@ -78,7 +78,7 @@ module Mixlib
           # Start the process
           #
           process = Process.create(create_process_args)
-          logger.debug(Utils.format_process(process, app_name, command_line, timeout)) if logger
+          logger.debug(format_process(process, app_name, command_line, timeout)) if logger
           begin
             # Start pushing data into input
             stdin_write << input if input
@@ -109,7 +109,7 @@ module Mixlib
                   begin
                     require "wmi-lite/wmi"
                     wmi = WmiLite::Wmi.new
-                    Utils.kill_process_tree(process.process_id, wmi, logger)
+                    kill_process_tree(process.process_id, wmi, logger)
                     Process.kill(:KILL, process.process_id)
                   rescue Errno::EIO, SystemCallError
                     logger.warn("Failed to kill timed out process #{process.process_id}") if logger
@@ -118,7 +118,7 @@ module Mixlib
                   raise Mixlib::ShellOut::CommandTimeout, [
                     "command timed out:",
                     format_for_exception,
-                    Utils.format_process(process, app_name, command_line, timeout),
+                    format_process(process, app_name, command_line, timeout),
                   ].join("\n")
                 end
 
@@ -146,8 +146,6 @@ module Mixlib
         end
       end
 
-      private
-
       class ThingThatLooksSortOfLikeAProcessStatus
         attr_accessor :exitstatus
         def success?
@@ -155,6 +153,8 @@ module Mixlib
         end
       end
 
+      private
+
       def consume_output(open_streams, stdout_read, stderr_read)
         return false if open_streams.length == 0
         ready = IO.select(open_streams, nil, nil, READ_WAIT_TIME)
@@ -188,7 +188,7 @@ module Mixlib
       IS_BATCH_FILE = /\.bat"?$|\.cmd"?$/i
 
       def command_to_run(command)
-        return _run_under_cmd(command) if Utils.should_run_under_cmd?(command)
+        return _run_under_cmd(command) if should_run_under_cmd?(command)
 
         candidate = candidate_executable_for_command(command)
 
@@ -196,8 +196,8 @@ module Mixlib
         return [ nil, command ] if candidate.length == 0
 
         # Check if the exe exists directly.  Otherwise, search PATH.
-        exe = Utils.find_executable(candidate)
-        exe = Utils.which(unquoted_executable_path(command)) if exe.nil? && exe !~ /[\\\/]/
+        exe = find_executable(candidate)
+        exe = which(unquoted_executable_path(command)) if exe.nil? && exe !~ /[\\\/]/
 
         # Batch files MUST use cmd; and if we couldn't find the command we're looking for,
         # we assume it must be a cmd builtin.
@@ -240,7 +240,7 @@ module Mixlib
         end
 
         environment.each_pair do |k, v|
-          if v == nil
+          if v.nil?
             result.delete(k)
           else
             result[k] = v
@@ -249,134 +249,140 @@ module Mixlib
         result
       end
 
-      module Utils
-        # api: semi-private
-        # If there are special characters parsable by cmd.exe (such as file redirection), then
-        # this method should return true.
-        #
-        # This parser is based on
-        # https://github.com/ruby/ruby/blob/9073db5cb1d3173aff62be5b48d00f0fb2890991/win32/win32.c#L1437
-        def self.should_run_under_cmd?(command)
-          return true if command =~ /^@/
-
-          quote = nil
-          env = false
-          env_first_char = false
-
-          command.dup.each_char do |c|
-            case c
-            when "'", '"'
-              if !quote
-                quote = c
-              elsif quote == c
-                quote = nil
-              end
-              next
-            when ">", "<", "|", "&", "\n"
-              return true unless quote
-            when "%"
-              return true if env
-              env = env_first_char = true
-              next
-            else
-              next unless env
-              if env_first_char
-                env_first_char = false
-                (env = false) && next if c !~ /[A-Za-z_]/
-              end
-              env = false if c !~ /[A-Za-z1-9_]/
+      # api: semi-private
+      # If there are special characters parsable by cmd.exe (such as file redirection), then
+      # this method should return true.
+      #
+      # This parser is based on
+      # https://github.com/ruby/ruby/blob/9073db5cb1d3173aff62be5b48d00f0fb2890991/win32/win32.c#L1437
+      def should_run_under_cmd?(command)
+        return true if command =~ /^@/
+
+        quote = nil
+        env = false
+        env_first_char = false
+
+        command.dup.each_char do |c|
+          case c
+          when "'", '"'
+            if !quote
+              quote = c
+            elsif quote == c
+              quote = nil
             end
+            next
+          when ">", "<", "|", "&", "\n"
+            return true unless quote
+          when "%"
+            return true if env
+            env = env_first_char = true
+            next
+          else
+            next unless env
+            if env_first_char
+              env_first_char = false
+              (env = false) && next if c !~ /[A-Za-z_]/
+            end
+            env = false if c !~ /[A-Za-z1-9_]/
           end
-          return false
         end
+        return false
+      end
 
-        def self.pathext
-          @pathext ||= ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""]
-        end
+      def pathext
+        @pathext ||= ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""]
+      end
 
-        # which() mimicks the Unix which command
-        # FIXME: it is not working
-        def self.which(cmd)
-          ENV["PATH"].split(File::PATH_SEPARATOR).each do |path|
-            exe = find_executable("#{path}/#{cmd}")
-            return exe if exe
-          end
-          return nil
+      # which() mimicks the Unix which command
+      # FIXME: it is not working
+      def which(cmd)
+        ENV["PATH"].split(File::PATH_SEPARATOR).each do |path|
+          exe = find_executable("#{path}/#{cmd}")
+          return exe if exe
         end
+        return nil
+      end
 
-        # Windows has a different notion of what "executable" means
-        # The OS will search through valid the extensions and look
-        # for a binary there.
-        def self.find_executable(path)
-          return path if executable? path
-
-          pathext.each do |ext|
-            exe = "#{path}#{ext}"
-            return exe if executable? exe
-          end
-          return nil
-        end
+      # Windows has a different notion of what "executable" means
+      # The OS will search through valid the extensions and look
+      # for a binary there.
+      def find_executable(path)
+        return path if executable? path
 
-        def self.executable?(path)
-          File.executable?(path) && !File.directory?(path)
+        pathext.each do |ext|
+          exe = "#{path}#{ext}"
+          return exe if executable? exe
         end
+        return nil
+      end
 
-        def self.system_required_processes
-          [
-            "System Idle Process",
-            "System",
-            "spoolsv.exe",
-            "lsass.exe",
-            "csrss.exe",
-            "smss.exe",
-            "svchost.exe",
-          ]
-        end
+      def executable?(path)
+        File.executable?(path) && !File.directory?(path)
+      end
 
-        def self.unsafe_process?(name, logger)
-          return false unless system_required_processes.include? name
-          logger.debug(
-            "A request to kill a critical system process - #{name} - was received and skipped."
-          )
-          true
-        end
+      def system_required_processes
+        [
+          "System Idle Process",
+          "System",
+          "spoolsv.exe",
+          "lsass.exe",
+          "csrss.exe",
+          "smss.exe",
+          "svchost.exe",
+        ]
+      end
 
-        # recursively kills all child processes of given pid
-        # calls itself querying for children child procs until
-        # none remain. Important that a single WmiLite instance
-        # is passed in since each creates its own WMI rpc process
-        def self.kill_process_tree(pid, wmi, logger)
-          wmi.query("select * from Win32_Process where ParentProcessID=#{pid}").each do |instance|
-            next if unsafe_process?(instance.wmi_ole_object.name, logger)
-            child_pid = instance.wmi_ole_object.processid
-            kill_process_tree(child_pid, wmi, logger)
-            kill_process(instance, logger)
-          end
-        end
+      def unsafe_process?(name, logger)
+        return false unless system_required_processes.include? name
+        logger.debug(
+          "A request to kill a critical system process - #{name} - was received and skipped."
+        )
+        true
+      end
 
-        def self.kill_process(instance, logger)
+      # recursively kills all child processes of given pid
+      # calls itself querying for children child procs until
+      # none remain. Important that a single WmiLite instance
+      # is passed in since each creates its own WMI rpc process
+      def kill_process_tree(pid, wmi, logger)
+        wmi.query("select * from Win32_Process where ParentProcessID=#{pid}").each do |instance|
+          next if unsafe_process?(instance.wmi_ole_object.name, logger)
           child_pid = instance.wmi_ole_object.processid
-          logger.debug([
-            "killing child process #{child_pid}::",
-            "#{instance.wmi_ole_object.Name} of parent #{pid}",
-            ].join) if logger
-          Process.kill(:KILL, instance.wmi_ole_object.processid)
-        rescue Errno::EIO, SystemCallError
-          logger.debug([
-            "Failed to kill child process #{child_pid}::",
-            "#{instance.wmi_ole_object.Name} of parent #{pid}",
-          ].join) if logger
+          kill_process_tree(child_pid, wmi, logger)
+          kill_process(instance, logger)
         end
+      end
+
+      def kill_process(instance, logger)
+        child_pid = instance.wmi_ole_object.processid
+        logger.debug([
+          "killing child process #{child_pid}::",
+          "#{instance.wmi_ole_object.Name} of parent #{pid}",
+        ].join) if logger
+        Process.kill(:KILL, instance.wmi_ole_object.processid)
+      rescue Errno::EIO, SystemCallError
+        logger.debug([
+          "Failed to kill child process #{child_pid}::",
+          "#{instance.wmi_ole_object.Name} of parent #{pid}",
+        ].join) if logger
+      end
+
+      def format_process(process, app_name, command_line, timeout)
+        msg = []
+        msg << "ProcessId: #{process.process_id}"
+        msg << "app_name: #{app_name}"
+        msg << "command_line: #{command_line}"
+        msg << "timeout: #{timeout}"
+        msg.join("\n")
+      end
 
-        def self.format_process(process, app_name, command_line, timeout)
-          msg = []
-          msg << "ProcessId: #{process.process_id}"
-          msg << "app_name: #{app_name}"
-          msg << "command_line: #{command_line}"
-          msg << "timeout: #{timeout}"
-          msg.join("\n")
+      # DEPRECATED do not use
+      class Utils
+        include Mixlib::ShellOut::Windows
+        def self.should_run_under_cmd?(cmd)
+          Mixlib::ShellOut::Windows::Utils.new.send(:should_run_under_cmd?, cmd)
         end
       end
-    end # class
+    end
   end
 end
diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb
index fe511eb..cd8ab04 100644
--- a/spec/mixlib/shellout/windows_spec.rb
+++ b/spec/mixlib/shellout/windows_spec.rb
@@ -4,7 +4,7 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
 
   describe "Utils" do
     describe ".should_run_under_cmd?" do
-      subject { Mixlib::ShellOut::Windows::Utils.should_run_under_cmd?(command) }
+      subject { Mixlib::ShellOut.new.send(:should_run_under_cmd?, command) }
 
       def self.with_command(_command, &example)
         context "with command: #{_command}" do
@@ -110,7 +110,7 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
     end
 
     describe ".kill_process_tree" do
-      let(:utils) { Mixlib::ShellOut::Windows::Utils }
+      let(:shell_out) { Mixlib::ShellOut.new }
       let(:wmi) { Object.new }
       let(:wmi_ole_object) { Object.new }
       let(:wmi_process) { Object.new }
@@ -129,8 +129,8 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
         end
 
         it "does not attempt to kill csrss.exe" do
-          expect(utils).to_not receive(:kill_process)
-          utils.kill_process_tree(200, wmi, logger)
+          expect(shell_out).to_not receive(:kill_process)
+          shell_out.send(:kill_process_tree, 200, wmi, logger)
         end
       end
 
@@ -141,10 +141,10 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
         end
 
         it "does attempt to kill blah.exe" do
-          expect(utils).to receive(:kill_process).with(wmi_process, logger)
-          expect(utils).to receive(:kill_process_tree).with(200, wmi, logger).and_call_original
-          expect(utils).to receive(:kill_process_tree).with(300, wmi, logger)
-          utils.kill_process_tree(200, wmi, logger)
+          expect(shell_out).to receive(:kill_process).with(wmi_process, logger)
+          expect(shell_out).to receive(:kill_process_tree).with(200, wmi, logger).and_call_original
+          expect(shell_out).to receive(:kill_process_tree).with(300, wmi, logger)
+          shell_out.send(:kill_process_tree, 200, wmi, logger)
         end
       end
     end
@@ -186,9 +186,8 @@ describe "Mixlib::ShellOut::Windows", :windows_only do
       let(:stubbed_shell_out) { raise NotImplemented, "Must declare let(:stubbed_shell_out)" }
       let(:shell_out) { Mixlib::ShellOut.new(cmd) }
 
-      let(:utils) { Mixlib::ShellOut::Windows::Utils }
-      let(:with_valid_exe_at_location) { lambda { |s| allow(utils).to receive(:find_executable).and_return(executable_path) } }
-      let(:with_invalid_exe_at_location) { lambda { |s| allow(utils).to receive(:find_executable).and_return(nil) } }
+      let(:with_valid_exe_at_location) { lambda { |s| allow(shell_out).to receive(:find_executable).and_return(executable_path) } }
+      let(:with_invalid_exe_at_location) { lambda { |s| allow(shell_out).to receive(:find_executable).and_return(nil) } }
 
       context "with empty command" do
         let(:stubbed_shell_out) { shell_out }
diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb
index b5df5dd..a55b3c0 100644
--- a/spec/mixlib/shellout_spec.rb
+++ b/spec/mixlib/shellout_spec.rb
@@ -1130,7 +1130,7 @@ describe Mixlib::ShellOut do
 
           context "and child processes should be killed" do
             it "kills the child processes" do
-              expect(Mixlib::ShellOut::Windows::Utils).to receive(:kill_process) do |instance|
+              expect(shell_cmd).to receive(:kill_process) do |instance|
                 expect(instance.wmi_ole_object.Name).to match(/powershell/)
                 Process.kill(:KILL, instance.wmi_ole_object.processid)
               end