Skip to content

Commit

Permalink
(PUP-11184) Return relative paths contained within the directory
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
joshcooper committed Sep 27, 2021
1 parent e0602f7 commit ca7fac8
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 2 deletions.
3 changes: 2 additions & 1 deletion lib/puppet/file_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/puppet/util/autoload.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions spec/unit/util/autoload_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit ca7fac8

Please sign in to comment.