Skip to content
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

Work towards JRuby compatibility #5

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

dgolombek
Copy link

This is part of #4. Until JRuby 9.2.0.0 is released, the
required_ruby_version in the gemspec will keep this from working there,
as well as jruby/jruby#5099.

  • Replaced Oj with MultiJson + Oj OR JrJackson
  • Removed Oj.dump parameters, since those are defaults when using MultiJson
  • Remove rdiscount for Markdown generation, use default Yard generator.
  • Made two minor code syntax changes to make the Yard generator happy

I've roughly compared the generated RDoc and they look very similar.

This is part of aws#4. Until JRuby 9.2.0.0 is released, the
required_ruby_version in the gemspec will keep this from working there,
as well as jruby/jruby#5099.

* Replaced Oj with MultiJson + Oj OR JrJackson
* Removed Oj.dump parameters, since those are defaults when using MultiJson
* Remove rdiscount for Markdown generation, use default Yard generator.
* Made two minor code syntax changes to make the Yard generator happy

I've roughly compared the generated RDoc and they look very similar.
@haotianw465
Copy link
Contributor

Thank you for the contribution. Will get back to you soon,

@@ -1,5 +1,4 @@
--title 'AWS X-Ray SDK for Ruby'
--markup markdown
--markup-provider rdiscount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure the API ref doc has the same style as the AWS SDK for Ruby: https://github.com/aws/aws-sdk-ruby/blob/master/.yardopts#L5. Any reason you need to remove it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rdiscount gem uses the rdiscount C library, which means it's not really JRuby compatible. That's a development dependency, and I couldn't run rake docs (or run default rake task) while it was present. I built docs with the default markup provider and got very similar output (after I fixed the two parsing problems I encountered).

Copy link
Contributor

@haotianw465 haotianw465 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understand, with this change, MRI users that upgrade the SDK are required to add gem 'oj', platform: :mri in their Gem file, so this is a breaking change correct? Could this be avoid somehow or this has to happen in order to support JRuby?

@dgolombek
Copy link
Author

That's a good point about this being a breaking change. I was hoping that since the gem was still in beta, that was ok. If we want to avoid it, we'll have to build and publish two "flavors" of the gem, one for java and one for mri. That would allow us to do things in the gemspec like add conditionals based on the ruby platform to require different gems (and different ruby versions). I can update the gemspec to do that if you'd like -- you'll have to change how it publishes the gem to do both flavors.

@haotianw465
Copy link
Contributor

It is ok to make this breaking change for JRuby support.

There are two concerns:

  1. Per https://github.com/ohler55/oj/blob/master/pages/Modes.md "Object mode is for fast Ruby object serialization and deserialization. That was the primary purpose of Oj when it was first developed. As such it is the default mode unless changed in the Oj default options."
irb(main):001:0> require 'oj'
=> true
irb(main):002:0> Oj.default_options
=> {:indent=>0, :second_precision=>9, :circular=>false, :class_cache=>true, :auto_define=>false, :symbol_keys=>false, :bigdecimal_as_decimal=>nil, :use_to_json=>false, :use_to_hash=>false, :use_as_json=>false, :nilnil=>false, :empty_string=>true, :allow_gc=>true, :quirks_mode=>true, :allow_invalid_unicode=>false, :allow_nan=>true, :trace=>false, :float_precision=>16, :mode=>:object, :escape_mode=>:json, :time_format=>:unix, :bigdecimal_load=>:auto, :create_id=>"json_class", :space=>nil, :space_before=>nil, :object_nl=>nil, :array_nl=>nil, :nan=>:auto, :omit_nil=>false, :hash_class=>nil, :array_class=>nil, :ignore=>nil}
irb(main):003:0> require 'oj'

So it seems like :compat and use_as_json options cannot be removed. I checked multj_json documentation but couldn't find any evidence it explicit changes the default behavior of Oj. It is just a wrapper that passes whatever options in the caller to the actual engine. Am I missing something?

  1. Have you tested Rails ActiveRecord capturing with multi_json in both MRI and JRuby? IIRC some special changes is necessary to have ActiveSupport to pick up the correct encoder (since ActiveSupport has its own encoder).

@dgolombek
Copy link
Author

  1. MultiJson has default options for each adapter:
    https://github.com/intridea/multi_json/blob/master/lib/multi_json/adapters/oj.rb#L10
    The tests pass, and without those options this blows up while trying to encode

  2. I haven't explicitly tested with Rails/ActiveRecord. If you call Oj.optimize_rails(), Oj will still try to hook itself into Rails -- MultiJson does not provide similar functionality on its own. But I have used MultiJson in Rails projects without problem -- it's a VERY widely used gem, with 10x more downloads than Oj. It is unlikely to cause extra problems.

@haotianw465
Copy link
Contributor

Thanks for point it out. It looks good to me. Please note the SDK will still need locking on some certain functions to completely work with JRuby.

@haotianw465 haotianw465 merged commit e7daa78 into aws:master Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants