Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Real, complete Ruby 1.9 support - at least in the tests #575

Merged
merged 55 commits into from Mar 30, 2012

Conversation

Projects
None yet
6 participants
Contributor

slippycheeze commented Mar 12, 2012

Ruby 1.9 was nominally a supported platform, but we didn't even have all the tests passing. It only "worked" in the sense that the code didn't obviously blow up for some people using it, and the tests "passed" in the sense that they worked fine, provided you didn't run any of the tests that revealed bugs.

This set of changes fixes that by identifying the root cause of each failure and fixing the genuine bugs in Puppet that led to test failures. (It also fixes a bunch of places where test stubs were making all sorts of strange assumptions, like "foo"[:bar] => nil, and other 1.8 oddities.

Key changes are:

  • disable RDoc support on versions we can't integrate with.
  • catch 1.9 as well as 1.8 require failure messages.
  • replace complex locking around files with simple and obvious atomic replace.
  • handle the new Psych YAML parser in Ruby 1.9.
  • fix ZAML to correctly emit binary and Unicode data.
  • handle changes to instance_variables, methods, etc.
  • fix the monkey-patch version of lines to work like, well, lines.
  • fix the monkey-patch version of binwrite to work like, well, binwrite.
  • partially fix PSON vs non-Unicode content on 1.9.
Contributor

slippycheeze commented Mar 12, 2012

Oh, darn. Turns out I had missed the 1.9.2 run on my final sweep, and there are a couple of lingering failure that are specific to 1.9.2 - all work on 1.8.5, 1.8.7, and 1.9.3. Will push an update shortly.

@nicklewis nicklewis commented on an outdated diff Mar 12, 2012

lib/puppet/util/monkey_patches/lines.rb
@@ -0,0 +1,19 @@
+require 'puppet/util/monkey_patches'
+module Puppet::Util::MonkeyPatches::Lines
+ def lines(separator = $/)
+ if block_given?
+ self.each_line(separator) {|line| yield line }
+ return self
+ else
+ # 1.8.5 makes this ugly by demanding a block
+ lines = []
+ self.each_line(separator) {|line| lines << line }
@nicklewis

nicklewis Mar 12, 2012

Member

enum_for(:each_line) should give the behavior we want on 1.8.5 of returning an Enumerator. You may need to require 'enumerator' first.

Contributor

slippycheeze commented Mar 12, 2012

This change folds in Nick's enum_for suggestion, which I thought was unavailable on 1.8.5. Gotta love that documentation level. 1.9.2 still doesn't work.

Contributor

pcarlisle commented Mar 12, 2012

This is also #6830

@pcarlisle pcarlisle commented on an outdated diff Mar 14, 2012

spec/unit/indirector/report/yaml_spec.rb
@@ -22,16 +22,7 @@
Puppet::Transaction::Report::Yaml.name.should == :yaml
end
- it "should inconditionnally save/load from the --lastrunreport setting", :'fails_on_ruby_1.9.2' => true do
- indirection = stub 'indirection', :name => :my_yaml, :register_terminus_type => nil
- Puppet::Indirector::Indirection.stubs(:instance).with(:my_yaml).returns(indirection)
- store_class = Class.new(Puppet::Transaction::Report::Yaml) do
- def self.to_s
- "MyYaml::MyType"
- end
- end
- store = store_class.new
-
- store.path(:me).should == Puppet[:lastrunreport]
+ it "should inconditionnally save/load from the --lastrunreport setting" do

@pcarlisle pcarlisle commented on the diff Mar 14, 2012

lib/puppet/util/storage.rb
return
end
Puppet::Util.benchmark(:debug, "Loaded state") do
- Puppet::Util::FileLocking.readlock(Puppet[:statefile]) do |file|
+ begin
+ @@state = YAML.load(::File.read(filename))
@pcarlisle

pcarlisle Mar 14, 2012

Contributor

should these be class variables?

@slippycheeze

slippycheeze Mar 14, 2012

Contributor

That code is not awesome, but it shouldn't have a problem with how those class variables are used. Refactoring it is interesting but not necessary to tie to the 1.9 series support.

@pcarlisle pcarlisle and 1 other commented on an outdated diff Mar 14, 2012

spec/unit/indirector/terminus_spec.rb
- @abstract_terminus = Class.new(Puppet::Indirector::Terminus) do
- def self.to_s
- "Abstract"
+ it "should register the terminus class with the terminus base class" do
+ Puppet::Indirector::Terminus.expects(:register_terminus_class).with do |type|
+ type.indirection_name == :abstract_concept and type.name == :intelect
@pcarlisle

pcarlisle Mar 14, 2012

Contributor

intellect?

@slippycheeze

slippycheeze Mar 14, 2012

Contributor

fixed in next push

@pcarlisle pcarlisle and 1 other commented on an outdated diff Mar 14, 2012

lib/puppet/indirector/yaml.rb
begin
return from_yaml(yaml)
- rescue => detail
+ rescue Exception => detail
@pcarlisle

pcarlisle Mar 14, 2012

Contributor

It seems like this should probably rescue StandardError and Psych::SyntaxError specifically

@slippycheeze

slippycheeze Mar 14, 2012

Contributor

Done.

@pcarlisle pcarlisle commented on the diff Mar 14, 2012

spec/unit/simple_graph_spec.rb
@@ -776,7 +790,7 @@ def graph_to_yaml(graph, which_format)
# Test serialization of graph to YAML.
[:old, :new].each do |which_format|
all_test_graphs.each do |graph_to_test|
- it "should be able to serialize #{graph_to_test} to YAML (#{which_format} format)" do
+ it "should be able to serialize #{graph_to_test} to YAML (#{which_format} format)", :if => (RUBY_VERSION[0,3] == '1.8' or YAML::ENGINE.syck?) do
@pcarlisle

pcarlisle Mar 14, 2012

Contributor

It seems like you've written 'fails_on_ruby_1.9.2' here a slightly different way. If we can't really test the psych stuff yet should we just be setting our yaml engine to syck for now?

@slippycheeze

slippycheeze Mar 14, 2012

Contributor

In the sense that this test - which is extremely intimate with the parse tree from the YAML engine - can't run on the default 1.9 YAML engine, yes.

I don't think we should force the older engine, though. This is just one test on the output structure, and is not critical to the success of the operation. It really belongs more in the world of integration or acceptance tests than unit tests, I think.

The Psych engine does get a whole bunch of other testing around this work, though. So, I think this is the right decision: a small risk if we don't test with Syck at all, but since we do...

Contributor

pcarlisle commented Mar 14, 2012

rake unit fails on 1.8.7

Contributor

pcarlisle commented Mar 14, 2012

  1) Error:
test_autogen(TestGroupProvider):
NameError: undefined local variable or method `resource' for Puppet::Type::Group::ProviderDirectoryservice:Class
    ../lib/puppet/provider/nameservice.rb:139:in `autogen_id'
    ../lib/puppet/provider/nameservice.rb:118:in `autogen'
    ./ral/providers/group.rb:239:in `test_autogen'
    /Users/patrick/.rvm/gems/ruby-1.8.7-p357@puppet/gems/mocha-0.10.4/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `__send__'
    /Users/patrick/.rvm/gems/ruby-1.8.7-p357@puppet/gems/mocha-0.10.4/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `run'

  2) Error:
test_autogen(TestUserProvider):
NameError: undefined local variable or method `resource' for Puppet::Type::User::ProviderDirectoryservice:Class
    ../lib/puppet/provider/nameservice.rb:139:in `autogen_id'
    ../lib/puppet/provider/nameservice.rb:118:in `autogen'
    ./ral/providers/user.rb:565:in `test_autogen'
    /Users/patrick/.rvm/gems/ruby-1.8.7-p357@puppet/gems/mocha-0.10.4/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `__send__'
    /Users/patrick/.rvm/gems/ruby-1.8.7-p357@puppet/gems/mocha-0.10.4/lib/mocha/integration/test_unit/ruby_version_186_and_above.rb:22:in `run'
Contributor

pcarlisle commented Mar 14, 2012

rake unit on 1.9.3 => 24 failures, 24 errors

Contributor

slippycheeze commented Mar 14, 2012

Gah, full of bugs. Guess which suite of tests I entirely forgot. Thanks for catching that, will work on getting those fixed up too.

Contributor

slippycheeze commented Mar 16, 2012

Awesome. So, fixed 1.8.7 and 1.8.5. Fixing 1.9.3 vs unit tests is going to be harder, because the structure of Test::Unit has changed, and so our hacks on the internal API redefining test cases no longer apply....

Contributor

slippycheeze commented Mar 21, 2012

All-righty. That should sort that out. The entire old test suite is eliminated.

Valuable tests are ported into the rspec suite, useless tests were tossed, as were things that the acceptance suite already covered.

This should pass on any Unix platform one 1.8.5, 1.8.7, and 1.9.3. rake unit won't work any longer, but rspec covers the same ground now. Have fun.

@daniel-pittman Are we still planning to merge this?

@cprice404 cprice404 commented on an outdated diff Mar 23, 2012

lib/puppet/provider/nameservice.rb
- # generate one field type per class.
- def autogen_id(field)
- highest = 0
-
- group = method = nil
- case @resource.class.name
- when :user; group = :passwd; method = :uid
- when :group; group = :group; method = :gid
+ # Autogenerate either a uid or a gid. This is not very flexible: we can
+ # only generate one field type per class, and get kind of confused if asked
+ # for both.
+ def self.autogen_id(field, resource_type)
+ # Figure out what sort of value we want to generate.
+ case resource_type
+ when :user; group = :passwd; method = :uid
+ when :group; group = :group; method = :gid
else
@cprice404

cprice404 Mar 23, 2012

Contributor

Not really relevant to this particular pull request, but the use of the variable name "group" here seems unnecessarily confusing given the context of what we're trying to accomplish.

@cprice404 cprice404 commented on an outdated diff Mar 23, 2012

lib/puppet/provider/nameservice.rb
else
raise Puppet::DevError, "Invalid resource name #{resource}"
end
- # Make sure we don't use the same value multiple times
- if defined?(@@prevauto)
- @@prevauto += 1
- else
+ # Initialize from the data set, if needed.
+ unless @prevauto
+ highest = 0
+
Etc.send(group) { |obj|
if obj.gid > highest
@cprice404

cprice404 Mar 23, 2012

Contributor

Not relevant to 1.9 changes... but...
I might not be understanding this code correctly, but this seems wrong... it seems like it should be:

if obj.send(method) > highest

And if that's accurate, then we should probably use a variable to avoid calling "obj.send(method)" three times.

If I'm misunderstanding the intent of the code and "obj.gid" is correct, then perhaps this code could use some commenting.

Contributor

slippycheeze commented Mar 23, 2012

This push rebases against the new master, after the previous major merge, and fixes one additional test failure that was revealed (but not caused) by those changes. I have not yet handled the comments, which I intend to do shortly - but I wanted the extra change out there to stabilize the last known test issue.

@cprice404 cprice404 commented on the diff Mar 23, 2012

lib/puppet/util.rb
@@ -407,13 +405,21 @@ def replace_file(file, default_mode, &block)
# If the file exists, use its current mode/owner/group. If it doesn't, use
# the supplied mode, and default to current user/group.
if file_exists
- stat = file.lstat
+ if Puppet.features.microsoft_windows?
+ mode = Puppet::Util::Windows::Security.get_mode(file.to_s)
+ uid = Puppet::Util::Windows::Security.get_owner(file.to_s)
+ gid = Puppet::Util::Windows::Security.get_owner(file.to_s)
+ else
@cprice404

cprice404 Mar 23, 2012

Contributor

I wonder if it would be worth considering a monkey patch to some File or Stat class to abstract away this windows permissions stuff? Only worthwhile if this kind of construct is going to appear frequently in our code... but that doesn't seem unfathomable.

@slippycheeze

slippycheeze Mar 23, 2012

Contributor

Actually, this is not very good: there is no real mapping between the Windows DACL to the simple Unix permissions model: you can go from Unix to Windows, but not vice-versa, without loss of data.

Ultimately we need to fix ACL handling in the code, and that may end up with changes to File - but this is the best short-term compromise. It is also the Windows team approved way.

@cprice404 cprice404 commented on the diff Mar 23, 2012

lib/puppet/util/monkey_patches.rb
+# Nothing else in the stack cares which form you get - you can pass the
+# string or symbol to things like `instance_variable_set` and they will work
+# transparently.
+#
+# Having the same form in all releases of Puppet is a win, though, so we
+# pick a unification and enforce than on all releases. That way developers
+# who do set math on them (eg: for YAML rendering) don't have to handle the
+# distinction themselves.
+#
+# In the sane tradition, we bring older releases into conformance with newer
+# releases, so we return symbols rather than strings, to be more like the
+# future versions of Ruby are.
+#
+# We also carefully support reloading, by only wrapping when we don't
+# already have the original version of the method aliased away somewhere.
+if RUBY_VERSION[0,3] == '1.8'
@cprice404

cprice404 Mar 23, 2012

Contributor

I've seen this statement in enough places to where I wonder if it wouldn't be worth abstracting it into a puppet utility method somewhere.

@slippycheeze

slippycheeze Mar 23, 2012

Contributor

I think that probably adds more cost than value, but if you wanted to experiment with, say, a feature to detect this, I would be cool with that. (Separately from this series, I think.)

@cprice404 cprice404 commented on an outdated diff Mar 23, 2012

lib/puppet/util/zaml.rb
@@ -237,37 +237,43 @@ def escaped_for_zaml
# will be reconstructed properly into the unicode character.
self.to_ascii8bit.gsub( /\x5C/n, "\\\\\\" ). # Demi-kludge for Maglev/rubinius; the regexp should be /\\/ but parsetree chokes on that.
gsub( /"/n, "\\\"" ).
- gsub( /([\x00-\x1F])/n ) { |x| ZAML_ESCAPES[ x.unpack("C")[0] ] }.
- gsub( /([\x80-\xFF])/n ) { |x| "\\x#{x.unpack("C")[0].to_s(16)}" }
+ gsub( /([\x00-\x1F])/n ) { |x| ZAML_ESCAPES[ x.unpack("C")[0] ] }#.
+# gsub( /([\x80-\xFF])/n ) { |x| "\\x#{x.unpack("C")[0].to_s(16)}" }
end
@cprice404

cprice404 Mar 23, 2012

Contributor

are you intentionally leaving this commented line in place? if so, perhaps a doc comment explaining why?

Contributor

slippycheeze commented Mar 23, 2012

The next push fixes the genuine bug that Chris found in the autogeneration of IDs. That resolves all the comments, as well as rebased to current master, and is ready to merge from my point of view.

@cprice404 cprice404 commented on the diff Mar 23, 2012

test/language/functions.rb
- end
-
- assert_raise(Puppet::ParseError, "Did not fail when command failed") do
- val = scope.function_generate([%x{which touch}.chomp, "/this/dir/does/not/exist"])
- end
-
- fake = File.join(File.dirname(command), "..")
- dir = File.dirname(command)
- dirname = File.basename(dir)
- bad = File.join(dir, "..", dirname, File.basename(command))
- assert_raise(Puppet::ParseError, "Did not fail when command failed") do
- val = scope.function_generate([bad])
- end
- end
-end
-
@cprice404

cprice404 Mar 23, 2012

Contributor

I don't know the parser well enough to easily determine whether these tests are useful... but it seems like it would be worth double-checking it and possibly porting some of this to spec or acceptance tests if you didn't already do so.

@slippycheeze

slippycheeze Mar 23, 2012

Contributor

Everything I deleted, I had checked and ported if it was missing in either the spec or acceptance tests. If you can positively identify it as "not covered", great, otherwise I did check that. :)

@cprice404

cprice404 Mar 23, 2012

Contributor

Sweet, figured you probably had.

@cprice404 cprice404 commented on the diff Mar 23, 2012

test/ral/providers/cron/crontab.rb
@@ -1,652 +0,0 @@
-#!/usr/bin/env ruby
-
-require File.expand_path(File.dirname(__FILE__) + '/../../../lib/puppettest')
-
-require 'puppettest'
-require 'mocha'
-require 'puppettest/fileparsing'
-
@cprice404

cprice404 Mar 23, 2012

Contributor

seems possible that there are a few things worth salvaging in this cron stuff if you didn't already.

Contributor

cprice404 commented Mar 23, 2012

I don't see anything else that jumps out at me... and there is a ton of good stuff in this change set that I am excited to see get merged in.

Contributor

cprice404 commented Mar 23, 2012

having a hard time getting the specs to run on my system--1.9.3-p125, rspec 2.3.0, mocha 0.9.12:

undefined method `run_all' for []:Array

Probably just an environment issue, in the process of investigating.

Contributor

cprice404 commented Mar 23, 2012

uninstalled rspec 2.3.0 and installed the latest and now it seems like I can run them OK. I usually use 2.3.0 because that is what the jenkins nodes have...

Contributor

cprice404 commented Mar 23, 2012

OK. I've now repro'd on 2 different machines:

All tests pass with ruby 1.9.3-p125 and rspec 2.6.
With rspec 2.3, on both machines, I get this:

Puppet::Util::Autoload should be able to load files directly from modules
 Failure/Error: Unable to find matching line from backtrace
 undefined method `run_all' for []:Array
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/hooks.rb:116:in `run_hook_filtered'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/example_group.rb:175:in `eval_before_alls'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/example_group.rb:230:in `run'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/command_line.rb:27:in `block (2 levels) in run'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/command_line.rb:27:in `map'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/command_line.rb:27:in `block in run'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/reporter.rb:12:in `report'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/command_line.rb:24:in `run'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/runner.rb:55:in `run_in_process'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/runner.rb:46:in `run'
 # /home/cprice/.rvm/gems/ruby-1.9.3-p125/gems/rspec-core-2.3.1/lib/rspec/core/runner.rb:10:in `block in autorun'

Thus I believe that we will probably have to upgrade all of the jenkins nodes to a newer version of rspec if we want to accept this patch as-is.

Contributor

slippycheeze commented Mar 24, 2012

Yup. This is a known bug in rspec 2.3, fixed in 2.4, so I don't feel strongly that we should do anything but update our CI systems to reflect this. Which I will just now proceed to do.

Contributor

slippycheeze commented Mar 25, 2012

Some additional testing with Ruby 1.9.2 found a couple of other cases worth accounting for. This doesn't bring back 1.9.2 support, just polishes some actual bugs out of the way that it showed because of strange quirks of implementation.

Now, everything passes on all Ruby 1.8.5, 1.8.7, and 1.9.3 with RSpec >= 2.4.0, and our CI system is prepared with RSpec 2.9.0. This should be ready to merge.

slippycheeze added some commits Mar 8, 2012

Add a feature to detect RDoc 1.0.1 being present on the system.
In practice the Puppet manifest documentation generator only works with the
version of RDoc that ships with Ruby 1.8 - version 1.0.1, which was identical
in 1.8.5 ad 1.8.7.

Previously, because it was a core library, we just assumed it was present; now
we need to test conditionally for it being there *and* the correct version.

That allows us to have a nicer failure mode as we work through rdoc use, to
clearly indicate to users which version we have.  As we support newer
versions, additional features can better identify what is available.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix 4-space indentation in puppet/util/rdoc.rb
There are no functional changes to this commit, just whitespace changes.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Document and cleanly fail when unsupported RDoc is used.
Update the documentation and code to fail cleanly when a supported version of
RDoc is not available.

This does not yet tackle loading an RDoc version specific implementation of
the process, only a clean failure when we can't cope.  In future this should
be replaced by some dynamic loading process based on the supported RDoc
version, if we retain the tool.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Ruby 1.9.3 is strict about block argument counts.
Unlike earlier versions, Ruby 1.9 is strict about matching argument counts in
blocks with their uses.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Ruby 1.9.3 has a different error when `require` fails.
The text of the error message when load fails has changed, resulting in the
test failing.  This adapts that to catch the different versions, allowing this
to pass in all cases.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Simplify a YAML report test to work with Ruby 1.9
The order of invocation of local method definition and the parent `inherited`
hook changed between 1.8 and 1.9, resulting in the hack that works around a
development check in this test stopping working.

None of that ceremony is actually needed, though, since we can totally just
use an instance of the actual class we are testing.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Don't cache check for `cdrom` source in the apt provider.
Ruby 1.9 warned about access to a class variable in providers, because their
semantics are almost never what users expect.

However, the use of a class variable in the apt provider was very strange: it
wanted to check if a cdrom source was specified, and fail if it was present,
to avoid a situation where apt would prompt for a CD to be inserted and hang
forever.

Since that is just as much a problem on the second package install as on the
first, it makes much more sense to perform this sanity check and fail before
every install - and the overall cost is relatively low.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Avoid class level variables, as they are not helpful.
Ruby 1.9.3 highlights some of the strange and confusing problems around class
level variables in Ruby; they tend to look and behave much more like global
state than people imagine, and lead down the road to trouble.

This migrates away from them to other mechanisms better fitted to solving the
problem in the Ruby language - and, typically, in ways that are much more
portable between the different Ruby interpreter implementations.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Revert "Disable replace_file on Windows"
This reverts commit 4272d1f.

To quote:

    Our current approach of setting owner/mode in the Puppet way risks
    losing any extra permissions beyond owner/group/everyone, which is
    significant enough that it is better to simply not support this
    operation until we have a better solution. replace_file isn't used in
    any code which actually runs on Windows at the moment, so this is
    a benign change.

...except that it isn't so benign as all that: various other parts of the code
open-code the same method, poorly, and with various different bugs.  They will
all, without question, lose permissions in exactly the same way on Windows.

At least by routing them through a central path we can start to unify their
behaviour and get to a place where fixing one point in the code fixes all the
faulty permission transfers that we previously had.

We win an overall net reduction in bug count, and an increase in
predictability of bugs, from this change.
Replace Puppet::Util::FileLocking with atomic `replace_file`
The Puppet::Util::FileLocking module had substantial complexity in aide of
allowing careful, synchronized updating of individual files.  It was super
careful, and wanted to be absolutely sure that it didn't have concurrent reads
and writes going on, so it:

 * used per-file Ruby level locks to avoid multiple threads accessing the file
   at the same time.
 * used file-system level cooperative file locking on the target file to avoid
   multiple processes seeing concurrent updates.
 * used a temporary file to avoid rewriting the target file in place, so there
   was no "partial file read" window.
 * carefully used portable constructs to achieve locking rather than
   OS-specific kernel level exclusions.

Of course, some of those claims were documented but no longer valid - using a
temporary file *and* cooperative file locking don't play nice, and the
temporary file bit had dropped from the actual implementation forever ago.

It also did nothing to satisfy any of the other interesting conditions around
files: if multiple concurrent writers began, there were no assurances beyond
chance in the OS process scheduler which would win and replace the file.

In practice all that was really wanted were some very, very simple semantics:

Any process or thread can read the file at any time, and will get a complete
and correct version of the file.

Any process or thread can write at any time, provided that the read constraint
is satisfied.

It turns out that we recently built a method with these exact semantics,
`replace_file`, which will perform an atomic replacement using rename, but
only if the new file content is safely and completely stored on disk.

That satisfies all current semantics: any process can read at any time, and
will either get the complete previous version, or the complete new version,
depending on how it schedules around the (POSIX defined atomic) rename.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix terminus testing for Ruby 1.9
The order of evaluation of abstract classes (Class.new) changed in Ruby 1.9,
resulting in the hacks here that worked around the "must be bound to a
constant" rules in the indirector starting to fail.

Technically, though, we don't actually care: we can define a whole class stack
properly, binding everything the way the system expects, and then clean it up
afterwards.

Less stubbing, better tests, and clearer logic, all of which are pretty nice.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Handle YAML load errors from the new Psych YAML parser.
Earlier versions of Ruby used the syck library to parse YAML; during 1.9 this
moved to the Psych parser based on libyaml.  This brings YAML 1.1 compliance
to the library, and generally moves in a forward direction.

It also makes some radical changes to the failure modes, since Psych is much
more forgiving of some YAML constructs that would previously block the heck
up.

It also, in Ruby tradition, does absolutely nothing to abstract over the
implementation: Ruby can use either Psych or Syck and will raise different
exceptions on failure in both cases.

Worse, Psych exceptions are not derived from StandardError, so
Psych::SyntaxError will actually bypass almost any normal attempt to trap the
error, and will have wildly different details to the Syck errors.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Action definition requires at least one argument.
Ruby 1.9 allows us to enforce the calling convention of face actions at load
time, rather than failing with a runtime error.  This, in turn, means that we
should define actions correctly in our tests, if we expect them to work.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Eliminate dead home-made assertion-at-dev-time-only code.
The assertion code hidden in here, designed to allow development time
assertions than vanish at runtime - in the classic "better to keep working on
corrupt data and failed assumptions than fail in front of the user" mode - was
not actually used anywhere in the product.

We may as well not carry this little legacy around any further than it current
has been, then.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
`instance_variables` changes return type between 1.8 and 1.9
The return type of `instance_variables` changes between Ruby 1.8 and 1.9
releases; it used to return an array of strings in the form "@foo", but
now returns an array of symbols in the form :@foo.

Nothing else in the stack cares which form you get - you can pass the
string or symbol to things like `instance_variable_set` and they will work
transparently.

Having the same form in all releases of Puppet is a win, though, so we
pick a unification and enforce than on all releases.  That way developers
who do set math on them (eg: for YAML rendering) don't have to handle the
distinction themselves.

In the sane tradition, we bring older releases into conformance with newer
releases, so we return symbols rather than strings, to be more like the
future versions of Ruby are.

We also carefully support reloading, by only wrapping when we don't
already have the original version of the method aliased away somewhere.

This includes a second monkey-patch, on the `resolv` library that ships with
Ruby 1.8.  This is the only bit of code in the core stack that made
assumptions about the return format of names from `instance_methods`.

Specifically, it assumed they were strings, and could be evaluated using
`instance_eval` to fetch out the value.  The fix is to replace that with a
call to `instance_variable_get` which, like it does in Ruby 1.9, avoids the
cost of eval parsing as well as being more robust overall.

I did audit the rest of the standard library on 1.8.5 and 1.8.7 to verify
this.  We only need worry if we start using the SOAP or Tk support.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Syck specific YAML parser tests can only run with the old engine.
Ruby 1.9 moved to using the Psych engine as the default YAML engine, replacing
the older Syck based YAML engine.  They did this without any regard to parse
tree compatibility, or abstraction over the underlying tool.

Consequently, the test that gets very intimate with the encoding format for
the parsed YAML tree is unable to function with the modern engine - it tries
to interact with a pile of methods that just don't match.

This commit disables that test unless the Syck parser is in use.  Really, the
entire test should be an external, abstracted acceptance level test.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Hash references on string now raise errors, not return nil.
Under Ruby 1.8 the syntax `"foo"[:bar]` would return `nil`, while Ruby 1.9 is
strict: it enforces that only numeric index arguments are acceptable and
raises an error when that precept is violated.

This means that a whole pile of specs that used to pass strings where
structured objects were accepted now blow up - because you can't request
arbitrary nonsense against their array dereference interface.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Calling map on string now fails, rather than yielding the string.
In Ruby 1.8 `"foo".map` would yield "foo" once, while in Ruby 1.9 it is not
defined as a method.

Various parts of our testing code stub other methods and return strings where
arrays of strings are appropriate; this historically worked because of that
1.8 shim, and no longer does.

The same is true of `String#each`, which fails in the same way.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Regular expression matching on symbols changed in Ruby 1.9
In Ruby 1.8 a regular expression match on a symbol would never match, while in
Ruby 1.9 it is equivalent t a regular expression match on the equivalent
string.  This is very sensible behaviour, but trips up parts of code that
expect an implicit "... but not symbol" match.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Whitespace and formatting cleanup on shellquote spec.
The formatting in the spec file was very odd, with extraneous and random
whitespace around arguments, odd line breaks, and so forth.

It also used a whole pile of temporary variables that it didn't need to.

This cleans that up, without making any functional changes to the code.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix the shellquote parser function on Ruby 1.9
The semantics of `String#include?` changed a little in Ruby 1.9, so that you
can't just pass an arbitrary integer and have it check for character
containment.  (...or, perhaps, the semantics of characters vs integers
changed.  Either way, the effect is the same.)

This fixes the code to do the right thing on all platforms; we want to process
the string byte-wise, since that is how sh may well regard it, and we have to
work around some limitations on 1.8.5 semantics of string iterators.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
`String#to_a` doesn't work in Ruby 1.9
In Ruby 1.8, counterintuitively, `"foo".to_a` would return `["foo"]` - an
array with a single entry, the string.  This went away in Ruby 1.9, likely
as a transition path to a much less confusing set of semantics.

Either way we should stop relying on that behaviour and get with the program.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Protect all existing methods when monkey-patching.
Most, but not all, the method we add to objects while monkey-patching were
correctly protected against overriding a core method.  This adds the right
protections to the handful that were missing them.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Handle the real semantics of `String#lines`...
The previous monkey-patching of `String#lines` and `IO#lines` led to their
behaving differently in our code than in standard Ruby code.  This is
generally surprising, and led to some subtle misuses of the method.

This corrects that, by adapting all the abusers to real Ruby calling
conventions over the enumerator interface.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
The return type of `methods` changed in 1.9
This test depended on the return type of `methods`, which went from array of
strings to array of symbols in Ruby 1.9.  Given that it really wanted to test
if the instance responded to the message, we can just directly test that.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
A correct implementation of IO#binwrite for Ruby 1.8
When we added the `IO#binwrite` method to our collection of backported
features, the specification was misinterpreted and the offset was treated as
"offset in the string we are writing".

Instead, it is an instruction to seek in the output file before writing, which
meant that our semantics were all wrong - and tests broke - when we ran
against the native binwrite on Ruby 1.9.

The new implementation is correct in behaviour on all tested cases, and has a
more extensive test suite exercising all the various parts of the behaviour
that we need.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix Cisco device canonicalization regular expression on 1.9
The behaviour of the `\b` Regexp match operator, or rather, the classification
of `/` as an end of word character, changed between Ruby 1.8 and 1.9.

Most uses in our code are safe, and this will lead to equally correct
behaviour, but the Cisco device interface canonicalization code used it where
"end of string before newline" was intended.

Moving to the `\Z` match, which imposes the exactly desired condition,
resolves the problem.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Partially fix UTF-8 and PSON encoding tests for Ruby 1.9
With the introduction of real encoding support in Ruby 1.9, various problems
including incorrectly encoded UTF-8 strings crop up.

By flagging the encoding of the spec file to Ruby, and forcing binary encoding
on strings we compare that contain invalid UTF-8, we can bypass some of those
headaches.

Since PSON is, by design, not compatible with JSON and can do useful things
with arbitrary binary data, this is the most reasonably current choice for how
to handle this troublesome set of data.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Implement YAML 1.1 binary data encoding in ZAML
The YAML spec has a mechanism to flag arbitrary binary data, as opposed to
sequences of Unicode characters in UTF-8.  This implements full support for
that in the ZAML encoder.

This is used on the basis of a string that doesn't contain valid UTF-8.
Another stinking heuristic, but at least one that results in conformant YAML
being emitted.

Pleasantly, the !binary tagged data is correctly ready by Syck as far back as
Ruby 1.8.5, allowing us to confidently ship this data without too much concern
for compatibility with older libraries.

Additionally, we stop escaping all UTF-8 data as a sequence of bytes.
Theoretically we could go back to escaping it, using real Unicode character
markers, but literal UTF-8 in the output file is just as acceptable.

This should gain better support for the Ruby 1.9 encoding markers, so that
strings could carry their binary or non-binary nature in ... but this gets
things working and passes both Unicode and Binary data tests.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
When shutting down Mongrel, wait synchronously for stop.
The Mongrel tests on Ruby 1.9 were highly unreliable, sometimes failing
because they tried to bind a port that was already in use.

This turns out to be because the default shutdown mode is to asynchronously
stop workers - combined with the fact that our test made a TCP connection to
the service, but never actually completed the request.

That led to one thread that dangled, if the socket didn't close in a timely
fashion, and blocked up the listen port.

Instead, two things: first, wait for all threads to terminate (or 60 seconds
to pass) in the stop method, and second, use a full HTTP request to test, not
just a TCP connection with no content.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Use PID-based ports to test web server listening.
When running more than one instance of the spec suite concurrently on a single
machine, the hard-coded listening ports in some of the integration tests could
lead to failures - when multiple suites hit the same tests at the same time.

This uses a port number based off the PID, which helps ensure we don't get
spurious failures from concurrency.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Remove the "fails_on_ruby_1.9.2" tag from tests...
Now we have passing tests on Ruby 1.9, we can make all those tags go away and
enable the full suite.  Now anything that fails should be treated like a real
failure, which is reasonable since this is a real and supported platform.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Trivial spelling fix.
Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Whitespace cleanup in old-style aptrpm package test.
There are no functional changes, just fixing odd whitespace.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port old `test/language` tests into rspec...
In Ruby 1.9 the Test::Unit library was replaced by MiniTest.  This changes
some "esoteric" parts of the code, sadly including the parts of the library we
depend on to confine certain tests to only some systems.

Instead of investing heavily in rebuilding that facility over the new MiniTest
library it makes sense to just check for and either destroy or port the old
tests to the new rspec harness.

This is our desired future - that only one test framework exist - and we might
as well take full advantage of this opportunity to clean up our legacy code.

It also drops tests that duplicate testing already present in the spec suite.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port `test/provider` to RSpec
This brings over the meaningful tests of the provider class over from
Test::Unit to RSpec.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port Puppet[:path] test from Test::Unit to RSpec
This test checks that changing the setting results in changing the process
environment.  Not the most awesome test, but nothing else checks either.

It also highlights that we didn't preserve the process environment over tests,
which is bad, because that is a shared, persistent resource that will change
state in response to test actions.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port Puppet[:libdir] and LOAD_PATH test from Test::Unit
This ports over the :libdir setting test, and checks that it adjusts the
LOAD_PATH.  That, like the previous test, highlights a shared resource that
was not being saved or restored in our test suite.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port tests of the defaults system from Test::Unit.
The only meaningful test was that the Puppet version number was sane.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port some type class tests over to RSpec.
The type tests exercise parts of the type code in ways that the other tests
don't, so they should be ported over.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port over `ral/manager` tests to RSpec.
The RAL manager tests exercise code that isn't otherwise exercised, so most of
the tests can be ported over.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port some exec tests from Test::Unit to RSpec
Most of the tests were redundant to either existing acceptance tests, or to
existing spec tests, but a couple of combination tests were worth porting.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the useful cron Test::Unit test to an acceptance test.
Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the mailalias test directly to RSpec.
The old Test::Unit mailalias test is fairly up-front about being a weak test,
but it was more than we had in the spec area at all.  So, it can be ported
as-is to retain at least the same level of testing of the type.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the crontab Test::Unit tests to RSpec.
These tests covered a bunch of content that wasn't already covered, so they
should be ported.  Includes some fun around enumerators and Ruby 1.8.5.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the aptrpm and aptitude tests from Test::Unit.
These are fairly weak tests, but they exist - which ensures that we can't slip
through a syntax error in that provider.  So, better ported than destroyed.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Remove useless or redundant Test::Unit tests.
The other test suites, especially acceptance, cover everything that these
tests cover for the a whole bunch of the RAL tests in the older system.

Others were entirely disabled, and can simply be stripped out.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the provider tests from Test::Unit.
The provider tests exercise a whole pile of code that wasn't covered by the
specs.  This ports those tests over, and extends them to do further
combination testing in a few places.

These are very repetitive, table driven tests - but so is the underlying
feature we are trying to test.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the base service provider tests from Test::Unit.
The `base` service provider is terribly anæmic, but we had some tests, and it
does serve as the parent for almost every other provider.  So, port those
tests over.

Also, add a dash of cross-platform support by invoking the current Ruby
interpreter to do file system manipulation things - rather than leaning on
Unix tools that are not available everywhere.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Port the large fixture for the sshkey test from Test::Unit.
The only test not present in the existing spec was an effort to parse a large
sample file, and ensure it would round-trip correctly through the parsedfile
provider.

This is now ported over.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Use RbConfig rather than Config
RbConfig and Config are both names for the compile-time configuration
constants out of Ruby as far back as 1.8.5, so we can happily use the newer
name.

This eliminates a deprecation warning from Ruby 1.9, which finally started
complaining that we should have made this move already.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix signal handling test vs Ruby 1.9 async signals.
Ruby 1.9, unlike Ruby 1.8, uses a dedicated OS-level thread to handle signals.
This means that there can be arbitrary delays in signal delivery, thanks to
the vagaries of OS level thread scheduling.

The recent refactoring of application bootstrapping reduced the amount of OS
level activity in the self-signalling area, revealing this problem.

In the specific test, we used to have enough delay and OS interaction to
reliably (enough that I never saw the failure) switch to the signal thread and
run the code *before* we restored the previous signal handler.

After the changes we don't do that - that work is delayed, so the signal is
delivered during the test cleanup cycle, or even delayed until after the suite
exits.

This is, of course, not a problem on Ruby 1.8, where the signal is handled in
the main thread, and much more synchronously to the rest of the code.

So, the test is rewritten: we poll waiting for completion in the signal
handler, and have an outer timeout to catch a failure to invoke and fail out
of the test anyhow.

None of the native Ruby cross-thread communication or locking mechanisms
really satisfy the needs of this code, since we want to complete regardless of
when the condition is satisfied - even before we enter the wait period.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Fix nameservice provider auto-generated IDs that may overlap.
When the ID autogenerator was implemented in the nameservice provider base, it
had a small bug: it would always use the GID of the entry to find the largest
number, regardless of it looking for a GID or a UID.

This means that if you did not use the modern convention of per-user primary
groups, and your highest UID was larger than your highest primary GID, you
would happily generate overlapping IDs.

While the method itself isn't awesome, it is broadly enough used that we
shouldn't get rid of it entirely.  This fixes it to correctly determine the
largest UID for UID search, and GID for GID search, with less metaprogramming.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Define Symbol#intern if missing.
Ruby 1.9 defined Symbol#intern as `return self`, which makes perfect sense.
There is no equivalent on 1.8, which means that we can hit some issues now
that we return symbols for 1.9 compatibility in places.

Since the definition is obvious and correct, we may as well just implement the
missing method.

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Modernize selector_spec, add Ruby 1.9.2 support, with bonus bugfix.
The spec was wildly out of date, and did horrible things with stubs and
instance variables.  Instead, lets rewrite to use nice let methods, helper
functions, and a modern approach without stubbing the world.

This sets the stage for the relatively minor changes needed to support 1.9.2
in this particular area of the tests.

Finally, actually *testing* the code turned up a cosmetic bug in the string
output code: if you only had a single match value, which pretty much never
happens, you would represent it as:

    ... ? { "match", "value" }

The correct output, now implemented in a nasty, inefficient way, is:

    ... ? { "match" => "value" }

Signed-off-by: Daniel Pittman <daniel@puppetlabs.com>
Contributor

slippycheeze commented Mar 30, 2012

I am finally going to merge this in; it had a bunch of review, and should be ready. Passes all the tests locally, time to roll it out everywhere.

slippycheeze added a commit that referenced this pull request Mar 30, 2012

Merge pull request #575 from daniel-pittman/maint/master/ruby-1.9.3-s…
…upport

Real, complete Ruby 1.9 support - at last.

@slippycheeze slippycheeze merged commit 125868b into puppetlabs:master Mar 30, 2012

@stschulte stschulte commented on the diff Apr 6, 2012

lib/puppet/resource/status.rb
@@ -12,7 +12,10 @@ class Status
attr_reader :source_description, :default_log_level, :time, :resource
attr_reader :change_count, :out_of_sync_count, :resource_type, :title
- YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count @out_of_sync_count @tags @time @events @out_of_sync @changed @resource_type @title @skipped @failed}
+ YAML_ATTRIBUTES = %w{@resource @file @line @evaluation_time @change_count
+ @out_of_sync_count @tags @time @events @out_of_sync
+ @changed @resource_type @title @skipped @failed}.
+ map(&:to_sym)
@stschulte

stschulte Apr 6, 2012

Member

Isn't map(&:to_sym) a ruby >= 1.8.7 only thing? IIRC this is not valid in 1.8.5

@stschulte

stschulte Apr 6, 2012

Member

ok, I guess this is already handled in lib/puppet/util/monkey_patches.rb:

class Symbol
  def to_proc
    Proc.new { |*args| args.shift.__send__(self, *args) }
  end unless method_defined? :to_proc
end
@slippycheeze

slippycheeze Apr 6, 2012

Contributor

Yes, just so. Our policy over the last year has been to upgrade older Ruby where possible, so that you can write code that works nicely on the most recent version we support, and "at least works" on older versions. That seems to be the most robust and effective way to keep things working, and so far hasn't had too high a support cost - certainly, much less than having to limit ourselves to Ruby 1.8.5 syntax ever did.

That is the same reason this line changed overall - 1.9 changed to return symbols rather than strings from methods, and rather than convert backward we upgraded older Rubies to also return symbols from the same core method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment