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

extensions found on classpath do not load #2055

Closed
mkristian opened this Issue Oct 19, 2014 · 10 comments

Comments

Projects
None yet
4 participants
@mkristian
Copy link
Member

mkristian commented Oct 19, 2014

$ bin/jruby -J-cp lib/ruby/gems/shared/gems/thread_safe-0.3.4-java/lib/ -e "require 'thread_safe/cache'"
NameError: uninitialized constant ThreadSafe::JRubyCacheBackend
  const_missing at org/jruby/RubyModule.java:2723
     ThreadSafe at /home/christian/projects/active/maven/jruby17/lib/ruby/gems/shared/gems/thread_safe-0.3.4-java/lib/thread_safe/cache.rb:12
         (root) at /home/christian/projects/active/maven/jruby17/lib/ruby/gems/shared/gems/thread_safe-0.3.4-java/lib/thread_safe/cache.rb:3
        require at org/jruby/RubyKernel.java:1071
         (root) at /home/christian/projects/active/maven/jruby17/lib/ruby/shared/rubygems/core_ext/kernel_require.rb:1
         (root) at -e:1

without the extra classpath it works:

$ bin/jruby -e "require 'thread_safe/cache'"

looking the the debug.loadService it show the jar with the extension is found and loaded BUT the jar-extension is not activated.

this is partly a blocker for: sonatype/nexus-oss#802

the nexus-ruby-plugin are just few ruby files and a few gems but both ScriptingContainer and OSGiScriptingContainer can not handle embedded gems.

so I did embed "lib" directories of the gems into the root of the jar, avoiding rubygems altogether. this works until jruby-1.7.12 for running the tests using a regular ScriptingContainer, or running the plugin with the old nexus-oss-2.6.x server or from nexus-oss-2.7.x onwards. nexus-oss-2.7.x uses felix-4.2.x.

this hack with the lib directories does not work with all gems and some "upgrade" was needed to get this trick working. (backports gem does use Directory globs which do not work with those ScriptingContainers).

the IsolatedScriptingContainer has a few flaws which prevents it use - though the pax testcases from maven/jruby/src/it and maven/jruby-complete/src/it do all work.

@headius

This comment has been minimized.

Copy link
Member

headius commented Dec 4, 2014

I guess this indicates that the classpath phase of loading does not do the *Service logic found in ExtensionSearcher/ClassExtensionLibrary. Looking into that.

@headius

This comment has been minimized.

Copy link
Member

headius commented Dec 4, 2014

I have a possible fix, but I feel like I'm adding more hacks on top of @ratnikov's hack.

diff --git a/core/src/main/java/org/jruby/runtime/load/LoadService.java b/core/src/main/java/org/jruby/runtime/load/LoadService.java
index 92b154a..650c524 100644
--- a/core/src/main/java/org/jruby/runtime/load/LoadService.java
+++ b/core/src/main/java/org/jruby/runtime/load/LoadService.java
@@ -1006,8 +1006,10 @@ public class LoadService {
         // TODO(ratnikov): Remove the special classpath case by introducing a classpath file resource
         Library library = findLibraryWithClassloaders(state, state.searchFile, state.suffixType);
         if (library != null) {
             state.library = library;
+            // normal search sequence would continue on to ExtensionSearcher, so we do that here manually
+            new ExtensionSearcher().trySearch(state);
         }
         return library;
     }
@mkristian

This comment has been minimized.

Copy link
Member Author

mkristian commented Dec 4, 2014

hmm - this means once we do not need findLibraryWithClassloaders(state, state.searchFile, state.suffixType) anymore this fix stops working.

I still have a local branch which does not need findLibraryWithClassloaders
anymore and master is "very close to this branch" already.

BTW my last URLResource fix allows me to use "normal" embedded gems with
OSGi containers. so I do not need this to work to switch from jruby-1.7.12
to 1.7.17 for the sonatype nexus-server.

I can look and find the right place in LibrarySearcher and propose a patch
tomorrow.

@headius

This comment has been minimized.

Copy link
Member

headius commented Dec 4, 2014

There's a danger of this slipping out of 1.17 if we don't have a fix today or tomorrow, FYI.

@mkristian

This comment has been minimized.

Copy link
Member Author

mkristian commented Dec 5, 2014

@headius @ratnikov
that would be my proposal:

diff --git a/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java b/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
index 273467a..e733398 100644
--- a/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
+++ b/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
@@ -150,8 +150,10 @@ class LibrarySearcher {
         }

         try {
+            FoundLibrary library = findFileResourceWithLoadPath(baseName, suffix, "uri:classloader:/");
+            if (library != null) return library;
             for (IRubyObject loadPathEntry : loadService.loadPath.toJavaArray()) {
-                FoundLibrary library = findFileResourceWithLoadPath(baseName, suffix, getPath(loadPathEntry));
+                library = findFileResourceWithLoadPath(baseName, suffix, getPath(loadPathEntry));
                 if (library != null) return library;
             }
         } catch (Throwable t) {
@mkristian

This comment has been minimized.

Copy link
Member Author

mkristian commented Dec 5, 2014

my proposal has one short coming: uri:classloader:/ means Thread.currentThread().getContextClassLoader() which is not always the classloader which loaded jruby (OSGi, etc). but it fixes my "testcase" from above.

#2278 would remove this short coming.

@mkristian

This comment has been minimized.

Copy link
Member Author

mkristian commented Dec 5, 2014

next step to handle classpath: via uri:classloader:

diff --git a/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java b/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
index 273467a..10102a4 100644
--- a/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
+++ b/core/src/main/java/org/jruby/runtime/load/LibrarySearcher.java
@@ -150,8 +150,12 @@ class LibrarySearcher {
         }

         try {
+            // search 'classpath:'-uri on uri:classloader:/
+            if (baseName.startsWith("classpath:")) baseName = baseName.replaceFirst("^classpath:/?", "");
+            FoundLibrary library = findFileResourceWithLoadPath(baseName, suffix, "uri:classloader:/");
+            if (library != null) return library;
             for (IRubyObject loadPathEntry : loadService.loadPath.toJavaArray()) {
-                FoundLibrary library = findFileResourceWithLoadPath(baseName, suffix, getPath(loadPathEntry));
+                library = findFileResourceWithLoadPath(baseName, suffix, getPath(loadPathEntry));
                 if (library != null) return library;
             }
         } catch (Throwable t) {

this is extra replace is enough to fix #1875 - I did not understood the test-case in a46ce8e since there is no file with the filename 'FILE.rb'

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Dec 5, 2014

I am trying to understand the risk scenarios of the change. It sounds like it fixes something significant but it also sounds like some unusual use of uri:classloader: will break? If it is so unusual no one should have been using it then that sounds like it might be fine. I am just having a hard time wrapping my head around the change and how it can potentially break older stuff.

@mkristian

This comment has been minimized.

Copy link
Member Author

mkristian commented Dec 5, 2014

to be on the save side: take @headius patch for jruby-1_7 and mine for master.

I doubt uri:classloader: breaks something, those jar artifact tests (ant `main-extended``` etc) use it "uri:classloader://META-INF/jruby.home" for jruby home and those j2ee and osgi tests add explicitly "uri:classloader:/" to the load path. so it has some testing for some time.

@ratnikov

This comment has been minimized.

Copy link
Contributor

ratnikov commented Dec 5, 2014

I've added a pull request to fix this: findLibraryWithClassloaders eventually creates a JarredScript for the Jar, so I've made sure that JarredScript loads associated extension if it exists.

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.