Merge pull request #9052 from joshcooper/remove_env_monkeypatches

(PUP-11348) Use ENV directly on Windows

Author: tvpartytonight

lib/puppet/defaults.rb

@@ -369,11 +369,12 @@ def self.initialize_default_settings!(settings)
           be set in `[server]`, `[agent]`, or an environment config section.",
         :call_hook => :on_define_and_write,
         :hook             => proc do |value|
-          Puppet::Util.set_env('PATH', '') if Puppet::Util.get_env('PATH').nil?
-          Puppet::Util.set_env('PATH', value) unless value == 'none'
-          paths = Puppet::Util.get_env('PATH').split(File::PATH_SEPARATOR)
+          ENV['PATH'] = '' if ENV['PATH'].nil?
+          ENV['PATH'] = value unless value == 'none'
+          paths = ENV['PATH'].split(File::PATH_SEPARATOR)
           Puppet::Util::Platform.default_paths.each do |path|
-            Puppet::Util.set_env('PATH', Puppet::Util.get_env('PATH') + File::PATH_SEPARATOR + path) unless paths.include?(path)
+            next if paths.include?(path)
+            ENV['PATH'] = ENV['PATH'] + File::PATH_SEPARATOR + path
           end
           value
         end

lib/puppet/file_system/uniquefile.rb

@@ -150,7 +150,7 @@ def try_convert_to_hash(h)
 
   def tmpdir
     tmp = '.'
-    for dir in [ Puppet::Util.get_env('TMPDIR'), Puppet::Util.get_env('TMP'), Puppet::Util.get_env('TEMP'), @@systmpdir, '/tmp']
+    for dir in [ ENV['TMPDIR'], ENV['TMP'], ENV['TEMP'], @@systmpdir, '/tmp']
       stat = File.stat(dir) if dir
       if stat && stat.directory? && stat.writable?
         tmp = dir

lib/puppet/node/environment.rb

@@ -549,8 +549,8 @@ def hash
 
   # not private so it can be called in tests
   def self.extralibs()
-    if Puppet::Util.get_env('PUPPETLIB')
-      split_path(Puppet::Util.get_env('PUPPETLIB'))
+    if ENV['PUPPETLIB']
+      split_path(ENV['PUPPETLIB'])
     else
       []
     end

lib/puppet/provider/exec/windows.rb

@@ -45,7 +45,7 @@ def checkexe(command)
     end
 
     if resource[:path]
-      Puppet::Util.withenv( {'PATH' => resource[:path].join(File::PATH_SEPARATOR)}, :windows) do
+      Puppet::Util.withenv('PATH' => resource[:path].join(File::PATH_SEPARATOR)) do
         return if which(exe)
       end
     end

lib/puppet/provider/package/gem.rb

@@ -55,9 +55,9 @@ def self.provider_command
   # that contains the content of PATH but without puppet/bin dir.
   # This is used to pass a custom PATH and execute commands in a controlled environment
   def self.windows_path_without_puppet_bin
-    @path ||= Puppet::Util.get_env('PATH').split(File::PATH_SEPARATOR)
-                                          .reject { |dir| dir =~ /puppet\\bin$/ }
-                                          .join(File::PATH_SEPARATOR)
+    @path ||= ENV['PATH'].split(File::PATH_SEPARATOR)
+                         .reject { |dir| dir =~ /puppet\\bin$/ }
+                         .join(File::PATH_SEPARATOR)
   end
 
   private_class_method :windows_path_without_puppet_bin
@@ -73,7 +73,7 @@ def self.execute_gem_command(command, command_options, custom_environment = {})
     validate_command(command)
     cmd = [command] << command_options
 
-    custom_environment = {'HOME'=>Puppet::Util.get_env('HOME')}.merge(custom_environment)
+    custom_environment = {'HOME'=>ENV['HOME']}.merge(custom_environment)
 
     if Puppet::Util::Platform.windows?
       custom_environment[:PATH] = windows_path_without_puppet_bin

lib/puppet/provider/package/puppet_gem.rb

@@ -7,9 +7,9 @@
   confine :true => Puppet.runtime[:facter].value(:aio_agent_version)
 
   def self.windows_gemcmd
-    puppet_dir = Puppet::Util.get_env('PUPPET_DIR')
+    puppet_dir = ENV['PUPPET_DIR']
     if puppet_dir
-      File.join(Puppet::Util.get_env('PUPPET_DIR').to_s, 'bin', 'gem.bat')
+      File.join(ENV['PUPPET_DIR'].to_s, 'bin', 'gem.bat')
     else
       File.join(Gem.default_bindir, 'gem.bat')
     end

lib/puppet/test/test_helper.rb

@@ -69,13 +69,7 @@ def self.initialize()
     # @return nil
     def self.before_all_tests()
       # The process environment is a shared, persistent resource.
-      # Can't use Puppet.features.microsoft_windows? as it may be mocked out in a test.  This can cause test recurring test failures
-      if (!!File::ALT_SEPARATOR)
-        mode = :windows
-      else
-        mode = :posix
-      end
-      $old_env = Puppet::Util.get_environment(mode)
+      $old_env = ENV.to_hash
     end
 
     # Call this method once, at the end of a test run, when no more tests
@@ -188,20 +182,8 @@ def self.after_each_test()
       end
       $saved_indirection_state = nil
 
-      # Can't use Puppet.features.microsoft_windows? as it may be mocked out in a test.  This can cause test recurring test failures
-      if (!!File::ALT_SEPARATOR)
-        mode = :windows
-      else
-        mode = :posix
-      end
-      # Restore the global process environment.  Can't just assign because this
-      # is a magic variable, sadly, and doesn't do that™.  It is sufficiently
-      # faster to use the compare-then-set model to avoid excessive work that it
-      # justifies the complexity.  --daniel 2012-03-15
-      unless Puppet::Util.get_environment(mode) == $old_env
-        Puppet::Util.clear_environment(mode)
-        $old_env.each {|k, v| Puppet::Util.set_env(k, v, mode) }
-      end
+      # Restore the global process environment.
+      ENV.replace($old_env)
 
       # Clear all environments
       Puppet.lookup(:environments).clear_all

lib/puppet/util.rb

@@ -37,106 +37,55 @@ def default_env
   def create_erb(content)
     ERB.new(content, trim_mode: '-')
   end
-
   module_function :create_erb
 
-  # @param name [String] The name of the environment variable to retrieve
-  # @param mode [Symbol] Which operating system mode to use e.g. :posix or :windows.  Use nil to autodetect
-  # @return [String] Value of the specified environment variable.  nil if it does not exist
+  # @deprecated Use ENV instead
   # @api private
   def get_env(name, mode = default_env)
-    if mode == :windows
-      Puppet::Util::Windows::Process.get_environment_strings.each do |key, value |
-        if name.casecmp(key) == 0 then
-          return value
-        end
-      end
-      return nil
-    else
-      ENV[name]
-    end
+    ENV[name]
   end
   module_function :get_env
 
-  # @param mode [Symbol] Which operating system mode to use e.g. :posix or :windows.  Use nil to autodetect
-  # @return [Hash] A hashtable of all environment variables
+  # @deprecated Use ENV instead
   # @api private
   def get_environment(mode = default_env)
-    case mode
-      when :posix
-        ENV.to_hash
-      when :windows
-        Puppet::Util::Windows::Process.get_environment_strings
-      else
-        raise _("Unable to retrieve the environment for mode %{mode}") % { mode: mode }
-    end
+    ENV.to_hash
   end
   module_function :get_environment
 
-  # Removes all environment variables
-  # @param mode [Symbol] Which operating system mode to use e.g. :posix or :windows.  Use nil to autodetect
+  # @deprecated Use ENV instead
   # @api private
   def clear_environment(mode = default_env)
-    case mode
-      when :posix
-        ENV.clear
-      when :windows
-        Puppet::Util::Windows::Process.get_environment_strings.each do |key, _|
-          Puppet::Util::Windows::Process.set_environment_variable(key, nil)
-        end
-      else
-        raise _("Unable to clear the environment for mode %{mode}") % { mode: mode }
-    end
+    ENV.clear
   end
   module_function :clear_environment
 
-  # @param name [String] The name of the environment variable to set
-  # @param value [String] The value to set the variable to.  nil deletes the environment variable
-  # @param mode [Symbol] Which operating system mode to use e.g. :posix or :windows.  Use nil to autodetect
+  # @deprecated Use ENV instead
   # @api private
   def set_env(name, value = nil, mode = default_env)
-    case mode
-      when :posix
-        ENV[name] = value
-      when :windows
-        Puppet::Util::Windows::Process.set_environment_variable(name,value)
-      else
-        raise _("Unable to set the environment variable %{name} for mode %{mode}") % { name: name, mode: mode }
-    end
+    ENV[name] = value
   end
   module_function :set_env
 
-  # @param name [Hash] Environment variables to merge into the existing environment.  nil values will remove the variable
-  # @param mode [Symbol] Which operating system mode to use e.g. :posix or :windows.  Use nil to autodetect
+  # @deprecated Use ENV instead
   # @api private
   def merge_environment(env_hash, mode = default_env)
-    case mode
-      when :posix
-        env_hash.each { |name, val| ENV[name.to_s] = val }
-      when :windows
-        env_hash.each do |name, val|
-          Puppet::Util::Windows::Process.set_environment_variable(name.to_s, val)
-        end
-      else
-        raise _("Unable to merge given values into the current environment for mode %{mode}") % { mode: mode }
-    end
+    ENV.merge!(hash.transform_keys(&:to_s))
   end
   module_function :merge_environment
 
   # Run some code with a specific environment.  Resets the environment back to
   # what it was at the end of the code.
-  # Windows can store Unicode chars in the environment as keys or values, but
-  # Ruby's ENV tries to roundtrip them through the local codepage, which can
-  # cause encoding problems - underlying helpers use Windows APIs on Windows
-  # see https://bugs.ruby-lang.org/issues/8822
+  #
+  # @param hash [Hash{String,Symbol => String}] Environment variables to override the current environment.
+  # @param mode [Symbol] ignored
   def withenv(hash, mode = :posix)
-    saved = get_environment(mode)
-    merge_environment(hash, mode)
-    yield
-  ensure
-    if saved
-      clear_environment(mode)
-      merge_environment(saved, mode)
+    saved = ENV.to_hash
+    begin
+      ENV.merge!(hash.transform_keys(&:to_s))
+      yield
+    ensure
+      ENV.replace(saved)
     end
   end
   module_function :withenv
@@ -258,16 +207,16 @@ def which(bin)
     if absolute_path?(bin)
       return bin if FileTest.file? bin and FileTest.executable? bin
     else
-      exts = Puppet::Util.get_env('PATHEXT')
+      exts = ENV['PATHEXT']
       exts = exts ? exts.split(File::PATH_SEPARATOR) : %w[.COM .EXE .BAT .CMD]
-      Puppet::Util.get_env('PATH').split(File::PATH_SEPARATOR).each do |dir|
+      ENV['PATH'].split(File::PATH_SEPARATOR).each do |dir|
         begin
           dest = File.expand_path(File.join(dir, bin))
         rescue ArgumentError => e
           # if the user's PATH contains a literal tilde (~) character and HOME is not set, we may get
           # an ArgumentError here.  Let's check to see if that is the case; if not, re-raise whatever error
           # was thrown.
-          if e.to_s =~ /HOME/ and (Puppet::Util.get_env('HOME').nil? || Puppet::Util.get_env('HOME') == "")
+          if e.to_s =~ /HOME/ and (ENV['HOME'].nil? || ENV['HOME'] == "")
             # if we get here they have a tilde in their PATH.  We'll issue a single warning about this and then
             # ignore this path element and carry on with our lives.
             #TRANSLATORS PATH and HOME are environment variables and should not be translated

lib/puppet/util/execution.rb

@@ -379,7 +379,7 @@ def self.execute_windows(command, options, stdin, stdout, stderr)
     end.join(" ") if command.is_a?(Array)
 
     options[:custom_environment] ||= {}
-    Puppet::Util.withenv(options[:custom_environment], :windows) do
+    Puppet::Util.withenv(options[:custom_environment]) do
       Puppet::Util::Windows::Process.execute(command, options, stdin, stdout, stderr)
     end
   end

spec/integration/defaults_spec.rb

@@ -137,18 +137,18 @@
 
     it "path should not add anything" do
       path = "c:\\windows\\system32#{File::PATH_SEPARATOR}c:\\windows"
-      Puppet::Util.withenv( {"PATH" => path }, :windows ) do
+      Puppet::Util.withenv("PATH" => path) do
         Puppet.settings[:path] = "none" # this causes it to ignore the setting
         expect(ENV["PATH"]).to eq(path)
       end
     end
 
     it "path should support UTF8 characters" do
       path = "c:\\windows\\system32#{File::PATH_SEPARATOR}c:\\windows#{File::PATH_SEPARATOR}C:\\" + rune_utf8
-      Puppet::Util.withenv( {"PATH" => path }, :windows) do
+      Puppet::Util.withenv("PATH" => path) do
         Puppet.settings[:path] = "none" # this causes it to ignore the setting
 
-        expect(Puppet::Util.get_env('Path')).to eq(path)
+        expect(ENV['Path']).to eq(path)
       end
     end
   end

spec/integration/util_spec.rb

@@ -96,7 +96,7 @@
       Puppet::FileSystem.touch(filepath)
 
       path = [utf8, "c:\\windows\\system32", "c:\\windows"].join(File::PATH_SEPARATOR)
-      Puppet::Util.withenv( { "PATH" => path } , :windows) do
+      Puppet::Util.withenv("PATH" => path) do
         expect(Puppet::Util.which(filename)).to eq(filepath)
       end
     end

spec/unit/provider/package/gem_spec.rb

@@ -51,8 +51,8 @@
 
         before do
           allow(Puppet::Util::Platform).to receive(:windows?).and_return(true)
-          allow(Puppet::Util).to receive(:get_env)
-          allow(Puppet::Util).to receive(:get_env).with('PATH').and_return(path)
+          allow(ENV).to receive(:[]).and_call_original
+          allow(ENV).to receive(:[]).with('PATH').and_return(path)
           allow(described_class).to receive(:validate_command).with(provider_gem_cmd)
           stub_const('::File::PATH_SEPARATOR', ';')
         end

spec/unit/provider/package/puppet_gem_spec.rb

@@ -35,8 +35,8 @@
   describe '.windows_gemcmd' do
     context 'when PUPPET_DIR is not set' do
       before do
-        allow(Puppet::Util).to receive(:get_env).and_call_original
-        allow(Puppet::Util).to receive(:get_env).with('PUPPET_DIR').and_return(nil)
+        allow(ENV).to receive(:[]).and_call_original
+        allow(ENV).to receive(:[]).with('PUPPET_DIR').and_return(nil)
         allow(Gem).to receive(:default_bindir).and_return('default_gem_bin')
       end
 
@@ -48,8 +48,8 @@
 
     context 'when PUPPET_DIR is set' do
       before do
-        allow(Puppet::Util).to receive(:get_env).and_call_original
-        allow(Puppet::Util).to receive(:get_env).with('PUPPET_DIR').and_return('puppet_dir')
+        allow(ENV).to receive(:[]).and_call_original
+        allow(ENV).to receive(:[]).with('PUPPET_DIR').and_return('puppet_dir')
       end
 
       it 'uses Gem.default_bindir' do

spec/unit/test/test_helper_spec.rb

@@ -4,14 +4,14 @@
   context "#after_each_test" do
     it "restores the original environment" do
       varname = 'test_helper_spec-test_variable'
-      Puppet::Util.set_env(varname, "\u16A0")
+      ENV[varname] = "\u16A0"
 
-      expect(Puppet::Util.get_env(varname)).to eq("\u16A0")
+      expect(ENV[varname]).to eq("\u16A0")
 
       # Prematurely trigger the after_each_test method
       Puppet::Test::TestHelper.after_each_test
 
-      expect(Puppet::Util::get_env(varname)).to be_nil
+      expect(ENV[varname]).to be_nil
     end
   end
-end
+end