Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rubinius load file twice when first time developer require with .rb suffix #1832

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

LTe commented Jul 26, 2012

Before patch Rubinius::CodeLoader#update_paths set @feature to as file

def update_paths(file, path)

...

@feature = file

...

end

Thats why it was possible to double load the file

require("file.rb") # => true
require("file") # => true

In this way, the interpreter thinks that in LOAD_FEATURES there is no such file because the loader does not extend to the path

Related to #1571

@dbussink dbussink and 1 other commented on an outdated diff Jul 26, 2012

spec/ruby/core/kernel/shared/require.rb
@@ -244,6 +244,14 @@
ScratchPad.recorded.should == [:loaded]
end
+ it "does not load twice the same file" do
+ $LOAD_PATH << CODE_LOADING_DIR
+ path = File.expand_path "load_fixture", CODE_LOADING_DIR
+ File.exists?(path).should be_true
@dbussink

dbussink Jul 26, 2012

Owner

Any reason you added these specs? It's fine that the specs just throw an exception if a file doesn't exist or anything. This isn't really the behavior we're testing here, so I would leave those cases out and just do the two require assertions.

@LTe

LTe Jul 26, 2012

Contributor

I will delete that

Owner

dbussink commented Jul 26, 2012

Isn't this also a fix for #1788?

If so, you could add Fixes #1788 and perhaps also the other issue to the commit message. Those issues will then be automatically closed and we have a reference to it in the commit logs.

Contributor

LTe commented Jul 26, 2012

@dbussink Hm... yes

Contributor

LTe commented Jul 26, 2012

@dbussink updated

This pull request fails (merged 9631d8c7 into 31cc036).

@dbussink dbussink and 1 other commented on an outdated diff Jul 26, 2012

spec/ruby/core/kernel/shared/require.rb
@@ -406,7 +412,7 @@
it "adds the suffix of the resolved filename" do
$LOAD_PATH << CODE_LOADING_DIR
@object.require("load_fixture").should be_true
- $LOADED_FEATURES.should == ["load_fixture.rb"]
+ $LOADED_FEATURES.should == [@path]
end
@dbussink

dbussink Jul 26, 2012

Owner

Any reason you changed this spec? The original one was much more clear to me as to what it means

@LTe

LTe Jul 26, 2012

Contributor

Now rubinius behaves exactly like MRI. In $LOADED_FEATURES we have full path to file.

@dbussink

dbussink Jul 26, 2012

Owner

Hmm, well, in 1.9 $LOADED_FEATURES has full paths, but in 1.8 mode it doesn't. I wonder if that would break stuff

@LTe

LTe Jul 26, 2012

Contributor

It is possible that it may break something

This pull request passes (merged c8fd2172 into 31cc036).

Owner

dbussink commented Jul 26, 2012

Ok, this changes the behavior on 1.8 so we shouldn't do that. The problem is that when we require 'config.rb', it adds './config.rb' to $LOADED_FEATURES instead of 'config.rb'. The fix therefore should fix that problem and not change the behavior like this.

LTe added some commits Jul 27, 2012

Ruby in 1.8 mode should not load file twice. Add spec for that.
Reproduce:
require('file.rb') # => true
require('file') # => true
Rubinius load file twice when first time developer require with .rb
Before patch when user require file with .rb suffix Rubinius recognize
file as :ruby type and execute method :verify_load_path from
Rubinius::CodeLoader. In this method interpreter interate via $LOAD_PATH
and search for proper file. After that Rubinius add to $LOADED_FEATURES
relative path. But when developer require witout .rb suffix Rubinius
execute another method (because interpreter need guess extension).
Method without .rb suffix add to $LOADED_FEATURES only short file path.

Example

require 'ruby.rb'
$LOADED_FEATURES # => ["./ruby.rb"]

require 'ruby'
$LOAD_FEATURES # => ["ruby.rb"]

In this way, the interpreter can load file twice.

After patch:

require 'ruby.rb'
$LOADED_FEATURES # => ["ruby.rb"]

require 'ruby'
$LOAD_FEATURES # => ["ruby.rb"]

* Fixes #1788
* Fixes #1571

@dbussink dbussink closed this in c520d3b Jul 27, 2012

This pull request fails (merged 4ec23934 into f1d811f).

This pull request passes (merged 71a2efb into f1d811f).

Gibheer added a commit to Gibheer/rubinius that referenced this pull request Aug 14, 2012

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