(PUP-7130) Unify ruby Integer types, ENV['HOME'] handling and HTTP time #5653

thallgren · 2017-02-21T23:12:03Z

This PR changes three things to make the Puppet code base execute without problems using a Ruby 2.4.0 runtime.

Changes all Fixnum references to Integer.
Fixes an ambiguous time representation in HttpMetaData uses UTC.
Conditions some tests that sets ENV['HOME'] to nil (no-op on 2.4).

puppetcla · 2017-02-22T01:00:22Z

CLA signed by all contributors.

Iristyle · 2017-02-22T19:12:45Z

spec/unit/util/run_mode_spec.rb

@@ -2,6 +2,13 @@
 require 'spec_helper'

 describe Puppet::Util::RunMode do
+
+  # Discriminator for tests that attempts to unset HOME since that, for reasons currently unknown,


I'm not seeing a problem (with core Ruby ENV), at least on OSX / Ruby 2.4.0

irb(main):001:0> RUBY_VERSION => "2.4.0" irb(main):002:0> ENV['HOME'] => "/Users/Iristyle" irb(main):003:0> ENV['HOME'] = nil => nil irb(main):004:0> ENV['HOME'] => nil irb(main):009:0> ENV.keys.include?('HOME') => false

With our helpers:

[1] pry(main)> require 'puppet' => true [2] pry(main)> Puppet::Util.get_env('HOME') => "/Users/Iristyle" [3] pry(main)> Puppet::Util.set_env('HOME') => nil [4] pry(main)> Puppet::Util.get_env('HOME') => nil [5] pry(main)> RUBY_VERSION => "2.4.0" [6] pry(main)> Puppet.version => "5.0.0"

Were there particular platforms you saw having a problem with unsetting HOME @thallgren ?

The tests you do do not reveal the problem. Try:

File.expand_path('~/tmp')

and you will discover that it expands the path even when you have set the ENV['HOME'] to nil. Prior to 2.4.0, you'd get an exception and that exception is what the disabled tests rely on.

I would have preferred we understand / solve this problem rather than papering over the test failures with exclusions, as some Puppet code may need to change to accommodate the new behavior.

FWIW - it looks like the change was made in ruby/ruby@6b88dd2

The associated Ruby ticket is https://bugs.ruby-lang.org/issues/12695

@Iristyle, Great job finding the actual issue. I tried but didn't succeed. I agree that it's a good thing to understand the problem. My intention was not to paper over it, but rather that the current tests could be made into acceptance tests instead. I suggest that in the commit comment.

hlindberg

In addition to the individual comments - the changes realted to HOME are about something else than the changes related to Integer/Fixnum. It should at least be a separate commit - but preferably a separate ticket.

hlindberg · 2017-02-22T23:35:16Z

lib/puppet/provider/augeas/augeas.rb

@@ -203,7 +203,7 @@ def close_augeas

  def is_numeric?(s)
    case s
-    when Fixnum
+    when Integer,Float


This is not the same - augeas may not handle floats well.

Take a look at the regular expression used for the String a couple of lines down. It accepts floats. Seems rather odd that the string form would, but the numeric form would not. One of them should be adjusted. I chose to make that adjustment in a way that isn't more restrictive than before.

While I agree that it looks like a bug in the original, this is not a change that is required to make puppet run on Ruby 2.4.0. After this change, more values are allowed than earlier (floats in float form).

If also attempting to fix the potential bug, I would like that as a separate commit with rationale etc.

hlindberg · 2017-02-22T23:41:26Z

spec/unit/pops/types/type_calculator_spec.rb

-        expect(calculator.type(c).class).to eq(PIntegerType)
-      end
+    it 'should yield \'PIntegerType\' for Integer' do
+      expect(calculator.type(Integer).class).to eq(PIntegerType)


The original test needs to be retained for < Ruby 2.4 as we otherwise remove the assertion of how Fixnum/Bignum are mapped to an Integer PType. Make the test depend on Ruby version, and add new test for >= 2.4.

I added the tests, but without dependency on Ruby version. The Fixnum and Bignum still exists in Ruby 2.4 so they might be encountered.

hlindberg · 2017-02-22T23:43:13Z

spec/unit/provider/user/directoryservice_spec.rb

@@ -799,8 +799,8 @@ module Puppet::Util::Plist
    it "should accept a hash containing a PBKDF2 password hash, salt, and iterations value and return the correct iterations value" do
        expect(provider.class.get_salted_sha512_pbkdf2('iterations', pbkdf2_embedded_bplist_hash)).to eq(pbkdf2_iterations_value)
    end
-    it "should return a Fixnum value when looking up the PBKDF2 iterations value" do
-        expect(provider.class.get_salted_sha512_pbkdf2('iterations', pbkdf2_embedded_bplist_hash)).to be_a_kind_of(Fixnum)
+    it "should return a Integer value when looking up the PBKDF2 iterations value" do


"return an Integer"

thallgren · 2017-02-23T08:44:32Z

@hlindberg, regarding the change of HOME. I think the title of this ticket should change instead. To "Make it possible to run Puppet with Ruby 2.4". It makes no sense to me having separate tickets for what needs to be fixed since they wouldn't produce a functional code-base.

hlindberg · 2017-02-23T12:19:22Z

It would be much better if the changes were split up into several commits. You list them as "1,2,3" already - plus add one for the possible bug-fixed auegas.

This commit changes all Fixnum/Bignum references to Integer

Setting `ENV['HOME'] = nil` was used to provoke an exception when using a tilde character in `expand_path`. In 2.4.0, the expansion works OK even after assigning nil. NOTE! The exception can be provoked in an acceptance test if HOME set to empty before the Ruby process is started: ``` $ export HOME= $ irb 2.4.0 :001 > File.expand_path('~/tmp') ArgumentError: non-absolute home ```

In Ruby versions prior <2.4.0, the statement ``` x = DateTime.httpdate('Sat, 03 Feb 2001 04:05:06 GMT').to_time.to_s ``` produces a local time like "2001-02-03 05:05:06 +0100". In 2.4.0 the timezone is retained. This commit ensures that the time used for the http_metadata checksum is UTC and that the test expects UTC. That works, regardless of Ruby version.

- Now that RubyInstaller has produced an installer for Ruby 2.4.1 on Windows and AppVeyor has integrated it into their workflow per appveyor/ci#1350 we can move from 2.3 to 2.4 given Puppet 5 will be shipping against 2.4 - Update tests that behave differently under Ruby 2.4: * Ruby has changed how ~ resolves when no HOME, HOMEDRIVE or USERPROFILE has set. It no longer fails to resolve / causes an exception thanks to https://bugs.ruby-lang.org/issues/12695 puppetlabs#5653 was the first pass in f4f5ccb but was incomplete. * Ruby has changed the implementation of sort_by! so that items already in order will not be returned in the same order. For instance, compare the behaviors between Ruby 2.3 and 2.4: [[1.0, 'a'], [1.0, 'b'], [1.0, 'c']].sort_by! { |i| i[0] } Ruby 2.3: [[1.0, "a"], [1.0, "b"], [1.0, "c"]] Ruby 2.4: [[1.0, "b"], [1.0, "c"], [1.0, "a"]] [[1.0, 'c'], [1.0, 'b'], [1.0, 'a']].sort_by! { |i| i[0] } Ruby 2.3: [[1.0, "c"], [1.0, "b"], [1.0, "a"]] Ruby 2.4: [[1.0, "b"], [1.0, "a"], [1.0, "c"]]

Iristyle reviewed Feb 22, 2017

View reviewed changes

hlindberg suggested changes Feb 22, 2017

View reviewed changes

thallgren added 3 commits February 23, 2017 13:54

(PUP-7130) Unify ruby Integer types

7619782

This commit changes all Fixnum/Bignum references to Integer

thallgren force-pushed the issue/pup-7130/unify-ruby-integer-types branch from ed393e5 to d472bef Compare February 23, 2017 13:09

hlindberg approved these changes Feb 23, 2017

View reviewed changes

hlindberg merged commit 94202f8 into puppetlabs:master Feb 23, 2017

thallgren deleted the issue/pup-7130/unify-ruby-integer-types branch May 23, 2017 16:45

Iristyle mentioned this pull request Jun 14, 2017

(maint) Switch AppVeyor to Ruby 2.4 from 2.3 #5975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PUP-7130) Unify ruby Integer types, ENV['HOME'] handling and HTTP time #5653

(PUP-7130) Unify ruby Integer types, ENV['HOME'] handling and HTTP time #5653

thallgren commented Feb 21, 2017 •

edited

Loading

puppetcla commented Feb 22, 2017

Iristyle Feb 22, 2017

Iristyle Feb 22, 2017

thallgren Feb 23, 2017 •

edited

Loading

Iristyle Feb 23, 2017 •

edited

Loading

Iristyle Feb 23, 2017

thallgren Feb 23, 2017

hlindberg left a comment

hlindberg Feb 22, 2017

thallgren Feb 23, 2017 •

edited

Loading

hlindberg Feb 23, 2017

hlindberg Feb 23, 2017

hlindberg Feb 22, 2017

thallgren Feb 23, 2017 •

edited

Loading

hlindberg Feb 22, 2017

thallgren commented Feb 23, 2017

hlindberg commented Feb 23, 2017

(PUP-7130) Unify ruby Integer types, ENV['HOME'] handling and HTTP time #5653

(PUP-7130) Unify ruby Integer types, ENV['HOME'] handling and HTTP time #5653

Conversation

thallgren commented Feb 21, 2017 • edited Loading

puppetcla commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thallgren Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Iristyle Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hlindberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thallgren Feb 23, 2017 • edited Loading

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

thallgren Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thallgren commented Feb 23, 2017

hlindberg commented Feb 23, 2017

thallgren commented Feb 21, 2017 •

edited

Loading

thallgren Feb 23, 2017 •

edited

Loading

Iristyle Feb 23, 2017 •

edited

Loading

thallgren Feb 23, 2017 •

edited

Loading

thallgren Feb 23, 2017 •

edited

Loading