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
base: master
from

Conversation

Projects
None yet
2 participants
@ChrisBr
Contributor

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.

Fix TestLoad#test_symlinked_jar
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.
@headius

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

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