pygments / pygments.rb

💎 Ruby wrapper for Pygments syntax highlighter
MIT License
572 stars 141 forks source link

jRuby compatibility #99

Closed hiravgandhi closed 7 years ago

hiravgandhi commented 10 years ago

POSIX::Spawn causes issues with the latest version of jRuby. Is there a way to remove this dependency or alter it for jRuby so that it is not needed?

bartkamphorst commented 10 years ago

@mojavelinux I can get the tests to pass on ruby-2.1.1 using open3 instead of posix-spawn.

# Replace this @pid, @in, @out, @err = popen4(script) with
@in, @out, @err, @wait = Open3.popen3(script)
@pid = @wait[:pid]

So, in theory at least, a solution like this with Open3.popen3 might work for JRuby. POSIX::Spawn is much more efficient, though, so the popen3 solution should be implemented as the fallback option specifically for JRuby.

The other dependency with native extensions is Yajl. If I simply gsub instances of Yajl with JSON, then I get a couple of errors using 2.1.1, and under JRuby it hangs as you describe here. So I get the feeling that the real issue is in the dumping and loading of the json; that there is something specific in the way that Yajl does this. Perhaps a good first step would be to use 'multi_json' with the Yajl adapter, so that swapping in another adapter becomes trivial.

bartkamphorst commented 10 years ago

Just had a chance to quickly do a few tests on JRuby too. It works if I use multi_json with

MultiJson.engine = 'json_gem'

The only thing to pay attention to is that you must close the input stream @in before reading from @out (for example, by calling @in.close in get_header). Hope this helps, @mojavelinux.

mojavelinux commented 8 years ago

For reference, if you install the following gem:

$ gem install pygments.rb-jruby

Then replace the call to popen4 as @bartkamphorst suggested:

require 'open3' if RUBY_ENGINE == 'jruby'

...

@in, @out, @err, @wait = Open3.popen3(script)
@pid = @wait[:pid]

Then it works. Of course, that's an older version of the gem ;(

mojavelinux commented 8 years ago

I put together a pull request that implements these changes. I'm happy to say it works as expected!