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

Pathname find fails with (LocalJumpError: yield called out of block) instead of returning an Enumerator #1944

Closed
blanquer opened this issue Sep 3, 2014 · 9 comments

Comments

@blanquer
Copy link

blanquer commented Sep 3, 2014

while Pathname.find works as expected when yielding to a block, it fails without one (where it should return an enum).

Repro:

bug:foo $ irb
jruby-head :001 > require 'pathname'
 => true
jruby-head :002 > p = Pathname.new("/tmp/foo")
 => #<Pathname:/tmp/foo>
jruby-head :003 > p.find{|f| puts f}
/tmp/foo
/tmp/foo/Gemfile
 => nil
jruby-head :004 > p.find
LocalJumpError: yield called out of block
    from /Users/blanquer/.rvm/rubies/jruby-head/lib/ruby/2.1/pathname.rb:1002:in `find'
    from /Users/blanquer/.rvm/rubies/jruby-head/lib/ruby/2.1/find.rb:47:in `find'
    from org/jruby/RubyKernel.java:1092:in `catch'
    from /Users/blanquer/.rvm/rubies/jruby-head/lib/ruby/2.1/find.rb:46:in `find'
    from org/jruby/RubyArray.java:1546:in `each'
    from /Users/blanquer/.rvm/rubies/jruby-head/lib/ruby/2.1/find.rb:42:in `find'
    from /Users/blanquer/.rvm/rubies/jruby-head/lib/ruby/2.1/pathname.rb:1002:in `find'
    from (irb):4:in `evaluate'
    from org/jruby/RubyKernel.java:962:in `eval'
    from org/jruby/RubyKernel.java:1282:in `loop'
    from org/jruby/RubyKernel.java:1092:in `catch'
    from org/jruby/RubyKernel.java:1092:in `catch'
    from /Users/blanquer/.rvm/rubies/jruby-head/bin/irb:13:in `(root)'
jruby-head :005 >
@enebo enebo closed this as completed in 150c4ec Sep 3, 2014
@enebo enebo added this to the JRuby 9000 milestone Sep 3, 2014
@enebo
Copy link
Member

enebo commented Sep 3, 2014

Our pathname.rb file is a bit out of date. I added the missing logic for find.

@blanquer When you find other 9000 issues you can tag with 9000_feedback and we will try and evaluate as quickly as we can.

@eregon You originally did this work right? How am I supposed to merge this file? It is mostly the same but I need to tweak some portions right?

@eregon
Copy link
Member

eregon commented Sep 3, 2014

@enebo The direct delegation to others methods and #<=> have been
implemented in Java but the rest should have been unchanged. Merging and
removing new methods already implemented in Java should work.

@eregon
Copy link
Member

eregon commented Sep 14, 2014

@enebo It seems like @headius took an older lib/pathname.rb for stdlib 2.1, so the extension is likely ignored in master since supporting 2.1. I don't know the proper path for pathname.rb.

In MRI, Pathname native extension is in ext/pathname/pathname.c and Pathname Ruby library is in ext/pathname/lib/pathname.rb. The JRuby native extension should map to the C native part. So where should we put pathname.rb? In lib/ruby/2.1/? In ext/pathname/lib?
If we do that again it should simply be the matter of directly merging ext/pathname/lib/pathname.rb to the jruby fork.
Would require 'pathname.so' works with LazyBuiltins?

@enebo
Copy link
Member

enebo commented Sep 14, 2014

@eregon I think it should get put into lib/ruby/2.1. MRI on install puts it in this location so I guess we should just put it there. It is less complicated to find it as well.

@eregon
Copy link
Member

eregon commented Sep 15, 2014

@enebo OK, that's what I did in 0d9120f.

What about jruby/ruby? Should I git rm lib/pathname.rb?
Or/and git mv ext/pathname/lib/pathname.rb lib/pathname.rb?
Leaving no file in lib/ seems cleaner for merging with upstream. But I am afraid it get reintroduced for say, stdlib 2.2.0 as it was in jruby/ruby@c97f813. Is there any way to modify the mentioned patches so it doesn't add lib/pathname.rb anymore?

@enebo
Copy link
Member

enebo commented Sep 16, 2014

Possibly we could git rm lib/pathname from our ruby fork. Then fetching/merging later MRI should give us a clue we need to keep it rm'd?

@eregon how do we require it in etc/pathname/lib/pathname.rb? CLASSPATH require?

@headius how do you think we should handle extension .rb file relative to our periodic merging of MRI stdlib?

@eregon
Copy link
Member

eregon commented Sep 16, 2014

For the moment I just copied jruby/ruby's ext/pathname/lib/pathname.rb to lib/ruby/2.1/pathname.rb. I doubt it makes sense to reproduce the complicated ext/somelib/lib/somelib.rb hierarchy. But it means special attention when importing stdlib. (See the comment in 0d9120f#diff-942e078ea8b33787fb88d5ec489ca145R1)

@enebo
Copy link
Member

enebo commented Sep 17, 2014

Perhaps we can make a merge script which knows about which ext we implement? Not sure..just brainstorming :)

@eregon
Copy link
Member

eregon commented Sep 17, 2014

OK, seems https://github.com/jruby/jruby/blob/master/tool/sync_ruby is what I was looking for :)
And indeed in https://github.com/jruby/jruby/blob/master/tool/globals_2_1_0.rb we have that ext mapping.
It completely solves the problem as even if lib/pathname.rb is re-added it should be ignored.

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

No branches or pull requests

4 participants