Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malformed version number string 2.7.5+ #79

Closed
jtimberman opened this Issue Nov 29, 2013 · 15 comments

Comments

Projects
None yet
5 participants
@jtimberman
Copy link
Contributor

jtimberman commented Nov 29, 2013

Environment

  • Debian jessie (testing)
  • Chef 11.8.0
  • Ohai 6.20.0
  • Python 2.7.5+

(what's the +? Read on...)

Problem

When loading the default attributes file, Chef raises with this lovely exception:

================================================================================
Recipe Compile Error in /var/chef/cache/cookbooks/datadog/attributes/default.rb
================================================================================

ArgumentError
-------------
Malformed version number string 2.7.5+

Cookbook Trace:
---------------
  /var/chef/cache/cookbooks/datadog/attributes/default.rb:47:in `new'
  /var/chef/cache/cookbooks/datadog/attributes/default.rb:47:in `from_file'

Relevant File Content:
----------------------
/var/chef/cache/cookbooks/datadog/attributes/default.rb:

 47>>   default['datadog']['install_base'] = Gem::Version.new(node['languages']['python']['version']) < Gem::Version.new('2.6.0')

For some reason, a + was appended to the default version of Python in Debian "jessie" (testing).

jtimberman@jenkins:~$ python --version
Python 2.7.5+

On Debian 7.2 (wheezy), this is not the case, so the error doesn't manifest itself.

jtimberman@debian-wheezy:~$ python --version
Python 2.7.3

Proposed Solution

A horrible #gsub could be sent to the python version attribute can be used to strip out the character(s) not allowed in Gem::Version comparison.

jtimberman added a commit to jtimberman/chef-datadog that referenced this issue Nov 29, 2013

@ghost ghost assigned miketheman Nov 29, 2013

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Nov 29, 2013

@jtimberman Thanks for surfacing this issue.

Here's a few things:

  1. Why would Debian jessie report that, when the package clearly denotes a version string of 2.7.5-5? That's something definitely wrong on the Debian side of things.
    I would recommend that you use the Debian reportbug/email methods to surface this problem to the maintainers, as it's clearly a new version string differing from anything they have done before.
    Long ago, I had requested that fauxhai not blacklist the languages section, so these kind of tests could surface to those that don't own these platforms, but that didn't work out.
    This is probably the most effective option, as it seems they are using an incorrect string identifier, and fixing this to the correct string is probably the best overall action.
  2. In rubygems 2.0.5 (current release) as well as master branch, Gem::Version currently does not fully support SemVer 2.0.0 build metadata tags, that's something I'm actually going to hammer on for a bit to see if I can extend it to allow for the + indicator.
    Regardless, even if fixed immediately, will not fix your current issue.
  3. I thought about adding a dependency on the versionomy lib (which I've used before) or mixlib-versioning - both might work, but both introduce a new dependency that is not already there, and I'd prefer to not do that. (and I don't think we need more than 1 version comparison library, but that's just me..)

I agree that the solution proposed in #80 will fix your specific use case, but that's patching code for general use that may not solve things for others - I'd rather get to the bottom of this and get it fixed at the root cause.

In general, working with a testing distribution may produce these kinds of bugs, so it's on you for using something that cutting edge. 😉

Looking at the internals of the Debian package source, I couldn't really find where that string would be represented as such, so it's pretty confusing to me why this would report such.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Nov 29, 2013

Here's the ticket I opened for the Gem::Version comparison: rubygems/rubygems#724

@jtimberman

This comment has been minimized.

Copy link
Contributor Author

jtimberman commented Nov 30, 2013

Wow, thanks for the supreme detail and investigation. I'll add the test and push, but probably not today (maybe later tonight...)

@jtimberman

This comment has been minimized.

Copy link
Contributor Author

jtimberman commented Dec 2, 2013

I'm trying to run the specs and getting this error:

% bundle exec rake chefspec
bundle exec rspec cookbooks/datadog/spec/
/Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/configuration.rb:896:in `load': cannot load such file -- /Users/jtimberman/Development/cookbooks/chef-datadog/cookbooks/datadog/spec (LoadError)
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/configuration.rb:896:in `block in load_spec_files'
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/configuration.rb:896:in `each'
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/configuration.rb:896:in `load_spec_files'
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/command_line.rb:22:in `run'
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:80:in `run'
    from /Users/jtimberman/.rbenv/versions/1.9.3-p448/lib/ruby/gems/1.9.1/gems/rspec-core-2.14.7/lib/rspec/core/runner.rb:17:in `block in autorun'
rake aborted!
Command failed with status (1): [bundle exec rspec cookbooks/datadog/spec/...]
/Users/jtimberman/Development/cookbooks/chef-datadog/Rakefile:55:in `block in <top (required)>'
Tasks: TOP => chefspec
(See full trace by running task with --trace)

I run rspec directly (against the spec dir, rather than the dir in the Rakefile), and all the specs fail (except the one that checks the default recipe does nothing)

https://gist.github.com/jtimberman/7744992

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Dec 2, 2013

Aw, phooey. A couple of questions:

  1. This was built around ChefSpec 2, there's been some shifting for ChefSpec 3, namely dropping support for Chef 10, which we need to maintain.
  2. It looks like most of the errors are CookbookNotFound - did you run rake berks prior? The test suite expects the cookbook (and dependent cookbooks, apt/yum) to be in ./cookbooks/. Don't know if this is good/bad/ugly, but it was largely due to some berks shims being removed a long time ago, and it's been working well ever since.
    The default rake task marshals all the pieces into place.
@jtimberman

This comment has been minimized.

Copy link
Contributor Author

jtimberman commented Dec 2, 2013

Ah. I didn't. Perhaps the chef spec task should call the berks task first? I'll give that a go.

@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Dec 2, 2013

It probably could/should, unless there's a better way to setup/test a cookbook these days?

@jtimberman

This comment has been minimized.

Copy link
Contributor Author

jtimberman commented Dec 2, 2013

Seems fine, I just overlooked it while poking around :).

@schisamo

This comment has been minimized.

Copy link
Contributor

schisamo commented Dec 11, 2013

I just hit this issue on an Ubuntu 11.04 box in our CI infrastructure :(

================================================================================
Recipe Compile Error in /var/chef/cache/cookbooks/datadog/attributes/default.rb
================================================================================


ArgumentError
-------------
Malformed version number string 2.7.1+


Cookbook Trace:
---------------
  /var/chef/cache/cookbooks/datadog/attributes/default.rb:47:in `new'
  /var/chef/cache/cookbooks/datadog/attributes/default.rb:47:in `from_file'


Relevant File Content:
----------------------
/var/chef/cache/cookbooks/datadog/attributes/default.rb:

 40:
 41:  # Agent Version
 42:  default['datadog']['agent_version'] = nil
 43:
 44:  # Set to true to always install datadog-agent-base (usually only installed on
 45:  # systems with a version of Python lower than 2.6) instead of datadog-agent
 46:  begin
 47>>   default['datadog']['install_base'] = Gem::Version.new(node['languages']['python']['version']) < Gem::Version.new('2.6.0')
 48:  rescue NoMethodError # nodes['languages']['python'] == nil
 49:    Chef::Log.warn 'no version of python found'
 50:  end
 51:
 52:  # Chef handler version
 53:  default['datadog']['chef_handler_version'] = nil
 54:
 55:  # Boolean to enable debug_mode, which outputs massive amounts of log messages
 56:  # to the /tmp/ directory.

Maybe we should use an alternate version string parser (vs Gem::Version) that speaks SemVer?

schisamo added a commit to schisamo/chef-datadog that referenced this issue Dec 11, 2013

Additionally catch ArgumentError; Fixes DataDog#79
This ensures the cookbook is still usable even if the Python version 
string is not parsable by `Gem::Version`. This occurs with certain 
Debian/Ubuntu Python version strings like `2.7.1+`.
@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Dec 12, 2013

@jtimberman @schisamo Thank you both for reporting and submitting two different approaches to fixing this.

@schisamo On the one hand, I'd love to have a widely-distributed method for parsing version strings readily available within a given Chef run, so I wouldn't have to drag any more in (versionomy, mixlib-versioning, etc).

That doesn't solve the basic problem that the string 2.7.5+ isn't valid SemVer to begin with, due to it not having any characters following the + sign. So that's all Someone Else's Fault, and we have to live with that for now. Ugh.

I think @jtimberman's solution to strip off anything after the Version is a good place to go - as it continues to ensure that there's a valid version of Python installed.

Will the ArgumentError catch if no python is available? That's rare, sure, but possible, and at that point, I think we'd rather fail than warn.

Thoughts?

@schisamo

This comment has been minimized.

Copy link
Contributor

schisamo commented Dec 13, 2013

@miketheman Both approaches will get the job done, I just don't like the idea of introducing additional parsing logic or regexes that could continue to break. My approach (catching ArgumentError) will also ensure the cookbook can load no matter what sort of wacky format the Python version is in:

irb(main):003:0> Gem::Version.new('eggnog')
ArgumentError: Malformed version number string eggnog
    from /usr/local/var/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/version.rb:187:in `initialize'
    from (irb):3:in `new'
    from (irb):3
    from /usr/local/var/rbenv/versions/1.9.3-p448/bin/irb:12:in `<main>'
irb(main):004:0> Gem::Version.new('2.7.0+')
ArgumentError: Malformed version number string 2.7.0+
    from /usr/local/var/rbenv/versions/1.9.3-p448/lib/ruby/1.9.1/rubygems/version.rb:187:in `initialize'
    from (irb):4:in `new'
    from (irb):4
    from /usr/local/var/rbenv/versions/1.9.3-p448/bin/irb:12:in `<main>'

The original NoMethodError rescue (which is still in place) should still catch cases where no Python is available.

IMO this sort of logic belongs in a recipe. This would allow the Python version string parsing logic to only get exercised if a user didn't override node['datadog']['install_base'].

@dmerrick

This comment has been minimized.

Copy link

dmerrick commented Mar 5, 2014

Has there been any progress on this?

@alq666

This comment has been minimized.

Copy link
Contributor

alq666 commented Mar 18, 2014

@dmerrick pushing an update this week

miketheman added a commit that referenced this issue Mar 19, 2014

Merge pull request #84 from schisamo/fix-malformed-version-string
Additionally catch ArgumentError; Fixes #79
@miketheman

This comment has been minimized.

Copy link
Collaborator

miketheman commented Mar 19, 2014

Thanks to @jtimberman and @schisamo, we've got a few fixes in place for this.

@dmerrick

This comment has been minimized.

Copy link

dmerrick commented Mar 19, 2014

Thanks all!

miketheman added a commit that referenced this issue Mar 19, 2014

n1koo pushed a commit to Shopify/chef-datadog that referenced this issue Oct 20, 2014

Additionally catch ArgumentError; Fixes DataDog#79
This ensures the cookbook is still usable even if the Python version 
string is not parsable by `Gem::Version`. This occurs with certain 
Debian/Ubuntu Python version strings like `2.7.1+`.

n1koo pushed a commit to Shopify/chef-datadog that referenced this issue Oct 20, 2014

n1koo pushed a commit to Shopify/chef-datadog that referenced this issue Oct 20, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.