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

Define packages for classes in nested JARs #3160

Merged
merged 2 commits into from
Jul 25, 2015

Conversation

jowl
Copy link
Contributor

@jowl jowl commented Jul 21, 2015

JRubyClassLoader doesn't define packages for classes found in JARs in JARs in JRuby 1.7 (it works as expected on master). The following example demonstrates the behavior:

mkdir -p src/{main,nested} bin build
echo "package main; class Main { }" > src/main/Main.java
echo "package nested; class Nested { }" > src/nested/Nested.java
javac -sourcepath src -d bin src/*/*.java
jar cf build/main.jar -C bin main
jar cf build/nested.jar -C bin nested
jar uf build/main.jar -C build nested.jar
ruby -rbuild/main -rnested -e'[Java::Main::Main, Java::Nested::Nested].each { |c| printf("%s.java_class.package # => %s\n", c.name, c.java_class.package) }'

In the example, a JAR containing the class main.Main and another JAR containing the class nested.Nested created. The Ruby script then loads these classes and prints out the package names; which is main for main.Main, but empty for nested.Nested.

Packages are defined in the private defineClass(String name, Resource res) of URLClassLoader, which is only called from findClass(final String name). Thus, whenever findClass() in URLClassLoader throws ClassNotFoundException (e.g. for JARs within JARs), the package doesn't get defined, even if findClass(String className) in JRubyClassLoader is able to find and define the sought class. I've used http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/net/URLClassLoader.java for reference.

This proposed solution is a simplified version of that in the URLClassLoader in that it doesn't care about versioning information and sealing. The reason for this is that I really don't know anything about it and URLClassLoader defaults to defining packages in the same manner whenever a manifest isn't available.

I didn't know whether to include the example above as an integration test example or not? If I would, I thought I'd add the JAR as a fixture and then simply assert on the package name. I'd really appreciate feedback on my code, as well as on the PR description, so that I can improve.

@kares
Copy link
Member

kares commented Jul 23, 2015

very interesting, I did not know overriding class-loader might need to call definePackage themselves.
vote for getting this piece in and possibly tuning it further so that the .jar's manifest is read and passed to definePackage if/when available ... as for tests that should be easy to add somewhere appropriate under test/jruby (there's already a .jar within .jar that can be re-used)

UPDATE: actually for jruby-1_7 the path is only test (those tests were moved under test/jruby on master) ...

@mkristian
Copy link
Member

+1 to get this in with a test

@jowl
Copy link
Contributor Author

jowl commented Jul 24, 2015

Great, thanks for the feedback. I tried to get the test suit running properly this morning, which ended in rvm implode since RVM seemed to mess with just about everything. Hopefully I'll have time to get this working tonight.

@jowl jowl force-pushed the define-package-in-nested-jars branch from cd3dbc7 to c0a9e8e Compare July 24, 2015 22:13
Joel Segerlind added 2 commits July 25, 2015 00:13
Packages are defined in the private `defineClass(String name, Resource res)` of `URLClassLoader`, which is only called from `findClass(final String name)`. Thus, whenever `findClass()` in `URLClassLoader` throws `ClassNotFoundException` (e.g. for JARs within JARs), the package doesn't get defined, even if `findClass(String className)` in `JRubyClassLoader` is able to find and define the sought class. I've used http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/java/net/URLClassLoader.java for reference.
The example would fail unless the package has been defined, as
JavaClass#package would return nil, which obviously doesn't define the
method #name.
@jowl
Copy link
Contributor Author

jowl commented Jul 24, 2015

I've finally managed to get (at least some of) the tests working and have added a test for defining packages for classes in nested JARs.

As for using the JARs manifest for definePackage, I think it sounds like a good idea but it would require a much larger addition:

  • One would have to enable getting hold of the manifest, probably through org.jruby.util.CompoundJarURLConnection somehow. Or directly from the JAR URL.
  • Packages should be verified with regards to sealing, and one would have to decide how to handle security violations (this is approx. 35 loc in URLClassLoader).

kares added a commit that referenced this pull request Jul 25, 2015
Define packages for classes in nested JARs
@kares kares merged commit 1512d9a into jruby:jruby-1_7 Jul 25, 2015
@kares
Copy link
Member

kares commented Jul 25, 2015

@jowl excellent, thanks ... maybe for your notes on what might be done further, please open a new issue

@mkristian
Copy link
Member

@kares I hope you know this is not required for jruby-9k. we improved the current situation already quite a bit with his patch. not sure we should invest further until there is real need, i.e. blocking people to use jruby-1.7.x

just my thoughts on this.

@kares
Copy link
Member

kares commented Jul 25, 2015

@mkristian yeah, thanks - just figured as I'm once again struggling through the merge :(

@kares kares removed the JRuby 9000 label Jul 25, 2015
@jowl
Copy link
Contributor Author

jowl commented Jul 25, 2015

@kares sorry about not being clear about this not being required in 9k in the description and not noticing the JRuby 9000 label. Given @mkristian's input, I won't pursue this any further now. Thanks guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants