cognitect / transit-ruby

Apache License 2.0
113 stars 24 forks source link

Outdated msgpack breaks on Ruby 2.4.0 #21

Closed plexus closed 7 years ago

plexus commented 7 years ago

The version of msgpack is currently specified as "~> 0.5.8", since msgpack is past 1.0.0 this keeps it locked to an outdated version. Msgpack contains C extensions, and these no longer compile on Ruby 2.4.0.

Would it be possible to bump msgpack?

Example failure: https://travis-ci.org/plexus/yaks/jobs/208618189

dchelimsky commented 7 years ago

Definitely possible! I'll look into it soon.

fnordfish commented 7 years ago

For ruby 2.4, Oj needs an update to 2.18. Unfortunately 2.12.13 doesn't parse very small floats correctly, making doubles_interesting.json and doubles_interesting.verbose.json fail. see: ohler55/oj#339

dchelimsky commented 7 years ago

Thanks @fnordfish

fnordfish commented 7 years ago

The float issue is fixed in Oj's master (v2.18.3a1)

Which makes only two tests fail:

spec/transit/reader_spec.rb[1:2:1]:

["~#'","abc"]["~#'","~n123456789012345678901234567890"]["~:this","~:that"]["^ ","~:this",[1,2,3,["^ ","~:that","the other"]]]

spec/transit/reader_spec.rb[1:2:2]:

{"~#'":"abc"}
{"~#'":"~n123456789012345678901234567890"}
["~:this","~:that"]
{"~:this":[1,2,3,{"~:that":"the other"}]}

The good news, updating msgpack to v1.1.0 doesn't seem to introduce any issues.

dchelimsky commented 7 years ago

I will try to look at this on Friday.

fnordfish commented 7 years ago

Got it. Since, Oj 2.12.10 you can/need to pass a block to sc_parse to deal with multi-document inputs. Using this in the Unmarshaler works: Oj.sc_parse(@parse_handler, @io) { |_stack| }

fnordfish commented 7 years ago

Oj released the fix as 2.18.3

dchelimsky commented 7 years ago

@fnordfish thanks for doing all the homework! I pushed the changes to a branch. Might not have time to release until Friday, but I'll definitely get it out then. If you have time and inclination between now and then, please feel free to verify. Thanks!

fnordfish commented 7 years ago

@dchelimsky I've checked your branch against yaks locally - all tests passing Update: that was running on ruby 2.3.3. Still need to check ruby 2.4 Update2: ✅ ruby 2.4

dchelimsky commented 7 years ago

Just released transit-ruby-0.8.599. Thanks @fnordfish