-
Notifications
You must be signed in to change notification settings - Fork 245
Code changes to handle nokogiri gem version changes #749
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left few question. Let me know what you think?
azure.gemspec
Outdated
@@ -36,7 +36,11 @@ Gem::Specification.new do |s| | |||
s.add_runtime_dependency('faraday', '~> 0.9') | |||
s.add_runtime_dependency('faraday_middleware', '~> 0.10') | |||
s.add_runtime_dependency('mime-types', ['>= 1', '< 4.0']) # vagrant-share and other stuff relies on 1 | |||
s.add_runtime_dependency('nokogiri', '~> 1.6') | |||
if RUBY_VERSION.match(/(^1\.9.*)|(^2\.0.*)/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why use regex? Cann't we use >
checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intersting.. I totally forgot ruby this kind of comparison
azure.gemspec
Outdated
@@ -36,7 +36,11 @@ Gem::Specification.new do |s| | |||
s.add_runtime_dependency('faraday', '~> 0.9') | |||
s.add_runtime_dependency('faraday_middleware', '~> 0.10') | |||
s.add_runtime_dependency('mime-types', ['>= 1', '< 4.0']) # vagrant-share and other stuff relies on 1 | |||
s.add_runtime_dependency('nokogiri', '~> 1.6') | |||
if RUBY_VERSION.match(/(^1\.9.*)|(^2\.0.*)/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also need to check defined?(RUBY_VERSION) &&
? I see people using it. Do you know why or why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RUBY_VERSION is predefined constant in the ruby interpreter. If you have ruby installed you have this defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sarangan12 As discussed offline, I'd be inclined to have test for making sure we are installing right version so that the next owner would not have to test thing locally with all permutation that you have.
I'll leave it upto your best dev judgement !
Context
Problem
Assume a user with ruby version 1.9.3 and using azure gem. While installing the gem, the nokogiri gem version of 1.7 will be attempted to install [since it is compatible to 1.6 - in theory]. But the gem install will fail since nokogiri(1.7) could be installed only on ruby versions >= 2.1.0
Solution
We are going to check in the gemspec file for the Ruby version. If the version starts with '1.9'/'2.0' then nokogiri version that is compatible with 1.6.0 could be installed. Else, nokogiri version that is compatible with 1.7 (or greater) could be installed.
Test
Built the gem locally and tested with multiple ruby versions.
@yaxia @seguler @vishrutshah @veronicagg @salameer @devigned Could you please review and approve?