unicode shell_out fixes plus mixlib-shellout 2.x

- use en_US.UTF-8 explicitly rather than relying on mixlib-shellout's
  'C' locale in order to force the LANG by default (restores unicode
  to most chef-client calls now)

5 files changed, 168 insertions, 82 deletions

diff --git a/Gemfile b/Gemfile
index 1418235ebc..0aedba19d1 100644
--- a/Gemfile
+++ b/Gemfile
@@ -3,6 +3,9 @@ gemspec :name => "chef"
 
 gem "activesupport", "< 4.0.0", :group => :compat_testing, :platform => "ruby"
 
+gem "mixlib-shellout", github: "opscode/mixlib-shellout", branch: "lcg/remove-lc-all-hack"
+gem "ohai", github: "opscode/ohai", branch: "master"
+
 group(:docgen) do
   gem "yard"
 end
diff --git a/chef.gemspec b/chef.gemspec
index 89b4ac2665..fb5cf8406e 100644
--- a/chef.gemspec
+++ b/chef.gemspec
@@ -19,6 +19,7 @@ Gem::Specification.new do |s|
   s.add_dependency "mixlib-cli", "~> 1.4"
   s.add_dependency "mixlib-log", "~> 1.3"
   s.add_dependency "mixlib-authentication", "~> 1.3"
+  # FIXME: needs to be bumped to ~> 2.0 once 2.0 is released
   s.add_dependency "mixlib-shellout", "~> 1.4"
   s.add_dependency "ohai", "~> 7.2"
 
diff --git a/lib/chef/mixin/shell_out.rb b/lib/chef/mixin/shell_out.rb
index 881c94b862..fb563f068d 100644
--- a/lib/chef/mixin/shell_out.rb
+++ b/lib/chef/mixin/shell_out.rb
@@ -30,32 +30,37 @@ class Chef
       # Generally speaking, 'extend Chef::Mixin::ShellOut' in your recipes and include 'Chef::Mixin::ShellOut' in your LWRPs
       # You can also call Mixlib::Shellout.new directly, but you lose all of the above functionality
 
+      # we use 'en_US.UTF-8' by default because we parse localized strings in English as an API and
+      # generally must support UTF-8 unicode.
       def shell_out(*command_args)
-        cmd = Mixlib::ShellOut.new(*run_command_compatible_options(command_args))
-        cmd.live_stream ||= io_for_live_stream
-        cmd.run_command
-        cmd
+        args = Marshal.load( Marshal.dump(command_args) )  # we need a deep clone
+        if args.last.is_a?(Hash)
+          options = args.last
+          env_key = options.has_key?(:env) ? :env : :environment
+          options[env_key] ||= {}
+          options[env_key]['LC_ALL'] ||= 'en_US.UTF-8' unless options[env_key].has_key?('LC_ALL')
+        else
+          args << { :environment => { 'LC_ALL' => 'en_US.UTF-8' } }
+        end
+
+        shell_out_command(*args)
       end
 
+      # call shell_out (using en_US.UTF-8) and raise errors
       def shell_out!(*command_args)
-        cmd= shell_out(*command_args)
+        cmd = shell_out(*command_args)
         cmd.error!
         cmd
       end
 
-      # environment['LC_ALL'] should be nil or what the user specified
       def shell_out_with_systems_locale(*command_args)
-        args = command_args.dup
-        if args.last.is_a?(Hash)
-          options = args.last
-          env_key = options.has_key?(:env) ? :env : :environment
-          options[env_key] ||= {}
-          options[env_key]['LC_ALL'] ||= nil
-        else
-          args << { :environment => { 'LC_ALL' => nil } }
-        end
+        shell_out_command(*command_args)
+      end
 
-        shell_out(*args)
+      def shell_out_with_systems_locale!(*command_args)
+        cmd = shell_out_with_systems_locale(*command_args)
+        cmd.error!
+        cmd
       end
 
       DEPRECATED_OPTIONS =
@@ -82,6 +87,13 @@ class Chef
 
       private
 
+      def shell_out_command(*command_args)
+        cmd = Mixlib::ShellOut.new(*run_command_compatible_options(command_args))
+        cmd.live_stream ||= io_for_live_stream
+        cmd.run_command
+        cmd
+      end
+
       def deprecate_option(old_option, new_option)
         Chef::Log.logger.warn "DEPRECATION: Chef::Mixin::ShellOut option :#{old_option} is deprecated. Use :#{new_option}"
       end
diff --git a/spec/functional/mixin/shell_out_spec.rb b/spec/functional/mixin/shell_out_spec.rb
index 92492fcf6f..35e5b30eae 100644
--- a/spec/functional/mixin/shell_out_spec.rb
+++ b/spec/functional/mixin/shell_out_spec.rb
@@ -29,7 +29,7 @@ describe Chef::Mixin::ShellOut do
           shell_out_with_systems_locale('echo $LC_ALL')
         end
 
-        cmd.stdout.chomp.should match_environment_variable('LC_ALL')
+        expect(cmd.stdout.chomp).to match_environment_variable('LC_ALL')
       end
     end
 
@@ -41,7 +41,7 @@ describe Chef::Mixin::ShellOut do
           shell_out_with_systems_locale('echo $LC_ALL', :environment => {'LC_ALL' => 'POSIX'})
         end
 
-        cmd.stdout.chomp.should eq 'POSIX'
+        expect(cmd.stdout.chomp).to eq 'POSIX'
       end
     end
   end
diff --git a/spec/unit/mixin/shell_out_spec.rb b/spec/unit/mixin/shell_out_spec.rb
index 1f85ec6bf1..744492a1a9 100644
--- a/spec/unit/mixin/shell_out_spec.rb
+++ b/spec/unit/mixin/shell_out_spec.rb
@@ -32,7 +32,7 @@ describe Chef::Mixin::ShellOut do
 
     let(:output) { StringIO.new }
     let!(:capture_log_output) { Chef::Log.logger = Logger.new(output)  }
-    let(:assume_deprecation_log_level) { Chef::Log.stub(:level).and_return(:warn) }
+    let(:assume_deprecation_log_level) { allow(Chef::Log).to receive(:level).and_return(:warn) }
 
     context 'without options' do
       let(:command_args) { [ cmd ] }
@@ -55,9 +55,9 @@ describe Chef::Mixin::ShellOut do
       it 'should emit a deprecation warning' do
         assume_deprecation_log_level and capture_log_output
         subject
-        output.string.should match /DEPRECATION:/
-        output.string.should match Regexp.escape(old_option.to_s)
-        output.string.should match Regexp.escape(new_option.to_s)
+        expect(output.string).to match /DEPRECATION:/
+        expect(output.string).to match Regexp.escape(old_option.to_s)
+        expect(output.string).to match Regexp.escape(new_option.to_s)
       end
     end
 
@@ -106,7 +106,7 @@ describe Chef::Mixin::ShellOut do
     end
   end
 
-  describe "#shell_out_with_systems_locale" do
+  context "when testing individual methods" do
     before(:each) do
       @original_env = ENV.to_hash
       ENV.clear
@@ -120,82 +120,152 @@ describe Chef::Mixin::ShellOut do
     let(:shell_out) { Chef::Mixin::ShellOut }
     let(:cmd) { "echo '#{rand(1000)}'" }
 
-    describe "when the last argument is a Hash" do
-      describe "and environment is an option" do
-        it "should not change environment['LC_ALL'] when set to nil" do
-          options = { :environment => { 'LC_ALL' => nil } }
-          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+    describe "#shell_out" do
+
+      describe "when the last argument is a Hash" do
+        describe "and environment is an option" do
+          it "should not change environment['LC_ALL'] when set to nil" do
+            options = { :environment => { 'LC_ALL' => nil } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should not change environment['LC_ALL'] when set to non-nil" do
+            options = { :environment => { 'LC_ALL' => 'en_US.UTF-8' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should set environment['LC_ALL'] to 'en_US.UTF-8' when 'LC_ALL' not present" do
+            options = { :environment => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, {
+              :environment => { 'HOME' => '/Users/morty', 'LC_ALL' => 'en_US.UTF-8' },
+            }).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should not mutate the options hash when it adds LC_ALL" do
+            options = { :environment => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, {
+              :environment => { 'HOME' => '/Users/morty', 'LC_ALL' => 'en_US.UTF-8' },
+            }).and_return(true)
+            shell_out.shell_out(cmd, options)
+            expect(options[:environment].has_key?('LC_ALL')).to be false
+          end
         end
 
-        it "should not change environment['LC_ALL'] when set to non-nil" do
-          options = { :environment => { 'LC_ALL' => 'en_US.UTF-8' } }
-          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+        describe "and env is an option" do
+          it "should not change env when set to nil" do
+            options = { :env => { 'LC_ALL' => nil } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should not change env when set to non-nil" do
+            options = { :env => { 'LC_ALL' => 'de_DE.UTF-8'}}
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should set env['LC_ALL'] to 'en_US.UTF-8' when 'LC_ALL' not present" do
+            options = { :env => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, {
+              :env => { 'HOME' => '/Users/morty', 'LC_ALL' => 'en_US.UTF-8' },
+            }).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
+
+          it "should not mutate the options hash when it adds LC_ALL" do
+            options = { :env => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, {
+              :env => { 'HOME' => '/Users/morty', 'LC_ALL' => 'en_US.UTF-8' },
+            }).and_return(true)
+            shell_out.shell_out(cmd, options)
+            expect(options[:env].has_key?('LC_ALL')).to be false
+          end
         end
 
-        it "should set environment['LC_ALL'] to nil when 'LC_ALL' not present" do
-          options = { :environment => { 'HOME' => '/Users/morty' } }
-          shell_out.should_receive(:shell_out).with(
-            cmd,
-            { :environment => {
-                'HOME' => '/Users/morty',
-                'LC_ALL' => nil }
-            }
-          ).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+        describe "and no env/environment option is present" do
+          it "should add environment option and set environment['LC_ALL'] to 'en_US.UTF_8'" do
+            options = { :user => 'morty' }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, {
+              :user => 'morty', :environment => { 'LC_ALL' => 'en_US.UTF-8' },
+            }).and_return(true)
+            shell_out.shell_out(cmd, options)
+          end
         end
       end
 
-      describe "and env is an option" do
-        it "should not change env when set to nil" do
-          options = { :env => { 'LC_ALL' => nil } }
-          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+      describe "when the last argument is not a Hash" do
+        it "should add environment options and set environment['LC_ALL'] to 'en_US.UTF-8'" do
+          expect(shell_out).to receive(:shell_out_command).with(cmd, {
+            :environment => { 'LC_ALL' => 'en_US.UTF-8' },
+          }).and_return(true)
+          shell_out.shell_out(cmd)
         end
+      end
+
+    end
 
-        it "should not change env when set to non-nil" do
-          options = { :env => { 'LC_ALL' => 'en_US.UTF-8'}}
-          shell_out.should_receive(:shell_out).with(cmd, options).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+    describe "#shell_out_with_systems_locale" do
+
+      describe "when the last argument is a Hash" do
+        describe "and environment is an option" do
+          it "should not change environment['LC_ALL'] when set to nil" do
+            options = { :environment => { 'LC_ALL' => nil } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
+
+          it "should not change environment['LC_ALL'] when set to non-nil" do
+            options = { :environment => { 'LC_ALL' => 'en_US.UTF-8' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
+
+          it "should no longer set environment['LC_ALL'] to nil when 'LC_ALL' not present" do
+            options = { :environment => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
         end
 
-        it "should set env['LC_ALL'] to nil when 'LC_ALL' not present" do
-          options = { :env => { 'HOME' => '/Users/morty' } }
-          shell_out.should_receive(:shell_out).with(
-            cmd,
-            { :env => {
-                'HOME' => '/Users/morty',
-                'LC_ALL' => nil }
-            }
-          ).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+        describe "and env is an option" do
+          it "should not change env when set to nil" do
+            options = { :env => { 'LC_ALL' => nil } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
+
+          it "should not change env when set to non-nil" do
+            options = { :env => { 'LC_ALL' => 'en_US.UTF-8'}}
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
+
+          it "should no longer set env['LC_ALL'] to nil when 'LC_ALL' not present" do
+            options = { :env => { 'HOME' => '/Users/morty' } }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
         end
-      end
 
-      describe "and no env/environment option is present" do
-        it "should add environment option and set environment['LC_ALL'] to nil" do
-          options = { :user => 'morty' }
-          shell_out.should_receive(:shell_out).with(
-            cmd,
-            { :environment => { 'LC_ALL' => nil },
-              :user => 'morty'
-            }
-          ).and_return(true)
-          shell_out.shell_out_with_systems_locale(cmd, options)
+        describe "and no env/environment option is present" do
+          it "should no longer add environment option and set environment['LC_ALL'] to nil" do
+            options = { :user => 'morty' }
+            expect(shell_out).to receive(:shell_out_command).with(cmd, options).and_return(true)
+            shell_out.shell_out_with_systems_locale(cmd, options)
+          end
         end
       end
-    end
 
-    describe "when the last argument is not a Hash" do
-      it "should add environment options and set environment['LC_ALL'] to nil" do
-        shell_out.should_receive(:shell_out).with(
-          cmd,
-          { :environment => { 'LC_ALL' => nil } }
-        ).and_return(true)
-        shell_out.shell_out_with_systems_locale(cmd)
+      describe "when the last argument is not a Hash" do
+        it "should no longer add environment options and set environment['LC_ALL'] to nil" do
+          expect(shell_out).to receive(:shell_out_command).with(cmd).and_return(true)
+          shell_out.shell_out_with_systems_locale(cmd)
+        end
       end
     end
-  end
 
+  end
 end