Add native jruby support to psych. Fixes #145 #148

Open
wants to merge 1 commit into
from

4 participants

@atambo

I'm able to run rake compile and rake test on jruby now and things mostly work. The only change I had to make was to PsychToRuby.java because 2c644e1 moved the path2class method from Psych::Visitors to Psych::ClassLoader. However, there are still 20 test failures.

@atambo

@headius, @tenderlove

I added jruby to travis-ci: https://travis-ci.org/tenderlove/psych/jobs/8375032

Most of the failures are expected judging by our excludes in the jruby test suite:

https://github.com/jruby/jruby/tree/master/test/externals/ruby1.9/excludes/Psych

Should I port our excludes to the psych repo as well so the tests run clean?

@headius
Collaborator

I think we'll want to try to fix the JRuby impl or add exclusions/skips to the psych tests, right @tenderlove?

I'll have a quick look at the failures too.

Thanks for getting this done so quickly, @atambo! I'm very excited about the possibility of getting Psych ext out of our codebase and making it gem-upgradable!

@headius
Collaborator

Ok yeah... I remember some of these failures.

A lot of them look like they're just behavioral differences between libyaml and SnakeYAML. In some cases, there may be configuration we can figure out to make SnakeYAML match up better (like that first failure, where it's dumping or missing TAG lines). Others, like line number differences, might be bugs in SnakeYAML we can get them to fix. But a lot of these could be inherent differences we can't do anything about.

We will probably have to take them case-by-case and leave the JRuby travis build a "known failure" until we can resolve them or skip them.

@atambo

@headius, I see work being done on the atambo-native-jruby branch (https://github.com/tenderlove/psych/tree/atambo-native_jruby).

Do you think the psych extraction will make it into the jruby 1.7.5 release?
If not, will it make it into any of the later 1.7.* releases since they are supposed to be maintenance only?

@tenderlove
@tenderlove
Owner

@headius do you remember where we were with this? I'd like to do it, but I can't remember the roadblocks we were hitting.

@headius
Collaborator

I think it was mostly that I didn't know how to set up packaging and distribution with the tools that psych is using (rake-compiler, hoe). The library builds and it's almost nearly completely compatible with the C version :-)

We'd want to update this from JRuby master, of course.

@tenderlove
Owner

Ok. I'll give it a whirl today and see what I can find. If I hit issues, I'll update this ticket so at least it's documented and I won't have to write embarrassing "I can't remember stuff" comments. ;)

@jemc

Ironically enough given our discussion in #216 about instance variables as public/private implementation details, I just ran into this tidbit of MRI-dependent code that may trip up JRuby like it's currently tripping up Rubinius in the psych test suite:

https://github.com/tenderlove/psych/blob/47adf879591570daebde62895b3712c216710661/ext/psych/psych_yaml_tree.c#L5-L13

I saw this in this PR: https://github.com/tenderlove/psych/pull/148/files#diff-5e38cee2815f08c9932d19acfd91a797R50

Hopefully JRuby happens to use the names 'mesg' and 'backtrace' as internal variables like MRI. Otherwise, tests for Psych::Visitors::YAMLTree#visit_Exception will fail.

Maybe we can find a better way to get the message and backtrace than reaching into MRI hidden ivars that would serve both JRuby and Rubinius?

@headius
Collaborator

We do emit those internal variables for Marshal, but they're not really there...so Psych isn't seeing them and so exceptions probably aren't serializing properly.

I filed #219 for that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment