From 1a081d5f7f91299013a5d6946d7a1f6892be7ece Mon Sep 17 00:00:00 2001 From: Lamont Granquist Date: Wed, 1 Feb 2017 18:41:41 -0800 Subject: spec cleanup and some light refactoring - mostly cleans up the windows specs so its easier to read the API out of them and removes some of the very brittle internal testing - refactors the 'which' logic a bit. trying to converge towards the chef/chef version and eventually extracting common code so that do not have to maintain 10+ slightly different copies everywhere. - adds the Mixlib::ShellOut::EmptyWindowsCommand exception because letting CreateProcessW throw a generic SystemCallError is pretty much useless to everyone. Signed-off-by: Lamont Granquist --- lib/mixlib/shellout.rb | 6 +- lib/mixlib/shellout/exceptions.rb | 1 + lib/mixlib/shellout/windows.rb | 88 ++++------ spec/mixlib/shellout/windows_spec.rb | 307 +++++++++++++++-------------------- 4 files changed, 170 insertions(+), 232 deletions(-) diff --git a/lib/mixlib/shellout.rb b/lib/mixlib/shellout.rb index 3f1106f..c9f2fa1 100644 --- a/lib/mixlib/shellout.rb +++ b/lib/mixlib/shellout.rb @@ -253,7 +253,7 @@ module Mixlib # within +timeout+ seconds (default: 600s) def run_command if logger - log_message = (log_tag.nil? ? "" : "#@log_tag ") << "sh(#@command)" + log_message = (log_tag.nil? ? "" : "#{@log_tag} ") << "sh(#{@command})" logger.send(log_level, log_message) end super @@ -290,9 +290,9 @@ module Mixlib end def inspect - "<#{self.class.name}##{object_id}: command: '#@command' process_status: #{@status.inspect} " + + "<#{self.class.name}##{object_id}: command: '#{@command}' process_status: #{@status.inspect} " + "stdout: '#{stdout.strip}' stderr: '#{stderr.strip}' child_pid: #{@child_pid.inspect} " + - "environment: #{@environment.inspect} timeout: #{timeout} user: #@user group: #@group working_dir: #@cwd >" + "environment: #{@environment.inspect} timeout: #{timeout} user: #{@user} group: #{@group} working_dir: #{@cwd} >" end private diff --git a/lib/mixlib/shellout/exceptions.rb b/lib/mixlib/shellout/exceptions.rb index af79721..e1fae8d 100644 --- a/lib/mixlib/shellout/exceptions.rb +++ b/lib/mixlib/shellout/exceptions.rb @@ -4,5 +4,6 @@ module Mixlib class ShellCommandFailed < Error; end class CommandTimeout < Error; end class InvalidCommandOption < Error; end + class EmptyWindowsCommand < Error; end end end diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb index 094e9b3..5f6058d 100644 --- a/lib/mixlib/shellout/windows.rb +++ b/lib/mixlib/shellout/windows.rb @@ -32,10 +32,8 @@ module Mixlib # Option validation that is windows specific def validate_options(opts) - if opts[:user] - unless opts[:password] - raise InvalidCommandOption, "You must supply both a username and password when supplying a user in windows" - end + if opts[:user] && !opts[:password] + raise InvalidCommandOption, "You must supply a password when supplying a user in windows" end end @@ -185,51 +183,45 @@ module Mixlib return true end - IS_BATCH_FILE = /\.bat"?$|\.cmd"?$/i - def command_to_run(command) - return _run_under_cmd(command) if should_run_under_cmd?(command) + return run_under_cmd(command) if should_run_under_cmd?(command) candidate = candidate_executable_for_command(command) - # Don't do searching for empty commands. Let it fail when it runs. - return [ nil, command ] if candidate.length == 0 + if candidate.length == 0 + raise Mixlib::ShellOut::EmptyWindowsCommand, "could not parse script/executable out of command: `#{command}`" + end # Check if the exe exists directly. Otherwise, search PATH. - 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. - if exe.nil? || exe =~ IS_BATCH_FILE - _run_under_cmd(command) + exe = which(candidate) + if exe_needs_cmd?(exe) + run_under_cmd(command) else - _run_directly(command, exe) + [ exe, command ] end end + # 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. + def exe_needs_cmd?(exe) + !exe || exe =~ /\.bat"?$|\.cmd"?$/i + end + # cmd does not parse multiple quotes well unless the whole thing is wrapped up in quotes. # https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4837859 # http://ss64.com/nt/syntax-esc.html - def _run_under_cmd(command) + def run_under_cmd(command) [ ENV["COMSPEC"], "cmd /c \"#{command}\"" ] end - def _run_directly(command, exe) - [ exe, command ] - end - - def unquoted_executable_path(command) - command[0, command.index(/\s/) || command.length] - end - + # FIXME: this extracts ARGV[0] but is it correct? def candidate_executable_for_command(command) if command =~ /^\s*"(.*?)"/ # If we have quotes, do an exact match $1 else # Otherwise check everything up to the first space - unquoted_executable_path(command).strip + command[0, command.index(/\s/) || command.length].strip end end @@ -289,35 +281,25 @@ module Mixlib return false end - def pathext - @pathext ||= ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""] - end - - # which() mimicks the Unix which command - # FIXME: it is not working + # FIXME: reduce code duplication with chef/chef def which(cmd) - ENV["PATH"].split(File::PATH_SEPARATOR).each do |path| - exe = find_executable("#{path}/#{cmd}") - return exe if exe + exts = ENV["PATHEXT"] ? ENV["PATHEXT"].split(";") + [""] : [""] + # windows always searches '.' first + exts.each do |ext| + filename = "#{cmd}#{ext}" + return filename if File.executable?(filename) && !File.directory?(filename) 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 - - pathext.each do |ext| - exe = "#{path}#{ext}" - return exe if executable? exe + # only search through the path if the Filename does not contain separators + if File.basename(cmd) == cmd + paths = ENV["PATH"].split(File::PATH_SEPARATOR) + paths.each do |path| + exts.each do |ext| + filename = File.join(path, "#{cmd}#{ext}") + return filename if File.executable?(filename) && !File.directory?(filename) + end + end end - return nil - end - - def executable?(path) - File.executable?(path) && !File.directory?(path) + false end def system_required_processes diff --git a/spec/mixlib/shellout/windows_spec.rb b/spec/mixlib/shellout/windows_spec.rb index cd8ab04..4a94eb4 100644 --- a/spec/mixlib/shellout/windows_spec.rb +++ b/spec/mixlib/shellout/windows_spec.rb @@ -1,5 +1,7 @@ require "spec_helper" +# FIXME: these are stubby enough unit tests that they almost run under unix, but the +# Mixlib::ShellOut object does not mixin the Windows behaviors when running on unix. describe "Mixlib::ShellOut::Windows", :windows_only do describe "Utils" do @@ -152,203 +154,156 @@ describe "Mixlib::ShellOut::Windows", :windows_only do # Caveat: Private API methods are subject to change without notice. # Monkeypatch at your own risk. - context "for private API methods" do - - describe "::IS_BATCH_FILE" do - subject { candidate =~ Mixlib::ShellOut::Windows::IS_BATCH_FILE } - - def self.with_candidate(_context, _options = {}, &example) - context "with #{_context}" do - let(:candidate) { _options[:candidate] } - it(&example) - end - end - - with_candidate("valid .bat file", :candidate => "autoexec.bat") { is_expected.to be_truthy } - with_candidate("valid .cmd file", :candidate => "autoexec.cmd") { is_expected.to be_truthy } - with_candidate("valid quoted .bat file", :candidate => '"C:\Program Files\autoexec.bat"') { is_expected.to be_truthy } - with_candidate("valid quoted .cmd file", :candidate => '"C:\Program Files\autoexec.cmd"') { is_expected.to be_truthy } - - with_candidate("invalid .bat file", :candidate => "autoexecbat") { is_expected.not_to be_truthy } - with_candidate("invalid .cmd file", :candidate => "autoexeccmd") { is_expected.not_to be_truthy } - with_candidate("bat in filename", :candidate => "abattoir.exe") { is_expected.not_to be_truthy } - with_candidate("cmd in filename", :candidate => "parse_cmd.exe") { is_expected.not_to be_truthy } - - with_candidate("invalid quoted .bat file", :candidate => '"C:\Program Files\autoexecbat"') { is_expected.not_to be_truthy } - with_candidate("invalid quoted .cmd file", :candidate => '"C:\Program Files\autoexeccmd"') { is_expected.not_to be_truthy } - with_candidate("quoted bat in filename", :candidate => '"C:\Program Files\abattoir.exe"') { is_expected.not_to be_truthy } - with_candidate("quoted cmd in filename", :candidate => '"C:\Program Files\parse_cmd.exe"') { is_expected.not_to be_truthy } - end + context "#command_to_run" do describe "#command_to_run" do - subject { stubbed_shell_out.send(:command_to_run, cmd) } - - let(:stubbed_shell_out) { raise NotImplemented, "Must declare let(:stubbed_shell_out)" } - let(:shell_out) { Mixlib::ShellOut.new(cmd) } - - 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 } - let(:cmd) { " " } - - it "should return with a nil executable" do - is_expected.to eql([nil, cmd]) - end - end - - context "with extensionless executable" do - let(:stubbed_shell_out) { shell_out } - let(:executable_path) { 'C:\Windows\system32/ping.EXE' } - let(:cmd) { "ping" } - - before do - allow(ENV).to receive(:[]).with("PATH").and_return('C:\Windows\system32') - allow(ENV).to receive(:[]).with("PATHEXT").and_return(".EXE") - allow(ENV).to receive(:[]).with("COMSPEC").and_return('C:\Windows\system32\cmd.exe') - allow(File).to receive(:executable?).and_return(false) - allow(File).to receive(:executable?).with(executable_path).and_return(true) - allow(File).to receive(:directory?).and_return(false) - end - - it "should return with full path with extension" do - is_expected.to eql([executable_path, cmd]) - end - - context "there is a directory named after command" do + subject { shell_out.send(:command_to_run, command) } + + # @param cmd [String] command string + # @param filename [String] the pathname to the executable that will be found (nil to have no pathname match) + # @param search [Boolean] false: will setup expectation not to search PATH, true: will setup expectations that it searches the PATH + # @param directory [Boolean] true: will setup an expectation that the search strategy will find a directory + def self.with_command(cmd, filename: nil, search: false, directory: false, &example) + context "with #{cmd}" do + let(:shell_out) { Mixlib::ShellOut.new } + let(:comspec) { 'C:\Windows\system32\cmd.exe' } + let(:command) { cmd } before do - # File.executable? returns true for directories - allow(File).to receive(:executable?).with(cmd).and_return(true) - allow(File).to receive(:directory?).with(cmd).and_return(true) - end - - it "should return with full path with extension" do - is_expected.to eql([executable_path, cmd]) - end - end - end - - context "with batch files" do - let(:stubbed_shell_out) { shell_out.tap(&with_valid_exe_at_location) } - let(:cmd_invocation) { "cmd /c \"#{cmd}\"" } - let(:cmd_exe) { "C:\\Windows\\system32\\cmd.exe" } - let(:cmd) { "#{executable_path}" } - - before { ENV["ComSpec"] = 'C:\Windows\system32\cmd.exe' } - - context "with .bat file" do - let(:executable_path) { '"C:\Program Files\Application\Start.bat"' } - - # Examples taken from: https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4825574 - context "with executable path enclosed in double quotes" do - - it "should use specified batch file" do - is_expected.to eql([cmd_exe, cmd_invocation]) + if search + expect(ENV).to receive(:[]).with("PATH").and_return('C:\Windows\system32') + else + expect(ENV).not_to receive(:[]).with("PATH") end - - context "with arguments" do - let(:cmd) { "#{executable_path} arguments" } - - it "should use specified executable" do - is_expected.to eql([cmd_exe, cmd_invocation]) - end + allow(ENV).to receive(:[]).with("PATHEXT").and_return(".COM;.EXE;.BAT;.CMD") + allow(ENV).to receive(:[]).with("COMSPEC").and_return(comspec) + allow(File).to receive(:executable?).and_return(false) + if filename + expect(File).to receive(:executable?).with(filename).and_return(true) + expect(File).to receive(:directory?).with(filename).and_return(false) end - - context "with quoted arguments" do - let(:cmd) { "#{executable_path} /i \"C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll\"" } - - it "should use specified executable" do - is_expected.to eql([cmd_exe, cmd_invocation]) - end + if directory + expect(File).to receive(:executable?).with(cmd).and_return(true) + expect(File).to receive(:directory?).with(cmd).and_return(true) end end - end - - context "with .cmd file" do - let(:executable_path) { '"C:\Program Files\Application\Start.cmd"' } - - # Examples taken from: https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4825574 - context "with executable path enclosed in double quotes" do - - it "should use specified batch file" do - is_expected.to eql([cmd_exe, cmd_invocation]) - end - - context "with arguments" do - let(:cmd) { "#{executable_path} arguments" } - - it "should use specified executable" do - is_expected.to eql([cmd_exe, cmd_invocation]) - end - end - - context "with quoted arguments" do - let(:cmd) { "#{executable_path} /i \"C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll\"" } - - it "should use specified executable" do - is_expected.to eql([cmd_exe, cmd_invocation]) - end - end - end - + it(&example) end end - context "with valid executable at location" do - let(:stubbed_shell_out) { shell_out.tap(&with_valid_exe_at_location) } - - context "with executable path" do - let(:cmd) { "#{executable_path}" } - let(:executable_path) { 'C:\RUBY192\bin\ruby.exe' } - - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end + # quoted and unqouted commands that have correct bat and cmd extensions + with_command("autoexec.bat", filename: "autoexec.bat") do + is_expected.to eql([ comspec, 'cmd /c "autoexec.bat"']) + end + with_command("autoexec.cmd", filename: "autoexec.cmd") do + is_expected.to eql([ comspec, 'cmd /c "autoexec.cmd"']) + end + with_command('"C:\Program Files\autoexec.bat"', filename: 'C:\Program Files\autoexec.bat') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\autoexec.bat""']) + end + with_command('"C:\Program Files\autoexec.cmd"', filename: 'C:\Program Files\autoexec.cmd') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\autoexec.cmd""']) + end - context "with arguments" do - let(:cmd) { "#{executable_path} arguments" } + # lookups via PATHEXT + with_command("autoexec", filename: "autoexec.BAT") do + is_expected.to eql([ comspec, 'cmd /c "autoexec"']) + end + with_command("autoexec", filename: "autoexec.CMD") do + is_expected.to eql([ comspec, 'cmd /c "autoexec"']) + end - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end - end + # unquoted commands that have "bat" or "cmd" in the wrong place + with_command("autoexecbat", filename: "autoexecbat") do + is_expected.to eql(%w{autoexecbat autoexecbat}) + end + with_command("autoexeccmd", filename: "autoexeccmd") do + is_expected.to eql(%w{autoexeccmd autoexeccmd}) + end + with_command("abattoir.exe", filename: "abattoir.exe") do + is_expected.to eql([ "abattoir.exe", "abattoir.exe" ]) + end + with_command("parse_cmd.exe", filename: "parse_cmd.exe") do + is_expected.to eql([ "parse_cmd.exe", "parse_cmd.exe" ]) + end - context "with quoted arguments" do - let(:cmd) { "#{executable_path} -e \"print 'fee fie foe fum'\"" } + # quoted commands that have "bat" or "cmd" in the wrong place + with_command('"C:\Program Files\autoexecbat"', filename: 'C:\Program Files\autoexecbat') do + is_expected.to eql([ 'C:\Program Files\autoexecbat', '"C:\Program Files\autoexecbat"' ]) + end + with_command('"C:\Program Files\autoexeccmd"', filename: 'C:\Program Files\autoexeccmd') do + is_expected.to eql([ 'C:\Program Files\autoexeccmd', '"C:\Program Files\autoexeccmd"']) + end + with_command('"C:\Program Files\abattoir.exe"', filename: 'C:\Program Files\abattoir.exe') do + is_expected.to eql([ 'C:\Program Files\abattoir.exe', '"C:\Program Files\abattoir.exe"' ]) + end + with_command('"C:\Program Files\parse_cmd.exe"', filename: 'C:\Program Files\parse_cmd.exe') do + is_expected.to eql([ 'C:\Program Files\parse_cmd.exe', '"C:\Program Files\parse_cmd.exe"' ]) + end - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end - end - end + # empty command + with_command(" ") do + expect { subject }.to raise_error(Mixlib::ShellOut::EmptyWindowsCommand) + end - # Examples taken from: https://github.com/opscode/mixlib-shellout/pull/2#issuecomment-4825574 - context "with executable path enclosed in double quotes" do - let(:cmd) { "#{executable_path}" } - let(:executable_path) { '"C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\Bin\NETFX 4.0 Tools\gacutil.exe"' } + # extensionless executable + with_command("ping", filename: 'C:\Windows\system32/ping.EXE', search: true) do + is_expected.to eql([ 'C:\Windows\system32/ping.EXE', "ping" ]) + end - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end + # it ignores directories + with_command("ping", filename: 'C:\Windows\system32/ping.EXE', directory: true, search: true) do + is_expected.to eql([ 'C:\Windows\system32/ping.EXE', "ping" ]) + end - context "with arguments" do - let(:cmd) { "#{executable_path} arguments" } + # https://github.com/opscode/mixlib-shellout/pull/2 with bat file + with_command('"C:\Program Files\Application\Start.bat"', filename: 'C:\Program Files\Application\Start.bat') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.bat""' ]) + end + with_command('"C:\Program Files\Application\Start.bat" arguments', filename: 'C:\Program Files\Application\Start.bat') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.bat" arguments"' ]) + end + with_command('"C:\Program Files\Application\Start.bat" /i "C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll"', filename: 'C:\Program Files\Application\Start.bat') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.bat" /i "C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll""' ]) + end - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end - end + # https://github.com/opscode/mixlib-shellout/pull/2 with cmd file + with_command('"C:\Program Files\Application\Start.cmd"', filename: 'C:\Program Files\Application\Start.cmd') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.cmd""' ]) + end + with_command('"C:\Program Files\Application\Start.cmd" arguments', filename: 'C:\Program Files\Application\Start.cmd') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.cmd" arguments"' ]) + end + with_command('"C:\Program Files\Application\Start.cmd" /i "C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll"', filename: 'C:\Program Files\Application\Start.cmd') do + is_expected.to eql([ comspec, 'cmd /c ""C:\Program Files\Application\Start.cmd" /i "C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll""' ]) + end - context "with quoted arguments" do - let(:cmd) { "#{executable_path} /i \"C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll\"" } + # https://github.com/opscode/mixlib-shellout/pull/2 with unquoted exe file + with_command('C:\RUBY192\bin\ruby.exe', filename: 'C:\RUBY192\bin\ruby.exe') do + is_expected.to eql([ 'C:\RUBY192\bin\ruby.exe', 'C:\RUBY192\bin\ruby.exe' ]) + end + with_command('C:\RUBY192\bin\ruby.exe arguments', filename: 'C:\RUBY192\bin\ruby.exe') do + is_expected.to eql([ 'C:\RUBY192\bin\ruby.exe', 'C:\RUBY192\bin\ruby.exe arguments' ]) + end + with_command('C:\RUBY192\bin\ruby.exe -e "print \'fee fie foe fum\'"', filename: 'C:\RUBY192\bin\ruby.exe') do + is_expected.to eql([ 'C:\RUBY192\bin\ruby.exe', 'C:\RUBY192\bin\ruby.exe -e "print \'fee fie foe fum\'"' ]) + end - it "should use specified executable" do - is_expected.to eql([executable_path, cmd]) - end - end - end + # https://github.com/opscode/mixlib-shellout/pull/2 with quoted exe file + exe_with_spaces = 'C:\Program Files (x86)\Microsoft SDKs\Windows\v7.0A\Bin\NETFX 4.0 Tools\gacutil.exe' + with_command("\"#{exe_with_spaces}\"", filename: exe_with_spaces) do + is_expected.to eql([ exe_with_spaces, "\"#{exe_with_spaces}\"" ]) + end + with_command("\"#{exe_with_spaces}\" arguments", filename: exe_with_spaces) do + is_expected.to eql([ exe_with_spaces, "\"#{exe_with_spaces}\" arguments" ]) + end + long_options = "/i \"C:\Program Files (x86)\NUnit 2.6\bin\framework\nunit.framework.dll\"" + with_command("\"#{exe_with_spaces}\" #{long_options}", filename: exe_with_spaces) do + is_expected.to eql([ exe_with_spaces, "\"#{exe_with_spaces}\" #{long_options}" ]) + end + # shell built in + with_command("copy thing1.txt thing2.txt", search: true) do + is_expected.to eql([ comspec, 'cmd /c "copy thing1.txt thing2.txt"' ]) end end end -- cgit v1.2.1