(MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode #72

Iristyle · 2015-02-19T19:41:53Z

No description provided.

Iristyle · 2015-02-23T16:05:32Z

A bit more work to do on this one, but if you want to take a first pass @ferventcoder

ferventcoder · 2015-02-23T17:41:25Z

Will take a look tomorrow when I'm back "in the office." :)

Iristyle · 2015-02-23T21:30:40Z

@cyberious feel free to take an early peek as well if you have a minute

cyberious · 2015-02-23T22:26:39Z

Gemfile

+end
+
+beaker_version = ENV['BEAKER_VERSION'] || '~> 2.2'
+group :acceptance do


Please use :system_tests as this is a pattern that the modules team has already adopted. https://github.com/puppetlabs/puppetlabs-concat/blob/master/Gemfile#L25

cyberious · 2015-02-24T00:29:30Z

@Iristyle yeah we have standardized on :system_tests as a group. We actually have this specified in our modulesync script defaults. https://github.com/puppetlabs/modulesync_configs/blob/master/config_defaults.yml

cyberious · 2015-02-24T00:45:55Z

Tests are now passing.

cyberious · 2015-02-25T21:17:31Z

Rakefile needed updating to get the spec running see #74

ferventcoder · 2015-02-27T19:25:57Z

lib/puppet/provider/registry_key/registry.rb

@@ -49,6 +51,8 @@ def values
    # Only try and get the values for this key if the key itself exists.
    if exists? then
      hive.open(subkey, Win32::Registry::KEY_READ | access) do |reg|
+        # TODO: tests for this since each_value triggers LOCALE conversion on Ruby 2.1.5
+        # require 'pry'; binding.pry


This must be part of the WIP.

Leaving a note here so you don't forget to remove these lines.

Iristyle · 2015-03-10T06:25:34Z

@ferventcoder / @joshcooper might want to take a gander at this... Now that I've flushed out tests, I'm pulling in a lot more code from Puppet than I originally planned on (reg_enum_value in particular drags in a lot of additional stuff).

I'm thinking now that it might be better to just write some small helpers that call in to Puppet on 4+ / Ruby 2.1.5 or preserve existing behavior by using Win32::Registry for backward compatibility (assuming new tests pass on 1.9.3 / 2.0.0)

Iristyle · 2015-03-10T14:44:28Z

spec/unit/puppet/provider/registry_value_spec.rb

+    # https://github.com/ruby/ruby/blob/v2_1_5/ext/win32/lib/win32/registry.rb#L323-L329
+    # therefore, use our own code instead of hklm.delete_value
+
+    # TODO: don't like registry_value tests depending on registry_key type


So we have 3 choices here:

Go a little heavier weight / include ref to a separate class and use the registry_key provider

Use the Win32 API directly (or from the provider base class)

Use Win32::Registry to call delete, which we know fails on Ruby 2.1.5

I went with option 1 for now, but I'm leaning toward reimplementing as option 2.

For 9db94e2, this is the only easy way of doing this. In a subsequent commit this could be rewritten to use the methods on ProviderBase instead, but not sure that's worth it.

ferventcoder · 2015-03-16T21:59:06Z

lib/puppet_x/puppetlabs/registry/provider_base.rb

+      end
+    else
+      # TODO: expectations around never calling this are not working
+      require 'pry'; binding.pry


So what you are saying is, do not merge? ;)

Need to remove binding.pry

ferventcoder · 2015-03-20T17:41:46Z

Ruby 2.1.5-x64 /4.x - good
Ruby 1.9.3 / 3.7.3 - flaky - I'm going to dive in a bit

ferventcoder · 2015-03-20T20:21:20Z

lib/puppet_x/puppetlabs/registry/provider_base.rb

+  def from_string_to_wide_string(str, &block)
+    self.class.from_string_to_wide_string(str, &block)
+  end
+


Assuming this was reintroduced for compatibility with older puppet versions :)

ferventcoder · 2015-03-20T20:44:45Z

So this technically looks okay, the TODOs should be cleaned up - and the Puppet 3.7.3 / Ruby 1.9.3 combination is failing on this for me:

  1) Puppet::Type::Registry_key::ProviderRegistry#purge_values does not use Rubys each_value, which unnecessarily string encodes
     Failure/Error: reg_key.provider.destroy
     RuntimeError:
       Failed to delete registry key: Registry_key[hklm\SOFTWARE\Puppet Labs\PuppetRegProviderTest\Unicode-1fbfbba7-5ec3-4dfc-a48f-5cf2b91e23ac](provider=registry)
     # ./lib/puppet/provider/registry_key/registry.rb:45:in `block in destroy'
     # ./lib/puppet_x/puppetlabs/registry/provider_base.rb:61:in `block in from_string_to_wide_string'
     # ./lib/puppet_x/puppetlabs/registry/provider_base.rb:58:in `initialize'
     # ./lib/puppet_x/puppetlabs/registry/provider_base.rb:58:in `new'
     # ./lib/puppet_x/puppetlabs/registry/provider_base.rb:58:in `from_string_to_wide_string'
     # ./lib/puppet_x/puppetlabs/registry/provider_base.rb:139:in `from_string_to_wide_string'
     # ./lib/puppet/provider/registry_key/registry.rb:42:in `destroy'
     # ./spec/unit/puppet/provider/registry_key_spec.rb:70:in `block (3 levels) in <top (required)>'

It's not a fluke - it's not deleting the registry key that it creates. It based in the after block when it is trying ti clean up. Well it could be a fluke. As an easy fix...

ferventcoder · 2015-03-20T20:54:18Z

I think something may be broken and the custom type is not using 64bit keys automatically anymore - normally a 32: prefix should be required to get to the 32 bit keys, but I don't see that in the test.

From https://github.com/puppetlabs/puppetlabs-registry/#custom-type-registry_key:
"If Puppet is running on a 64-bit system, the 32-bit registry key can be explicitly managed using a prefix. For example: '32:HKLM\Software'."

ferventcoder · 2015-03-20T21:01:30Z

Actually after researching, I think that there is a test now actually surfaced a pre-existing bug. After looking back through the changes several times I don't see anything that would have introduced a regression.

cyberious · 2015-03-25T16:07:33Z

Can't run through jenkins right now, please rebase from master as it has a fix in Rakefile when beaker is not loaded.

ferventcoder · 2015-03-25T16:49:29Z

@cyberious This is still ticketed as do not merge so we are good.

- Beaker 2.4 will actually be required to test AIO packages

- Without Beaker in isolation, there is no way to actually install the bundle for this module on Windows, since Beaker contains gems with native extensions that don't compile on Ruby 2.1.5 and Windows (unf_ext in particular). With this change, we can now install the gems to be used for local integration testing with: bundle install --path .bundle/gems --without=system_tests

Iristyle · 2015-03-25T17:08:49Z

Ok @ferventcoder / @cyberious - final validation - run this through CI and I think we're good to merge.

- New tests to demonstrate Ruby 2.1.5 failures for: - Win32::Registry#delete_value which always fails based on passing wide character UTF-16LE strings to an ANSI Api (used in the registry_value provider destroy method) Ruby bug filed at https://bugs.ruby-lang.org/issues/10820 - Win32::Registry#each_key enumeration which initiates unnecessary locale conversion (used in registry_key provider .values method) - Note that these tests were crafted specifically to introduce failures of Ruby 2.1.5 code, and do not introduce failures when run against Ruby 2.0.0. The non-ASCII test is guarded to only run in Ruby 2.1 as it doesn't affect Ruby 2.0 or Ruby 2.2 and can write corrupt data to the registry under Ruby 1.9.3 due to Ruby using Windows ANSI APIs. - Ensure that when creating registry keys with Ruby to specify KEY_WOW64_64KEY to control the location of the key. When using the module provider code, this is the default specified for deletion. When these don't match, the key delete attempt may fail based on bitness of the Ruby process.

- Ensure that under Ruby 2.1.x that problematic method calls to Win32::Registry are not made. This was made in a separate commit because unfortunately Mocha's any_instance will actually hide other errors (which are visible in the prior commit). Once code has been changed to alter code paths under other Ruby versions, the guarding will be changed accordingly.

- Switch to using RegQueryValueExW and RegDeleteKeyExW instead of their ANSI counterparts - Since Ruby 2.1.5 is broken with respec to calling delete_key and delete_value, add our own code for RegDeleteValueW to be used. Ensure that all deletions of registry keys go through the API rather than Ruby. - Implement a helper method for converting Ruby strings to UTF-16LE strings and for creating an unmanaged LCPWSTR pointer for use in FFI calls given a Ruby string. This code appears in Puppet itself, but for compatibility reasons, the Registry module cannot rely on the Puppet version that defines it. - Implement an each_value inside of the provider base code that either uses Rubys Win32::Registry#each_value in Ruby 2.0 and lower, or uses the Puppet 4 implementation if available. This provides existing behavior for older versions of Puppet / Ruby which don't have LOCALE conversion issues, and the correct behavior for Puppet 4. This was implemented this way to avoid dragging in a lot of duplicated FFI code from Puppet 4 introduced via: puppetlabs/puppet@b46ede7

- Previously, the purge_values tests were only for Ruby 2.1, where we know that there are issues in Win32::Registry. - Add tests that demonstrate that other versions of Ruby / Puppet work as well.

ferventcoder · 2015-03-25T22:38:19Z

I'm plus 1 to merge. Now if Travis CI will give a green light

Iristyle · 2015-03-25T22:55:48Z

Travis Y U So Slow?

ferventcoder · 2015-03-25T23:08:48Z

...

ferventcoder · 2015-03-25T23:10:23Z

Its like the energizer bunny in reverse

…2-compatibility (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch 7 times, most recently from 7e2e40f to 6406285 Compare February 19, 2015 19:53

Iristyle changed the title ~~(DO NOT MERGE) (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / UTF-8~~ (DO NOT MERGE) (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode Feb 19, 2015

cyberious reviewed Feb 23, 2015
View reviewed changes

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from 6406285 to eb42c1b Compare February 24, 2015 00:37

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from eb42c1b to 1df77dd Compare February 24, 2015 01:16

ferventcoder reviewed Feb 27, 2015
View reviewed changes

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch 2 times, most recently from b3bd0d2 to b1ebc86 Compare March 10, 2015 06:18

Iristyle reviewed Mar 10, 2015
View reviewed changes

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch 4 times, most recently from aa9108f to b3821f9 Compare March 10, 2015 23:56

ferventcoder reviewed Mar 16, 2015
View reviewed changes

ferventcoder reviewed Mar 20, 2015
View reviewed changes

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from db1d2de to 0e5eaa7 Compare March 25, 2015 15:30

Iristyle added 3 commits March 25, 2015 10:06

(maint) update Beaker dependency to ~> 2.2

104816b

- Beaker 2.4 will actually be required to test AIO packages

(maint) remove unused serverspec gem dependency

40e6c6b

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from 0e5eaa7 to 38a8c47 Compare March 25, 2015 17:06

Iristyle changed the title ~~(DO NOT MERGE) (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode~~ (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode Mar 25, 2015

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from 76d2626 to 14f4d58 Compare March 25, 2015 17:20

Iristyle and others added 4 commits March 25, 2015 13:56

(MODULES-1723) Additional tests for purge_values

868e77e

- Previously, the purge_values tests were only for Ruby 2.1, where we know that there are issues in Win32::Registry. - Add tests that demonstrate that other versions of Ruby / Puppet work as well.

Iristyle force-pushed the ticket/master/MODULES-1723-ruby-2-compatibility branch from 14f4d58 to 868e77e Compare March 25, 2015 20:59

cyberious added a commit that referenced this pull request Mar 25, 2015

Merge pull request #72 from Iristyle/ticket/master/MODULES-1723-ruby-…

d85701f

…2-compatibility (MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode

cyberious merged commit d85701f into puppetlabs:master Mar 25, 2015

jordanbreen28 added the maintenance label Jun 20, 2023

(MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode #72

(MODULES-1723) Update Registry module for Puppet 4 / Ruby 2.1.5 / Unicode #72

Uh oh!

Conversation

Iristyle commented Feb 19, 2015

Uh oh!

Iristyle commented Feb 23, 2015

Uh oh!

ferventcoder commented Feb 23, 2015

Uh oh!

Iristyle commented Feb 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyberious commented Feb 24, 2015

Uh oh!

cyberious commented Feb 24, 2015

Uh oh!

cyberious commented Feb 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Iristyle commented Mar 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferventcoder commented Mar 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ferventcoder commented Mar 20, 2015

Uh oh!

ferventcoder commented Mar 20, 2015

Uh oh!

ferventcoder commented Mar 20, 2015

Uh oh!

cyberious commented Mar 25, 2015

Uh oh!

ferventcoder commented Mar 25, 2015

Uh oh!

Iristyle commented Mar 25, 2015

Uh oh!

ferventcoder commented Mar 25, 2015

Uh oh!

Iristyle commented Mar 25, 2015

Uh oh!

ferventcoder commented Mar 25, 2015

Uh oh!

ferventcoder commented Mar 25, 2015

Uh oh!

Uh oh!