Add ruby 1.8 to travis #154
Conversation
iainbeeston
force-pushed
the
ruby_1_8_support
branch
from
28f7e29 to
0cd1ef5
Compare
|
Ok, that was interesting. As @hoxworth mentioned in #149, I had to add json to the Gemfile for it to work at all on ruby 1.8. It also took a few commits to get the test suite to even run (due to syntax errors and api changes). Now it is running, it seems like everything more or less works, except for two issues that I can't see an easy solution for:
There might be other issues too. For now, I've added ruby 1.8 to the travis build, but marked it as allowed to fail. At the very least we should be able to see if the tests are able to run, and check it manually if needed. |
|
Oh, and because test unit that comes with ruby 1.8 doesn't support the skip method, we no longer skip unsupported tests in the common test suite, we don't run them at all. This is actually pretty good because it makes the test output far more readable. |
| @@ -17,4 +17,8 @@ Gem::Specification.new do |s| | |||
| s.extra_rdoc_files = ["README.textile","LICENSE.md"] | |||
| s.license = "MIT" | |||
| s.required_rubygems_version = ">= 1.8" | |||
|
|
|||
| if RUBY_VERSION.start_with?("1.8") | |||
| s.add_dependency "json" | |||
There was a problem hiding this comment.
Sadly, I don't think this works the way you might expect. This will add the dependency to the built gem based on the ruby version used when building the gem, not based on the user's ruby.
AFAIK there's no reasonable way to do conditional dependencies like this; we would either need to make it a 100%-all-the-time dependency, or rescue the inevitable LoadError on 1.8 and just explain what went wrong to the user.
There was a problem hiding this comment.
Interesting point. That should probably go into them Gemfile then (which is only for development and can be tied to a specific platform).
I imagine there won't be many new users on Ruby 1.8 - it feels like more trouble than it's worth to add a special error message just for this=
There was a problem hiding this comment.
Sorry, my point was supposed to be that there won't be many new users, and I expect anyone who's already using the gem will have figured out that they need to require json already=
There was a problem hiding this comment.
I've moved "json" from the gemspec to the Gemfile
Wasn't there before, but we want to support ruby 1.8
For ruby 1.8 compatibility
There are two good reasons to do this: 1. skip isn't supported in ruby 1.8 2. much less verbose ouput
There are many failures due to ruby 1.8 hashes being unordered and encodings being different to ruby 1.9. But at least there's a build
… issues At least we have mri 1.8 in the build matrix. Better than nothing
As @pd pointed out, putting conditional flags in the gemspec only has an affect when packaging the gem, and does not help end users. I've put it in the Gemfile instead, which will only help developers, but at least bundler has the concept of platforms
iainbeeston
force-pushed
the
ruby_1_8_support
branch
from
f50d417 to
4d12be3
Compare
|
I've rebased this, so it's mergable again |
|
Ah thanks for the rebase. 👍 on this. Beginning to question my support of 1.8.7, should probably just straight up drop it for version 3. Would make a bunch of the hack code in this library irrelevant.. |
|
I think 1.8 works... It's just that the tests don't (reliably) |
|
Also, even if we don't merge this branch, I'd like to pull out the common test suite changes into a new pull request - it makes the test suite far, far less noisy and far nicer to use |
|
👍, I think this should go as is, and we can whittle down the travis failures for 1.8 over time. Skimming through them, I think @iainbeeston is right that it's mostly a matter of hash ordering; for encoding issues, I've had to deal with such annoyances before and I'll see if I can get them sorted out. As a sidenote, it was originally my idea to have the suite be noisy about skipped tests, I just didn't expect test/unit's output to be so verbose! With rspec, it's just a few lines at the end of the test run. I've regretted that suggestion every time I've run the test suite since that landed on master. =) |
|
I have no problems merging this as is. Thanks @iainbeeston ! |
This pull request is to make sure we have working ruby 1.8 support. I expect the build to fail initially, until I can see what's broken and fix it (I don't have a working version of ruby 1.8 on my development machine)