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

resolves #99 add support for JRuby #162

Closed
wants to merge 1 commit into from

Conversation

mojavelinux
Copy link
Contributor

@mojavelinux mojavelinux commented Jul 1, 2016

When RUBY_ENGINE == 'jruby', do the following:

  • map popen4 function to Open.popen3
  • map Yajl to MultiJson and set MultiJson engine to json_gem
  • update gemspec to build -java gem when running under JRuby
  • drop posix/spawn and yaml dependencies and add multi_json

Additionally, add rake and test-unit as development dependencies so rake and tests can be run via Bundler, respectively.

@mojavelinux
Copy link
Contributor Author

Please note of the following facts:

  • This patch does not change how pygments.rb functions under MRI in any way.
  • All the tests run successfully under JRuby 9.1.2.0
  • Installation and requiring of the library works exactly the same on all platforms (Ruby automatically selects the right version to install and load).
  • This gem must be released using JRuby (it will automatically upload the platform-specific variant).

@mojavelinux
Copy link
Contributor Author

I recognize that Rouge is now the preferred gem over pygments.rb. However, there are still integrations which rely on this gem that haven't had a chance to make the switch. Support for JRuby would really help in this transition period.

@mojavelinux mojavelinux changed the title add support for JRuby resolves #99 add support for JRuby Jul 1, 2016
When RUBY_ENGINE == 'jruby', do the following:

- map popen4 function to Open.popen3
- map Yajl to MultiJson and set MultiJson engine to json_gem
- update gemspec to build -java gem when running under JRuby
- drop posix/spawn and yaml dependencies and add multi_json

Additionally, add rake and test-unit as development dependencies so rake
and tests can be run via Bundler, respectively.
@gfx
Copy link
Collaborator

gfx commented Dec 11, 2016

This gem depends on the very POSIX process model. If you want to execute pygments for JRuby, it should be better that you call pygmentize(1) command directly.

Or, consider https://github.com/jneen/rouge, which is written in pure Ruby.

@gfx gfx closed this Dec 11, 2016
@mojavelinux
Copy link
Contributor Author

mojavelinux commented Dec 11, 2016

Seriously?!? Come on. This fix is critical for compatibility across Ruby platforms (esp during the transition to Rouge, which can't happen overnight). I spent a lot of time to figure this solution out and I think it is very reasonable. Can you please respect that effort and not close the door on JRuby? I urge you to reconsider.

@gfx gfx reopened this Dec 11, 2016
@gfx
Copy link
Collaborator

gfx commented Dec 11, 2016

Hmm, okay, will try to get compatibility with JRuby and keep it as much as possible.

However, dynamic dependencies in gemspec are useless. Can you fix some points?

  • Make POSIX::Spawn optional and add a configuring point to select POSIX::Spawn or pure Ruby open4
  • Always use MutiJson

@mojavelinux
Copy link
Contributor Author

Thank you. I'd be glad to update the patch. Stay tuned.

@gfx
Copy link
Collaborator

gfx commented Dec 24, 2016

The idea of this PR is included in #170. Thanks to the suggestion.

@gfx gfx closed this Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants