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

Fix TestLoad#test_symlinked_jar #5149

Closed
wants to merge 1 commit into from

Conversation

ChrisBr
Copy link
Contributor

@ChrisBr ChrisBr commented Apr 23, 2018

Introduced in #5121.
Ruby 2.5 loads now the real path of symlinked libraries.
We will now use the file extension of the symlink
to decide what ResourceLibrary to load.

Introduced in jruby#5121.
Ruby 2.5 loads now the real path of symlinked libraries.
We will now use the file extension of the symlink
to decide what ResourceLibrary to load.
@headius
Copy link
Member

headius commented Apr 23, 2018

We'll let this one finish just to check that test:jruby goes green. It should be green if not for the introduced failure. Thank you for following through on this!

@headius headius added this to the JRuby 9.2.0.0 milestone Apr 23, 2018
@ChrisBr
Copy link
Contributor Author

ChrisBr commented Apr 23, 2018

@headius the only difference is now that we pass the resource and not the expanded resource to ResourceLibrary.create(searchName, scriptName, resource) which decides which ResourceLibrary gets created based on the file extension.

However, this will now always use the file extension of the symlink (the extension of the expanded symlink gets always ignored). Is this what we want?

@headius
Copy link
Member

headius commented Apr 23, 2018

@ChrisBr That seems to be the MRI behavior, if the tests are to be trusted. As long as this fixes the regression and doesn't introduce anything else, we'll go with it.

I'm sure the truth is somewhere in the MRI load/require code, but that's a really tangled mess.

@ChrisBr
Copy link
Contributor Author

ChrisBr commented Apr 23, 2018

After talking with @headius, I double checked the behavior in MRI and it seems to be undefined:

When I create a symlink sym.so to nokogiri.so I get this exception

/tmp/ruby-test/sym.so: undefined symbol: Init_sym - /tmp/ruby-test/sym.so

The same happens also without file extension.

It seems like the current behavior is correct, so we can close this PR for now and need to double check with the MRI team.

@ChrisBr ChrisBr closed this Apr 23, 2018
@headius
Copy link
Member

headius commented Apr 23, 2018

I will modify the JRuby test, since it doesn't appear that MRI traverses symlinks for "extensions" like our .jar, which is the only failing test that regressed.

@headius
Copy link
Member

headius commented Apr 24, 2018

@ChrisBr Actually, after thinking about it a bit more, I cherry-picked your additional fix to master. The test is testing a valid behavior, in my opinion, even if using the extension in this order is not really specified (nor is being able to load a symlinked "extension"). In addition, the jar files we load are not necessarily JRuby extensions; they may simply be jar files containing a Java library, in which case we should also preserve the previous behavior.

Cherry-picked to master in 4b347a4.

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

Successfully merging this pull request may close these issues.

None yet

2 participants