(maint) Expand Windows 8.3 paths in templatedir #82

james-stocks · 2017-06-15T14:44:18Z

PDK::Util.make_tmpdir_name might return a short (8.3) path on Windows.
This commits adds a canonical path method to allow assurance of using
the full path.

This fixes module generation on Windows; previously PDK was failing to
match C:\Users\Administrator and C:\Users\ADMIN~1

DavidS · 2017-06-15T14:48:33Z

Poking around a bit in the win32 gems, I found that https://github.com/djberg96/win32-file has long_path/short_path methods that could make this nicer.

scotje · 2017-06-15T16:07:26Z

I have a branch where I was working on this with tragiccode in slack yesterday, but the win32 gem solution may be more robust, I was using relative paths: https://github.com/puppetlabs/pdk/compare/debug_windows_path

DavidS · 2017-06-15T16:09:55Z

according to the docs relative_path_from does not go to the file system, we assumed therefore that it won't work well with 8.3 confusion going on here.

scotje · 2017-06-15T16:12:21Z

It actually seems to work fine, it just does ../ all the way back to the place before the 8.3 style path and then rebuilds from there. Like I said, the win32 stuff will probably be more elegant. :)

DavidS · 2017-06-16T16:30:53Z

The errors in this PR seem to be connected to pdk new module foo --skip-interview silently not correctly rendering a module.

glennsarti · 2017-06-16T16:42:38Z

As an FYI, we actively try to remove any of the win32-* gems from Puppet due to a variety of reasons. Are you absolutely sure you need to bring in this gem to something as simple as 8.3 name expansion?

james-stocks · 2017-06-19T14:57:50Z

@glennsarti @jpogran This PR has been updated to use fiddle to access GetLongPathNameA within kernel32.dll, instead of using win32-gem

The PR still needs 1-2 test fixes plus a tidy-up; but a review specifically on this change would be appreciated, cheers

glennsarti · 2017-06-19T15:27:54Z

lib/pdk/util.rb

+          buffer    = nil
+          loop do
+            buffer = ' ' * bufferlen
+            len    = GetLongPathNameA(short_path.to_s, buffer, buffer.size)


There should be a check here if GetLongPathName fails (RC = 0)

https://msdn.microsoft.com/en-us/library/windows/desktop/aa364980(v=vs.85).aspx

You need to use the W (GetLongPathNameW) version of the API plus be able to handle double byte characters. Otherwise it'll break for non-English letters e.g. Jöhn

https://github.com/djberg96/win32-file/blob/ffi/lib/win32/file.rb#L189-L200

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/file.rb#L267-L279

glennsarti

Needs changes for non-english characters

glennsarti · 2017-06-19T15:29:04Z

lib/pdk/util.rb

+            end
+            break
+          end
+          buffer.rstrip.chomp("\0")


Also, you need to free the buffer after use otherwise you'll leak memory

glennsarti · 2017-06-19T15:31:11Z

lib/pdk/util.rb

+            len    = GetLongPathNameA(short_path.to_s, buffer, buffer.size)
+            # In the case of the path name being exceptionally long; resize the buffer
+            # to the actual size, then re-get it
+            if bufferlen < len


The correct way to get the buffer length is to call GetLongPathName with a buffer size of zero, which will then return the required buffer size. Then you can allocate the buffer and call with the correct size.

glennsarti · 2017-06-19T15:32:56Z

lib/pdk/util.rb

+      if Gem.win_platform?
+        extend Fiddle::Importer
+        dlload 'kernel32.dll'
+        extern('int GetLongPathNameA(char*, char*, int)', :stdcall)


Just a note - The actual return type is uint32 but the ruby equivalent is int

DavidS · 2017-06-20T13:57:44Z

pdk.gemspec

@@ -27,4 +27,6 @@ Gem::Specification.new do |spec|

  # Used in the pdk-module-template
  spec.add_runtime_dependency 'deep_merge', '~> 1.1'
+
+  spec.add_runtime_dependency 'win32-file', '~> 0.8'


can we remove this now?

james-stocks · 2017-06-20T14:44:37Z

@glennsarti This has been updated to re-use Puppet util code so we should be supporting wide strings now

DavidS · 2017-06-20T14:44:58Z

lib/pdk/cli/exec.rb

@@ -135,7 +135,8 @@ def run_process!
          begin
            @process.start
          rescue ChildProcess::LaunchError => e
-            raise PDK::CLI::FatalError, _("Failed to execute '%{command}': %{message}") % { command: argv.join(' '), message: e.message }
+            raise PDK::CLI::FatalError, _('Failed to execute process: %{message}') % { message: e.message } unless @process.respond_to?(:argv)
+            raise PDK::CLI::FatalError, _("Failed to execute '%{command}': %{message}") % { command: @process.argv.join(' '), message: e.message }


double raise

First raise is conditional. Rubocop preferred this raise x unless y form. I'll see if I can re-arrange it so that it is easily readable and passes rubocop.

ugh, yeah, the GuardClause cop is a little bit hit and miss. Maybe the better alternative would be to

message = if @process.respond_to?(:argv) # ... raise PDK::CLI::FatalError, message

PDK::Util.make_tmpdir_name might return a short (8.3) path on Windows. This commits adds a canonical path method to allow assurance of using the full path. This fixes module generation on Windows; previously PDK was failing to match C:\Users\Administrator and C:\Users\ADMIN~1

@process

When run_process! fails, an unhandled exception could occur when forming the PDK exception message. This commit handles the case of run_process! failing and @process not having an argv member.

Where the bundle management acceptance tests use mv to backup and restore Gemfile, cascading failures can occur if a pending test fails and leaves no Gemfile on disk.

Setting of GEM_PATH needs to use PATH_SEPARATOR for OS compatibility. Also needs to allow for GEM_PATH to not be set in advance.

The pending test for bundle management fails on appveyor because it actually passes due to the presence of devkit. This test should not be pending - there should be a devkit installed. The test can be manually ignored on workstations that don't/can't have devkit.

glennsarti · 2017-06-20T15:15:34Z

lib/puppet/util/windows/api_types.rb

+require 'ffi'
+require 'puppet/util/windows/string'
+
+module Puppet::Util::Windows::APITypes


Is there a plan to import the Puppet Gem into the PDK at some point? If so, this looks like a namespacing issue.

Since the PDK needs to be able to deal with multiple versions of puppet, the main binary will never reference the puppet gem.

For the record: I would prefer that utility code to live in a independent gem that others can use without having to depend on a full puppet package. But this is not relevant to this PR. In effect, we're treating this like a vendoring.

DavidS · 2017-06-20T15:16:22Z

Looks good to me. Since this incorporates the suggestions from @glennsarti and @Iristyle, I assume we'll be fine from that side too.

glennsarti

Do we need spec tests for the 8.3 naming then?

DavidS · 2017-06-20T16:40:48Z

https://github.com/puppetlabs/pdk/pull/82/files#diff-f1961fa3f901b19ee585c1962e910d5eR56 and friends is the thing we need to test. We've started down this rabbit hole, because in some situations the regex at https://github.com/puppetlabs/pdk/blob/master/lib/pdk/module/templatedir.rb#L198 did not match up due to different encodings of the same path. It should possible to finagle in a test exercising this part. I'll look into it tomorrow.

DavidS · 2017-06-21T13:08:41Z

Created SDK-286 to track the testing effort.

james-stocks force-pushed the windows_paths branch from 7c4085b to f9072d7 Compare June 15, 2017 15:57

james-stocks force-pushed the windows_paths branch 3 times, most recently from 6843126 to 192aafa Compare June 16, 2017 15:35

james-stocks force-pushed the windows_paths branch 2 times, most recently from 10c161c to c32bf5a Compare June 19, 2017 14:38

james-stocks force-pushed the windows_paths branch from c32bf5a to d9cae03 Compare June 19, 2017 15:15

glennsarti reviewed Jun 19, 2017

View reviewed changes

glennsarti suggested changes Jun 19, 2017

View reviewed changes

DavidS reviewed Jun 20, 2017

View reviewed changes

james-stocks force-pushed the windows_paths branch 3 times, most recently from 28470ce to a50632d Compare June 20, 2017 14:32

DavidS reviewed Jun 20, 2017

View reviewed changes

james-stocks force-pushed the windows_paths branch from a50632d to 4c15a1f Compare June 20, 2017 15:11

James Stocks added 5 commits June 20, 2017 15:13

(maint) Improve run_process! exception handling

fbd81c0

When run_process! fails, an unhandled exception could occur when forming the PDK exception message. This commit handles the case of run_process! failing and @process not having an argv member.

(maint) bundle acceptance spec improvement

2529936

Where the bundle management acceptance tests use mv to backup and restore Gemfile, cascading failures can occur if a pending test fails and leaves no Gemfile on disk.

(maint) Fix setting of GEM_PATH in acceptance

cfda061

Setting of GEM_PATH needs to use PATH_SEPARATOR for OS compatibility. Also needs to allow for GEM_PATH to not be set in advance.

james-stocks force-pushed the windows_paths branch from 4c15a1f to 6a42e8f Compare June 20, 2017 15:13

glennsarti reviewed Jun 20, 2017

View reviewed changes

DavidS approved these changes Jun 20, 2017

View reviewed changes

glennsarti approved these changes Jun 20, 2017

View reviewed changes

DavidS merged commit d711690 into puppetlabs:master Jun 20, 2017

DavidS deleted the windows_paths branch July 28, 2017 15:18

chelnak added the maintenance Internal maintenance work that shouldn't appear in the changelog label Jan 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(maint) Expand Windows 8.3 paths in templatedir #82

(maint) Expand Windows 8.3 paths in templatedir #82

james-stocks commented Jun 15, 2017 •

edited

Loading

DavidS commented Jun 15, 2017

scotje commented Jun 15, 2017

DavidS commented Jun 15, 2017

scotje commented Jun 15, 2017

DavidS commented Jun 16, 2017

glennsarti commented Jun 16, 2017

james-stocks commented Jun 19, 2017

glennsarti Jun 19, 2017

glennsarti Jun 19, 2017

glennsarti left a comment

glennsarti Jun 19, 2017

glennsarti Jun 19, 2017

glennsarti Jun 19, 2017 •

edited

Loading

DavidS Jun 20, 2017

james-stocks commented Jun 20, 2017

DavidS Jun 20, 2017

james-stocks Jun 20, 2017

DavidS Jun 20, 2017

glennsarti Jun 20, 2017

DavidS Jun 20, 2017

glennsarti Jun 20, 2017

DavidS commented Jun 20, 2017

glennsarti left a comment

DavidS commented Jun 20, 2017

DavidS commented Jun 21, 2017

(maint) Expand Windows 8.3 paths in templatedir #82

(maint) Expand Windows 8.3 paths in templatedir #82

Conversation

james-stocks commented Jun 15, 2017 • edited Loading

DavidS commented Jun 15, 2017

scotje commented Jun 15, 2017

DavidS commented Jun 15, 2017

scotje commented Jun 15, 2017

DavidS commented Jun 16, 2017

glennsarti commented Jun 16, 2017

james-stocks commented Jun 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennsarti Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidS commented Jun 20, 2017

glennsarti left a comment

Choose a reason for hiding this comment

DavidS commented Jun 20, 2017

DavidS commented Jun 21, 2017

james-stocks commented Jun 15, 2017 •

edited

Loading

glennsarti Jun 19, 2017 •

edited

Loading