Fix incorrect JRUBY version check #1080
Conversation
| @@ -128,7 +130,7 @@ def method_implemented?(mod) | |||
| # | |||
| # This is necessary due to a bug in JRuby prior to 1.7.5 fixed in: | |||
| # https://github.com/jruby/jruby/commit/99a0613fe29935150d76a9a1ee4cf2b4f63f4a27 | |||
| if RUBY_PLATFORM == 'java' && JRUBY_VERSION.split('.')[-1].to_i < 5 | |||
| if RSpec::Support::ComparableVersion.new(JRUBY_VERSION) < '1.7.5' | |||
There was a problem hiding this comment.
You need the platform check or a defined? check as naturally JRUBY_VERSION is only defined on JRuby :)
There was a problem hiding this comment.
Yep, saw the build and fixed it. I'm having some trouble setting up the enviroment so I couldn't run tests locally.
Previous check would also match JRuby 9000 versions.
deivid-rodriguez
force-pushed
the
fix_jruby_version_check
branch
from
a829255 to
660f1e0
Compare
|
CI failed in appveyor, but I added a workaround. |
| @@ -18,7 +18,7 @@ install: | |||
| - SET PATH=C:\Ruby%ruby_version%\bin;%PATH% | |||
| - ruby --version | |||
| - gem --version | |||
| - gem install bundler | |||
| - gem install bundler -v 1.11.2 | |||
There was a problem hiding this comment.
What's the reason appveyor needs this? This file is generated from https://github.com/rspec/rspec-dev/blob/master/travis/appveyor.yml and if this is a permanent change needed for all appveyor builds we should probably push this change to there.
There was a problem hiding this comment.
Hi @myronmarston!
What's the reason appveyor needs this?
Appveyor build was failing. It seemed like a bug in latest bundler, 1.12.0, so I figured forcing installation of the previous version would fix it. It worked!
This file is generated from https://github.com/rspec/rspec-dev/blob/master/travis/appveyor.yml and if this is a permanent change needed for all appveyor builds we should probably push this change to there.
True, I always forget about that... Just let me know how you want me to proceed!
There was a problem hiding this comment.
I guess it's not clear if it was a fluke or if bundler 1.12 is broken on Windows. Since we're not sure, let's not push this to the template just yet. Can you add a comment explaining that, though?
If we see other builds fail with bundler 1.12 we can add it to the template; or if we see other builds pass, this'll get blown away the next time we re-gen from the template.
There was a problem hiding this comment.
I guess it's not clear if it was a fluke or if bundler 1.12 is broken on Windows. Since we're not sure, let's not push this to the template just yet. Can you add a comment explaining that, though?
I've seen this one, plus https://ci.appveyor.com/project/deivid-rodriguez/byebug/build/502, https://ci.appveyor.com/project/deivid-rodriguez/byebug/build/503 and https://ci.appveyor.com/project/deivid-rodriguez/byebug/build/504 so far. I edited the commit to include a comment explaining.
If we see other builds fail with bundler 1.12 we can add it to the template; or if we see other builds pass, this'll get blown away the next time we re-gen from the template.
So should I move it to the template then?
There was a problem hiding this comment.
So should I move it to the template then?
I looked for other examples and couldn't find any (we haven't had any other RSpec builds on appveyor since bundler 1.12 was released), so my initial instinct was to wait until we saw more examples before moving it. But you've clearly seen other examples, so yes, moving it to the template would be nice.
But that can happen outside of this PR, so I plan to merge when green. And I don't consider it your responsibility to move it, but if you want to that would be great :). Otherwise we'll get to it later.
deivid-rodriguez
force-pushed
the
fix_jruby_version_check
branch
from
d460d23 to
9511f21
Compare
deivid-rodriguez
force-pushed
the
fix_jruby_version_check
branch
from
9511f21 to
660f1e0
Compare
|
Merging in spite of the red appveyor build since the bundler fix is coming in another PR. |
|
Thanks! |
Previous check would also match JRuby 9000 versions.
Refs: jruby/jruby#3686