[fix] Ruby 2.5 compat with JRuby 9.2 #361

kares · 2018-11-28T13:48:16Z

... also cleanup some pieces - mostly build tasks

wasn't possible to simply rake test_ext, now works

resolves GH-336

enebo · 2019-01-10T21:26:24Z

@kares I just ran into this while testing oj and it seems to compare results with json gem. I may be underthinking this but can't we just use instanceof for RubyFixnum as a minimal fix here? We still internally use RubyFixnum for fixnums (so it should be forwards and backwards compatible). I believe the real issue with the code is that it uses object identity of metaclasses vs instanceof check of actual internal type.

@headius not sure how hard this is to release but I would like to get this fixed so oj development does not need to work around this (this is only for testing but painful since it crashes part way through test runs without editing).

kares · 2019-01-10T21:47:29Z

@enebo arguably but there's the ugliness of Integer sub-class checking ... this seemed clearer
+ also a hot-method so why not do a switch - and as 1.9.3 gets dropped this is pretty much official API

kares · 2019-01-10T21:48:37Z

... did have some more fixes (and performance improvement) e.g. BigDecimal isn't on par with C-code now, but someone needs to start merging

headius · 2019-01-10T23:11:49Z

The PR looks fine. I do not appear to have push rights for this repository, though.

@kares @enebo JRuby 1.7 is EOL...I don't think we should continue to try to support it in this gem. I'd also like to think 1.9.3 is EOL.

kares · 2019-01-11T00:28:45Z

@kares @enebo JRuby 1.7 is EOL...I don't think we should continue to try to support it in this gem.

json gem decides - do not want to force an incompatibility on them in a patch release (2.1 works with 1.9.3)

kares · 2019-01-11T00:32:14Z

@hsbt could you help us with our JRuby efforts for the json gem, or shall we ask someone else?
there's finally some activity on the repo (which is great), obviously you're busy fixing for C-Ruby.

hsbt · 2019-01-11T00:36:36Z

@kares I can handle this for JRuby compatibility on this repository. But I don't have a push grant of json gem on rubygems.org.

@flori Can you add me to owner grant of json gem?

enebo · 2019-01-11T14:37:41Z

@hsbt @headius does have push rights but I am unsure if he is in the position to give it to you or not.

headius · 2019-01-11T20:50:43Z

@kares I think we should do whatever's the best for 9k+ and ditch 1.7. The instanceof check would also be fine; there's only two subclasses of Integer in 2.5.

It would also be nice if you could move the non-bug-related commits to another PR. They're great, but they don't relate to this issue.

@hsbt We will ping you when we think this is ready to go. I have push rights for the gem, but you probably should get them too...

headius · 2019-01-11T20:51:45Z

@kares Actually I'd almost prefer to just use instanceof for all the types, putting the most common ones at the top (string, integer, array, hash, etc).

kares · 2019-01-12T07:00:22Z

It would also be nice if you could move the non-bug-related commits to another PR. They're great, but they don't relate to this issue.

sorry about that - just ends up much more time spent on non-essential stuff for me. I'd like to have moved this forward and have more 'changes' around built on top - both fixes and refactorings (that I was hoping to get into whatever current stable is). did not see the other commits as harmful - they actually establish-ed a usable base line for me. if I made things hard to merge, I am sorry and will move them around.

I think we should do whatever's the best for 9k+ and ditch 1.7.

once again I did not want to force breaking change for the 2.1 line, which I hoped this would end up at.

The instanceof check would also be fine; there's only two subclasses of Integer in 2.5.

... is this the 'actual' thing blocking the merge here?

slightly ugly but the only way to still compile under 1.7 once Ruby 1.9.3 gets dropped this should get ironed out ! resolves rubyGH-336

wezm · 2019-02-15T00:59:05Z

Hi folks, is there anything I can do to help get this merged? It looks like @kares addressed the feedback. It would would be great to see this make it into a JRuby release so we can upgrade our app from 9.1 to 9.2.

headius · 2019-02-15T01:36:44Z

@wezm There has not been a new release of this gem in two years, unfortunately, and I don't know how to make that happen. I have push rights for the gem, but I do not have push rights for this repository.

I would hate to have to fork the repository so we can get fixes out, but this is getting kind of ridiculous.

kares · 2019-02-21T16:20:21Z

@skarmakar that is exactly what we're fixing here (a 9.2 issue)
just to be clear as the above comment might confuse folks 😞
9.2 without the patch (using json 2.1.0)

$ jruby -v -rjson -e 'p({"a" => 1, "b" => 12345678901234567890}.to_json)'
jruby 9.2.5.0 (2.5.0) 2018-12-06 6d5a228 Java HotSpot(TM) 64-Bit Server VM 25.171-b11 on 1.8.0_171-b11 +jit [linux-x86_64]
Unhandled Java exception: java.lang.ClassCastException: org.jruby.RubyBignum cannot be cast to org.jruby.RubyFixnum
java.lang.ClassCastException: org.jruby.RubyBignum cannot be cast to org.jruby.RubyFixnum
              generate at json/ext/Generator.java:224
                 visit at json/ext/Generator.java:354
                 visit at org/jruby/RubyHash.java:668
          visitLimited at org/jruby/RubyHash.java:690
              visitAll at org/jruby/RubyHash.java:2725
              generate at json/ext/Generator.java:333
              generate at json/ext/Generator.java:306
           generateNew at json/ext/Generator.java:171
          generateJson at json/ext/Generator.java:35
               to_json at json/ext/GeneratorMethods.java:70
                  call at json/ext/GeneratorMethods$RbHash$INVOKER$s$0$0$to_json.gen:-1
                  call at org/jruby/internal/runtime/methods/JavaMethod.java:793
                  call at org/jruby/internal/runtime/methods/DynamicMethod.java:191
          cacheAndCall at org/jruby/runtime/callsite/CachingCallSite.java:325
                  call at org/jruby/runtime/callsite/CachingCallSite.java:141
  invokeOther3:to_json at -e:1
                <main> at -e:1
   invokeWithArguments at java/lang/invoke/MethodHandle.java:627
                  load at org/jruby/ir/Compiler.java:94
             runScript at org/jruby/Ruby.java:849
           runNormally at org/jruby/Ruby.java:772
           runNormally at org/jruby/Ruby.java:790
           runFromMain at org/jruby/Ruby.java:602
         doRunFromMain at org/jruby/Main.java:415
           internalRun at org/jruby/Main.java:307
                   run at org/jruby/Main.java:234
                  main at org/jruby/Main.java:206

9.2 with changes from the PR :

$ jruby -v -Ilib -rjson -e 'p({"a" => 1, "b" => 12345678901234567890}.to_json)'
jruby 9.2.5.0 (2.5.0) 2018-12-06 6d5a228 Java HotSpot(TM) 64-Bit Server VM 25.171-b11 on 1.8.0_171-b11 +jit [linux-x86_64]
"{\"a\":1,\"b\":12345678901234567890}"

9.1.17 works fine with (also without) the patch :

$ jruby -v -Ilib -rjson -e 'p({"a" => 1, "b" => 12345678901234567890}.to_json); p JSON::VERSION'
jruby 9.1.17.0 (2.3.3) 2018-04-20 d8b1ff9 Java HotSpot(TM) 64-Bit Server VM 25.171-b11 on 1.8.0_171-b11 +jit [linux-x86_64]
"{\"a\":1,\"b\":12345678901234567890}"
"2.1.0"

skarmakar · 2019-02-22T04:50:58Z

@kares I am really sorry if it caused any confusion. Definitely want this fix to work and solve the issue.

I used this branch in a rails project, but still was getting this same error. Is it something that even if we mention a different json gem in the Gemfile, still jruby loads the default json gem? I have created a sample app for reference. Please let me know if I am missing something.

kares · 2019-03-01T10:48:43Z

seems @flori landed the patch from this PR on master (for json 2.2.0).

so we have JRuby progress, still not on par with the C code (BigDecimal warning) hopefully to happen later

headius · 2019-03-13T14:36:20Z

I have updated JRuby 9.2.7 to json 2.2.0.

skarmakar · 2019-03-13T16:24:27Z

@kares @headius I confirm that it works with JRuby 9.2.5 by forcefully upgrading json to 2.2.0. Going to try 9.2.7

kares mentioned this pull request Nov 28, 2018

Pass test_parse_big_integers on jruby #355

Closed

kares mentioned this pull request Dec 14, 2018

Fix for bigdecimal updates #362

Merged

kares force-pushed the jruby-fix branch from 8f7957e to 5f3b2b2 Compare January 12, 2019 07:05

[fix] 2.5 compat on JRuby 9.2 Fixnum/Bignum -> Integer

5f3b2b2

slightly ugly but the only way to still compile under 1.7 once Ruby 1.9.3 gets dropped this should get ironed out ! resolves rubyGH-336

This was referenced Jan 12, 2019

baseline changes to help (JRuby) development #371

Merged

[CI] setup supported JRubies to test against #372

Merged

kares closed this Mar 1, 2019

headius added a commit to headius/jruby that referenced this pull request Mar 13, 2019

Updage to json 2.2.0 to get Ruby 2.5 fix for ruby/json#361

2e33d2d

headius mentioned this pull request Mar 13, 2019

Updage to json 2.2.0 jruby/jruby#5653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] Ruby 2.5 compat with JRuby 9.2 #361

[fix] Ruby 2.5 compat with JRuby 9.2 #361

kares commented Nov 28, 2018

enebo commented Jan 10, 2019

kares commented Jan 10, 2019

kares commented Jan 10, 2019

headius commented Jan 10, 2019

kares commented Jan 11, 2019

kares commented Jan 11, 2019

hsbt commented Jan 11, 2019

enebo commented Jan 11, 2019

headius commented Jan 11, 2019

headius commented Jan 11, 2019

kares commented Jan 12, 2019

wezm commented Feb 15, 2019

headius commented Feb 15, 2019

kares commented Feb 21, 2019

skarmakar commented Feb 22, 2019

kares commented Mar 1, 2019

headius commented Mar 13, 2019

skarmakar commented Mar 13, 2019

[fix] Ruby 2.5 compat with JRuby 9.2 #361

[fix] Ruby 2.5 compat with JRuby 9.2 #361

Conversation

kares commented Nov 28, 2018

enebo commented Jan 10, 2019

kares commented Jan 10, 2019

kares commented Jan 10, 2019

headius commented Jan 10, 2019

kares commented Jan 11, 2019

kares commented Jan 11, 2019

hsbt commented Jan 11, 2019

enebo commented Jan 11, 2019

headius commented Jan 11, 2019

headius commented Jan 11, 2019

kares commented Jan 12, 2019

wezm commented Feb 15, 2019

headius commented Feb 15, 2019

kares commented Feb 21, 2019

skarmakar commented Feb 22, 2019

kares commented Mar 1, 2019

headius commented Mar 13, 2019

skarmakar commented Mar 13, 2019