From e0602f707e98529c1340190b3b9cf40e19cc6b06 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 24 Sep 2021 12:03:58 -0700 Subject: [PATCH 1/2] (maint) Remove overstubbing Dir.glob behaves differently across platforms and ruby versions, don't stub it. --- spec/unit/util/autoload_spec.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/spec/unit/util/autoload_spec.rb b/spec/unit/util/autoload_spec.rb index 3a03d12af85..be3e8a79b1f 100644 --- a/spec/unit/util/autoload_spec.rb +++ b/spec/unit/util/autoload_spec.rb @@ -178,15 +178,14 @@ def with_libdir(libdir) end describe "when loading all files" do + let(:basedir) { tmpdir('autoloader') } + let(:path) { File.join(basedir, @autoload.path, 'file.rb') } + before do - allow(@autoload.class).to receive(:search_directories).and_return([make_absolute("/a")]) - allow(FileTest).to receive(:directory?).and_return(true) - allow(Dir).to receive(:glob).and_return([make_absolute("/a/foo/file.rb")]) - allow(Puppet::FileSystem).to receive(:exist?).and_return(true) - @time_a = Time.utc(2010, 'jan', 1, 6, 30) - allow(File).to receive(:mtime).and_return(@time_a) + FileUtils.mkdir_p(File.dirname(path)) + FileUtils.touch(path) - allow(@autoload.class).to receive(:loaded?).and_return(false) + allow(@autoload.class).to receive(:search_directories).and_return([basedir]) end [RuntimeError, LoadError, SyntaxError].each do |error| @@ -198,7 +197,7 @@ def with_libdir(libdir) end it "should require the full path to the file" do - expect(Kernel).to receive(:load).with(make_absolute("/a/foo/file.rb"), any_args) + expect(Kernel).to receive(:load).with(path, any_args) @autoload.loadall(env) end From ca7fac811fbd905ca5e61db79b48b2be21cf2188 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Fri, 17 Sep 2021 15:12:07 -0700 Subject: [PATCH 2/2] (PUP-11184) Return relative paths contained within the directory Previously, puppet failed to load if the current working directory was a short Windows path. This was due to trying to require `Puppet::Util::Uniquefile` using its long and short path. This regression was introduced with the move to require_relative. The `Autoload.files_in_dir` method is expected to return relative paths that are contained within the `dir` argument. For example, returning an array containing `puppet/provider/exec/posix`. However, if the `dir` argument contained a short Windows path and the short path wasn't the last directory component, then the `files_in_dir` method returned a relative path of the form, because `Dir.glob` always returns long paths: ../../../../../../../Program Files/Puppet Labs/Puppet/puppet/lib/ruby/vendor_ruby/puppet/provider/exec/posix Later the autoloader joined `Puppet[:libdir]` and the relative path above, resulting in a non-canonical path: C:/ProgramData/PuppetLabs/puppet/cache/lib/../../../../../../../Program Files/Puppet Labs/Puppet/puppet/lib/ruby/vendor_ruby/puppet/provider/exec/posix.rb The autoloader then called `Kernel.load` with the long path: C:/Program Files/Puppet Labs/Puppet/puppet/lib/ruby/vendor_ruby/puppet/provider/exec/posix.rb Which eventually tried to require relatively uniquefile using a long path. However, that would fail because it had already been required using its 8.3 path: irb(main):002:0> $LOADED_FEATURES.grep(/\/uniquefile/) => ["C:/PROGRA~1/PUPPET~1/Puppet/puppet/lib/ruby/vendor_ruby/puppet/file_system/uniquefile.rb"] This issue wasn't an issue prior to require_relative, because we called `require 'puppet/util/uniquefile'` and ruby knew it had already required that file. This commit modifies `files_in_dir` to convert the base `dir` to a long path, so that `dir` is always a prefix for the globbed child. --- lib/puppet/file_system.rb | 3 ++- lib/puppet/util/autoload.rb | 2 +- spec/unit/util/autoload_spec.rb | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/puppet/file_system.rb b/lib/puppet/file_system.rb index b3676ee7aca..0600aafdcbb 100644 --- a/lib/puppet/file_system.rb +++ b/lib/puppet/file_system.rb @@ -345,7 +345,8 @@ def self.pathname(path) # value ~ will be expanded to something like /Users/Foo # # This method exists primarlily to resolve a Ruby deficiency where - # File.expand_path doesn't handle ~ in each segment on a Windows path + # File.expand_path doesn't convert short paths to long paths, which is + # important when resolving the path to load. # # @param path [Object] a path handle produced by {#pathname} # @return [String] a string representation of the path diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb index e032c1dab63..800a96cfa3f 100644 --- a/lib/puppet/util/autoload.rb +++ b/lib/puppet/util/autoload.rb @@ -117,7 +117,7 @@ def files_to_load(path, env) # @api private def files_in_dir(dir, path) - dir = Pathname.new(File.expand_path(dir)) + dir = Pathname.new(Puppet::FileSystem.expand_path(dir)) Dir.glob(File.join(dir, path, "*.rb")).collect do |file| Pathname.new(file).relative_path_from(dir).to_s end diff --git a/spec/unit/util/autoload_spec.rb b/spec/unit/util/autoload_spec.rb index be3e8a79b1f..803e195cbd0 100644 --- a/spec/unit/util/autoload_spec.rb +++ b/spec/unit/util/autoload_spec.rb @@ -201,6 +201,24 @@ def with_libdir(libdir) @autoload.loadall(env) end + + it "autoloads from a directory whose ancestor is Windows 8.3", if: Puppet::Util::Platform.windows? do + # File.expand_path will expand ~ in the last directory component only(!) + # so create an ancestor directory with a long path + dir = File.join(tmpdir('longpath'), 'short') + path = File.join(dir, @autoload.path, 'file.rb') + + FileUtils.mkdir_p(File.dirname(path)) + FileUtils.touch(path) + + dir83 = File.join(File.dirname(basedir), 'longpa~1', 'short') + path83 = File.join(dir83, @autoload.path, 'file.rb') + + allow(@autoload.class).to receive(:search_directories).and_return([dir83]) + expect(Kernel).to receive(:load).with(path83, any_args) + + @autoload.loadall(env) + end end describe "when reloading files" do