-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Run all branches against JRuby on CI #1496
Conversation
@nadavshatz Nice! 👍 do you think all these runs are worth the cost to the CI provider and time of the run? I've never tested against so many JDK's or point releases of JRuby. |
@@ -9,6 +9,13 @@ rvm: | |||
- 2.3.0 | |||
- ruby-head | |||
- rbx-2 | |||
- jruby-9.0.4.0 | |||
- jruby-9.0.5.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.
are they different enough that it's worth having both?
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.
One jruby-9xxx
should be enough IMO. About having jruby-9xxx
and jruby-head
, I think it's generally a good idea to keep both. On the other side, I wonder if keeping 2.0.0
is necessary.
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.
We could also maybe consider using an include
so that we don't run jrubies against all rails version.
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.
@groyoh
jruby-9xxx
and jruby-91xx
(coming soon) are two different ruby
versions. but for now, having just one jruby-9xxx
should be enough, i agree.
regarding the include
- you mean that we won't run jruby
against Rails 4.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.
Well, Rails doesn't ;) rails/rails#23499
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.
So, diff will be something like
- 2.3.0
- ruby-head
- rbx-2
+ - jruby-9.0.5.0
+
+jdk:
+ - oraclejdk8
+
+before_install:
+ - '[ "$JRUBY_OPTS" != "" ] && export JRUBY_OPTS="-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug" && echo "JRUBY_OPTS: \'$JRUBY_OPTS\'"'
install: bundle install --path=vendor/bundle --retry=3 --jobs=3
cache:
@@ -30,10 +40,12 @@ matrix:
env: RAILS_VERSION=master
- rvm: 2.1
env: RAILS_VERSION=master
- rvm: jruby-head
- env: JRUBY_OPTS='-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug'
+ env: RAILS_VERSION=5.0.0.beta2
allow_failures:
- rvm: ruby-head
+ - rvm: jruby-head
- rvm: rbx-2
+ exclude:
+ - rvm: jruby-9.0.5.0
+ env: RAILS_VERSION=4.0
fast_finish: true
It also seems the Rails 4.0 is missing from the jruby 9k series... (only run on jruby-head). This is a version that often fails differently, so... |
@bf4 First off thanks for the quick response! Regarding the many JDK: Re many point releases: Rails 4.0 was failing, I thought that considering it is so old there is no need to add it, but I can put it back if you'd like. should it not be allowed to fail? Also - Jruby 9.0.x.0 is ruby 2.2 compatible and I wasn't sure if that is the issue for Rails 4.0. Let me know what you think and I'll update the PR! |
Well, we still support 4.0, so that needs to remain under test... I honestly don't know the value of testing the different jdks... I was going to ref Rails but it looks like they don't test JRuby at all anymore rails/rails@0e8c045 |
I removed OracleJDK7 and Added back Rails 4.0, is it possible that Rails 4.0 doesn't work with Ruby 2.2? I'm not sure why the tests are failing. |
@bf4 I've updated the PR and i think its now how you wanted it ? |
I'll pay attention and update to add/change to the latest stable versions as they come out with new PR's |
env: RAILS_VERSION=4.0 | ||
- rvm: jruby-head | ||
env: RAILS_VERSION=4.0 | ||
- rvm: jruby-9.0.5.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.
This is a duplicate from above at line 41.
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.
Good catch, thanks!
This look good to me (apart the duplicate). @bf4 any objection? |
@groyoh fixed the duplicate, thanks for that! |
- oraclejdk8 | ||
|
||
before_install: | ||
- '[ "$JRUBY_OPTS" != "" ] && export JRUBY_OPTS="-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug"' |
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.
plz change to 9.0.4.0
since travis caches it, faster, and JRUBY_OPTS to '--dev --debug --Xcli.debug=true'
. see rails/rails#23499 (comment)
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.
I pushed a fix for these, thanks!
Let me know if you want me to squash the commits into one.
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.
Squash plz :)
On Mon, Mar 7, 2016 at 8:57 AM Nadav Shatz notifications@github.com wrote:
In .travis.yml
#1496 (comment)
:@@ -9,6 +9,14 @@ rvm:
- 2.3.0
- ruby-head
- rbx-2
- jruby-9.0.5.0
- jruby-head
+jdk:
- oraclejdk8
+before_install:
- '[ "$JRUBY_OPTS" != "" ] && export JRUBY_OPTS="-Xcompat.version=2.0 --server -Xcompile.invokedynamic=false -Xcli.debug=true --debug"'
I pushed a fix for these, thanks!
Let me know if you want me to squash the commits into one.
—
Reply to this email directly or view it on GitHub
https://github.com/rails-api/active_model_serializers/pull/1496/files#r55213932
.
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.
Done!
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.
@nadavshatz you say done, but maybe you didn't push it yet?
LGTM 👍 I'll merge if no one has objections. |
@NullVoxPopuli please wait for rebase |
I need to make a checklist for reviewing. |
@bf4 @NullVoxPopuli Rebased and squashed. Commit should be clean now. Please let me know if there is anything else missing/to be fixed. Cheers 🍻 |
cool, thanks, @nadavshatz -- just gotta wait for travis/appveyor |
@@ -34,6 +34,7 @@ Misc: | |||
- [#1535](https://github.com/rails-api/active_model_serializers/pull/1535) Move the adapter and adapter folder to | |||
active_model_serializers folder and changes the module namespace. (@domitian @bf4) | |||
- [#1497](https://github.com/rails-api/active_model_serializers/pull/1497) Add JRuby-9000 to appveyor.yml(@corainchicago) | |||
- [#1496](https://github.com/rails-api/active_model_serializers/pull/1496) Run all branches against JRuby on CI |
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.
@nadavshatz would you mind adding this to the top of the list and give yourself credit? on CI (@nadavshatz)
. Thanks.
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.
Done! thanks!
Run all branches against JRuby on CI
Meant to resolve #1409
.travis.yml