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

load current directory file #3126

Closed
yamam opened this Issue Jul 9, 2015 · 12 comments

Comments

Projects
None yet
6 participants
@yamam

yamam commented Jul 9, 2015

jruby 9k rc2 can't load a file in current directory.

$ touch test.rb

# jruby 9k rc2
$ jruby -e 'load "test.rb" ; puts "OK"'
LoadError: no such file to load -- test.rb
   load at org/jruby/RubyKernel.java:958

# jruby 9k rc1
$ jruby -e 'load "test.rb" ; puts "OK"'
OK

# MRI
$ ruby -e 'load "test.rb" ; puts "OK"'
OK
@mkristian

This comment has been minimized.

Show comment
Hide comment
@mkristian

mkristian Jul 10, 2015

Member

oho

$ ruby -v -e 'load "test.rb" ; puts "OK"'
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]
OK

but

$ ruby -v -e 'require "test.rb" ; puts "OK"'
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]
/home/christian/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in
`require': cannot load such file -- test.rb (LoadError)
from
/home/christian/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in
`require'
from -e:1:in `<main>'

the latter is wanted. the first one not working with jruby is a bug.

Member

mkristian commented Jul 10, 2015

oho

$ ruby -v -e 'load "test.rb" ; puts "OK"'
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]
OK

but

$ ruby -v -e 'require "test.rb" ; puts "OK"'
ruby 2.1.5p273 (2014-11-13 revision 48405) [x86_64-linux]
/home/christian/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in
`require': cannot load such file -- test.rb (LoadError)
from
/home/christian/.rbenv/versions/2.1.5/lib/ruby/2.1.0/rubygems/core_ext/kernel_require.rb:55:in
`require'
from -e:1:in `<main>'

the latter is wanted. the first one not working with jruby is a bug.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 13, 2015

Member

That's weird. I'm not sure why MRI loads from CWD for "load" but not for "require" we need to clarify this behavior with MRI.

Member

headius commented Jul 13, 2015

That's weird. I'm not sure why MRI loads from CWD for "load" but not for "require" we need to clarify this behavior with MRI.

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans Jul 14, 2015

Contributor

This breaks Sequel's specs (and many uses of Sequel migrations). The historical and I believe expected behavior for all ruby versions (and JRuby versions prior to rc2) is for load to directly load the filename given (equivalent to eval File.read(filename)). The reason require behaves differently for relative paths is that it uses $: in that case, which load does not. For similar reasons, require doesn't require specifying file extensions, whereas load requires the full path.

Contributor

jeremyevans commented Jul 14, 2015

This breaks Sequel's specs (and many uses of Sequel migrations). The historical and I believe expected behavior for all ruby versions (and JRuby versions prior to rc2) is for load to directly load the filename given (equivalent to eval File.read(filename)). The reason require behaves differently for relative paths is that it uses $: in that case, which load does not. For similar reasons, require doesn't require specifying file extensions, whereas load requires the full path.

@enebo enebo added the JRuby 9000 label Jul 14, 2015

@enebo enebo modified the milestone: JRuby 9.0.0.0 Jul 14, 2015

@donv

This comment has been minimized.

Show comment
Hide comment
@donv

donv Jul 15, 2015

Member

This is a pretty serious bug that is likely to occur in many apps. It currently breaks our Capistrano deployment. Will it force a JRuby 9.0.0.0.rc3 release?

Member

donv commented Jul 15, 2015

This is a pretty serious bug that is likely to occur in many apps. It currently breaks our Capistrano deployment. Will it force a JRuby 9.0.0.0.rc3 release?

@enebo enebo added this to the JRuby 9.0.0.0 milestone Jul 15, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 15, 2015

Member

@donv @jeremyevans I think we will just make sure both your use cases work and still try for final. So long as we don't realize the semantics are weirder than reported...

Member

enebo commented Jul 15, 2015

@donv @jeremyevans I think we will just make sure both your use cases work and still try for final. So long as we don't realize the semantics are weirder than reported...

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 16, 2015

Member

wowsers. The semantics of this are interesting and it reflects yet another hard to believe lack of understanding on my part within one week (to_ary/to_a in masgn being the other -- love random trivia in an issue comment). If I have CWD like this:

a.rb
hoh/a.rb

Invocations of MRI:

# full paths are full paths
mri22 -Ihoh -e 'load "/Users/enebo/work/snippets/a.rb"'   # a.rb loaded
mri22 -Ihoh -e 'load "./a.rb"' # a.rb loaded

# aribtrary named file
mri22 -Ihoh -e 'load "a.rb"'  # hoh/a.rb loaded
mri22 -e 'load "a.rb"' # a.rb loaded

So if if it a qualified path we load that. If it is an unqualified path we try to load from LOAD_PATH and failing that we load from CWD.

Member

enebo commented Jul 16, 2015

wowsers. The semantics of this are interesting and it reflects yet another hard to believe lack of understanding on my part within one week (to_ary/to_a in masgn being the other -- love random trivia in an issue comment). If I have CWD like this:

a.rb
hoh/a.rb

Invocations of MRI:

# full paths are full paths
mri22 -Ihoh -e 'load "/Users/enebo/work/snippets/a.rb"'   # a.rb loaded
mri22 -Ihoh -e 'load "./a.rb"' # a.rb loaded

# aribtrary named file
mri22 -Ihoh -e 'load "a.rb"'  # hoh/a.rb loaded
mri22 -e 'load "a.rb"' # a.rb loaded

So if if it a qualified path we load that. If it is an unqualified path we try to load from LOAD_PATH and failing that we load from CWD.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

I believe this logic is in rb_f_load in MRI; it attempts to find the file using load path and typical require semantics, and if that fails it tries to blindly open the filename as-is:

    path = rb_find_file(fname);
    if (!path) {
        if (!rb_file_load_ok(RSTRING_PTR(fname)))
            load_failed(orig_fname);
        path = fname;
    }
    rb_load_internal(path, RTEST(wrap));

Seems simple enough to add to our load logic, yes?

Member

headius commented Jul 16, 2015

I believe this logic is in rb_f_load in MRI; it attempts to find the file using load path and typical require semantics, and if that fails it tries to blindly open the filename as-is:

    path = rb_find_file(fname);
    if (!path) {
        if (!rb_file_load_ok(RSTRING_PTR(fname)))
            load_failed(orig_fname);
        path = fname;
    }
    rb_load_internal(path, RTEST(wrap));

Seems simple enough to add to our load logic, yes?

@enebo enebo closed this in 2366376 Jul 16, 2015

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 16, 2015

Member

@headius please review this for me. I think it is what we discovered this morning but the actual code and where I injected this potentially could be wrong.

@donv @jeremyevans could I trouble you guys to test HEAD to make sure you see no more problems. This turns out to be not too risky because it comes after normal load semantics and MRI definitely has this additional check so we now line up with it.

Member

enebo commented Jul 16, 2015

@headius please review this for me. I think it is what we discovered this morning but the actual code and where I injected this potentially could be wrong.

@donv @jeremyevans could I trouble you guys to test HEAD to make sure you see no more problems. This turns out to be not too risky because it comes after normal load semantics and MRI definitely has this additional check so we now line up with it.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 16, 2015

Member

The fix looks right to me.

Member

headius commented Jul 16, 2015

The fix looks right to me.

@jeremyevans

This comment has been minimized.

Show comment
Hide comment
@jeremyevans

jeremyevans Jul 16, 2015

Contributor

I'm currently at an OpenBSD hackathon, and won't be able to test this until mid-next week. I don't ever build JRuby from source, but I suppose I could try jruby-head in RVM in a VM next week and see if it works there.

Contributor

jeremyevans commented Jul 16, 2015

I'm currently at an OpenBSD hackathon, and won't be able to test this until mid-next week. I don't ever build JRuby from source, but I suppose I could try jruby-head in RVM in a VM next week and see if it works there.

@donv

This comment has been minimized.

Show comment
Hide comment
@donv

donv Jul 16, 2015

Member

My use case with Capistrano that failed earlier work fine now. 😄 👍

Member

donv commented Jul 16, 2015

My use case with Capistrano that failed earlier work fine now. 😄 👍

@enebo

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Jul 16, 2015

Member

@jeremyevans I have sequel cloned and I was able to get a full green run doing a 'rake spec' with HEAD. I confirmed it was broken before this fix so I think we are golden.

Member

enebo commented Jul 16, 2015

@jeremyevans I have sequel cloned and I was able to get a full green run doing a 'rake spec' with HEAD. I confirmed it was broken before this fix so I think we are golden.

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