Test load service #2606
Test load service #2606
Conversation
| @@ -133,6 +132,20 @@ private FoundLibrary findResourceLibrary(String baseName, String suffix) { | |||
| return findFileResource(baseName, suffix); | |||
| } | |||
|
|
|||
| // ruby does not load a relative path unless the current working directory is in $LOAD_PATH | |||
| if (baseName.startsWith("./") || loadService.loadPath.contains(".")) { | |||
There was a problem hiding this comment.
Is there a test that requires to skip ./ check if . is in load path? This seems rather strange to me, since I thought MRI treats ./ as a pretty special file index. Also, MRI (2.1.2 at least) seems to bypass LOAD_PATH, even if it contains .:
$ ruby -Ilib:. -e 'require "./foo"'
local
(the lib contains another foo.rb that would have printed 'in lib')
There was a problem hiding this comment.
./ just bypasses the LOAD_PATH - yes
but if I do not find it with "." on the findFileResourceWithLoadPath then the classloader will find it. since the application classloader has the PWD (the one from the java world) on its classpath.
so the test of "." is needed to not fall into the classloader search and find it there. and the classloader finds always come with a "uri:classloader:" prefix which is not wrong but strange for files from the filesystem.
mkristian
force-pushed
the
test-load-service
branch
2 times, most recently
from
5dbea53
to
38f3892
Compare
|
@ratnikov your suspicion was correct: and all tests pass :) but now I can I will give it another trial and somehow add a test to it. the current test are failing since they do something like and here the PWD from ruby is different from java (which does not change to 'test') |
mkristian
force-pushed
the
test-load-service
branch
4 times, most recently
from
3b9f618
to
ea0194f
Compare
|
the last failing test is "strange" - this one fails and which is the same execution used by mvn -Prake ... but this one without the rakd_test_loader succeeds: anyways IMO not including the new load-service in an early state of a new major version makes it hard to switch any time later since we know people are OK with regression when jumping to a new major release but can be very unhappy wirh regressions on later point releases ;) to I would drop the last commit and move forward |
mkristian
force-pushed
the
test-load-service
branch
from
f5d02ae
to
809eae8
Compare
and let the protocol classpath: search on uri:classloader:/ or on LOAD_PATH
there is an extra search on "." necessary otherwise the relative path will be found via the classloader. the regular mri spec does not fail since current directory in java is independent from the current directory in ruby
old load-service will find it. new load-service will not find it.
mkristian
force-pushed
the
test-load-service
branch
from
809eae8
to
0ee53c5
Compare
|
@mkristian It looks like this was not really merged, so I'm marking as Invalid/Duplicate. |
this adds the missing pieces to avoid the legacy load-service.
the commit with passing the runtime to URLResource is actually not related to the load-service and will fix existing problems.
@ratnikov maybe you have some idea to avoid the double lookup on "." when it is also part of $LOAD_PATH:
jruby/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
Line 135 in a916d45
@headius I reverted the fix you "readded" to make jruby-openssl work on jruby-9k since this is not needed anymore for jruby-openssl-0.9.6. but let me know if support for jruby-openssl-0.9.5 is needed and I will revert the revert.
without legacy load-service there is no need for the https://github.com/jruby/jruby/blob/a916d4563c16b317ccc409dcd12c9a09aede5d85/core/src/main/java/org/jruby/util/CompoundJarURLStreamHandler.java which seems to cause some problems with newer JDK security permissions - see jruby/jruby-openssl#29
before merging I would set the default to use the legacy load-service - to stay as is. but for running travis I used the oposite default.