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

Symlinks in load path should remain unexpanded in LOADED_FEATURES #1941

Closed
headius opened this Issue Sep 2, 2014 · 11 comments

Comments

Projects
None yet
4 participants
@headius
Copy link
Member

commented Sep 2, 2014

This is related to #1940.

When a load path contains an entry with an embedded symlink, JRuby will canonicalize LOADED_FEATURES entries through that symlink. This can lead to issues similar to bundler/bundler#3164, though that specific issue appears to be fixed by avoiding canonicalization of FILE (#1940).

We should try to iron out exactly how and when MRI expands absolute paths and canonicalizes through symlinks for JRuby 1.7.16, so we can put this to rest. We also need tests or specs for all permutations, because both MRI's suite and RubySpec are weak here.

@headius headius added the core label Sep 2, 2014

@headius headius added this to the JRuby 1.7.16 milestone Sep 2, 2014

headius added a commit that referenced this issue Sep 2, 2014

@headius

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2014

This isn't going to make 1.7.16. I started looking into the logic in MRI, and it's enormous and very dense.

@headius headius modified the milestones: JRuby 1.7.17, JRuby 1.7.16 Sep 24, 2014

@enebo

This comment has been minimized.

Copy link
Member

commented Sep 24, 2014

@headius Did you happen to notice this difference or did this break something?

@camlow325

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2014

We have run into a couple of cases where the differences in how $LOADED_FEATURES works in MRI Ruby 1.9.3 vs. JRuby 1.7.16 under Ruby 1.9 has caused problems for the puppet-server project, https://github.com/puppetlabs/puppet-server.

In Puppet Ruby code, we have some logic which manipulates the $LOADED_FEATURES array directly in some cases where we need to manually manage the load process. The differences in MRI Ruby vs. JRuby interpretation of that array when a "require" is performed causes code to be errantly to be reloaded, which is problematic for us. Here are the cases we've seen:

  • We add files that can be resolved relative to one of the entries in the Ruby library load-path. For example, if the Ruby load-path includes "/my/full/pathlib" and a file on disk called "/my/full/pathlib/somedir/somefile.rb" exists, we add "somedir/somefile.rb" into the $LOADED_FEATURES array. Under MRI Ruby, a later "require" call for "somedir/somefile" will not cause the "somefile.rb" file to be reloaded. Under JRuby, however, the file will be reloaded.
  • We may add a file to the $LOADED_FEATURES array that represents a symlink. For example, we may have a symlink at "/my/sympath/thefile.rb" that points to "/my/realpath/thefile.rb". If "/my/sympath/thefile.rb" were added to the $LOADED_FEATURES array, a subsequent "require" referencing "realpath/thefile" would not cause "thefile.rb" to be reloaded under MRI Ruby but would cause "thefile.rb" to be reloaded under JRuby.

Can you confirm that the issues we found above are the same as what this issue is covering? If you think there's anything new here that warrants a separate issue, I'd be happy to create a new issue to cover that.

@headius

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2014

@camlow325 That does sound like the same issues. I'm not sure if we're going to be able to get this in for 1.7.17, given that we've already delayed that release too long.

Would it be possible for you to come up with some test cases for the issues you've seen? It would provide a good starting point for fixing them.

@headius

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2014

@ratnikov Actually, this might be something you'd be able to fix up quickly, since you've reworked so much of the loading logic. Or perhaps you can just point us (me and others looking at this bug) at the right place where LOADED_FEATURES is populated.

@ratnikov

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2014

@headius, Is this comment accurate: https://github.com/jruby/jruby/blob/jruby-1_7/core/src/main/java/org/jruby/util/RegularFileResource.java#L43 ? Or does it apply only to files, and not LOADED_FEATURES?

@headius headius modified the milestones: JRuby 1.7.17, JRuby 1.7.18 Dec 8, 2014

@headius

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2014

@ratnikov I believe it only applies to files. It's pretty easy to show that MRI does absolutize but does not canonicalize features with symlinks. Note here I'm using the JDK definition of absolute and canonical, where canonical means to expand all symlinks.

~/projects/jruby-1.7 $ ls -ld tmp foo
lrwxr-xr-x  1 headius  staff    3 Dec  8 14:06 foo -> tmp
drwxr-xr-x  3 headius  staff  102 Dec  8 14:06 tmp

~/projects/jruby-1.7 $ ls -l tmp
total 0
-rw-r--r--  1 headius  staff  0 Dec  8 14:06 bar.rb

~/projects/jruby-1.7 $ jruby -e 'require "./foo/bar.rb"; p $LOADED_FEATURES[-1]'
"/Users/headius/projects/jruby-1.7/tmp/bar.rb"

~/projects/jruby-1.7 $ rvm ruby-1.9 do ruby -e 'require "./foo/bar.rb"; p $LOADED_FEATURES[-1]'
"/Users/headius/projects/jruby-1.7/foo/bar.rb"

May just require switching a call...I will try it, but I have bumped this to 1.7.18 so we can let the change bake for a couple weeks.

@headius

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2014

@ratnikov et al: I have committed the fix in 6776f0b and it seems to pass MRI tests ok. I have not tried RubySpec, but I don't expect to see failures there either.

I probably should add a test case.

@headius

This comment has been minimized.

Copy link
Member Author

commented Dec 10, 2014

Regression spec added in 4c3fa76. Not sure why it didn't link to this issue.

@camlow325

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2015

@headius Sorry for the late reply back on this one. Unfortunately, the fix applied in JRuby 1.7.18 didn't appear to address the Puppet require reloading issue that I'd mentioned earlier in this thread. I submitted a new issue, #2477, with a more specific example to demonstrate the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.