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

JRubyClassloader seems to have a problem with file urls pointing to jar ... #1852

Merged
merged 1 commit into from Jul 23, 2014

Conversation

Projects
None yet
4 participants
@donv
Copy link
Member

donv commented Jul 23, 2014

...within a jar. converting them into jar:file: do work - fixes #1850

JRubyClassloader seems to have a problem with file urls pointing to j…
…ar within a jar. converting them into jar:file: do work - fixes #1850
@donv

This comment has been minimized.

Copy link
Member Author

donv commented Jul 23, 2014

This brings us closer to green tests.

donv added a commit that referenced this pull request Jul 23, 2014

Merge pull request #1852 from jruby/test-loading-embedded-jars-from-jars
JRubyClassloader seems to have a problem with file urls pointing to jar ...

@donv donv merged commit 30af4a3 into jruby-1_7 Jul 23, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details

@donv donv deleted the test-loading-embedded-jars-from-jars branch Jul 23, 2014

@donv donv added this to the JRuby 1.7.14 milestone Jul 23, 2014

@donv donv self-assigned this Jul 23, 2014

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jul 23, 2014

This patch looks wrong to me. At least it looks wrong that if at this point the URL in question can ever start with jar:. If so then this patch will make a URL like 'jar:jar:file:...'. Perhaps this cannot happen here?

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 23, 2014

new File( location ).toURI.toURL is always a file:// url

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 23, 2014

oh - well I could be wrong ;)

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jul 23, 2014

Well that makes sense to me :) Thanks for clarification.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

actually looking a bit more around the code. the patch seems totally wrong in the sense that the filename needs to be tested whether it has some "character sequence" which indicates a jar file entry.

the LibrarySearcher.ResourceLibrary takes FileResource and there are already a few subclasses of it, like JarFileResource. the only missing piece would be a getURI method on the FileResource interface and then whole loadJar method would just use the url.uri from the FileResource.

indeed the FileResource in question is a JarFileResource. happy to provide patch for the URI with "tests" ;)

@enebo

This comment has been minimized.

Copy link
Member

enebo commented Jul 24, 2014

@mkristian can you ask dfr about this on irc? This is pretty clearly in his zone now as he made all the *Resources.

One of my 'this seems wrong 'feelings was wondering what happens if you open a file called 'exciting!'. Making this scheme resolution part of the resource types gets rid of that question regardless of whether URI somehow appropriately escapes a non-jar ! or whatever.

@ratnikov

This comment has been minimized.

Copy link
Contributor

ratnikov commented Jul 24, 2014

From IRC discussion:

I'm reluctant to add getURI on FileResource, since FileResource is meant to abstract away ruby file access and getURI is a very load-service specific thing that is only required when trying to load jars.

Seems like a better solution would be:

  1. make sure FileResource.absolutePath retains the schema. That way we should be able to just do new URI(resource.absolutePath()) and if it used to refer to a jar entry, as long it's prefixed with jar:file: it should automatically work.
  2. The problem isthat JRuby is permissive to sloppy uri definitions like file:foo/bar.jar!/zeta.rb which is technically incorrect. I think it's reasonable for JarResource to still accept such urls but prefix them with jar so that it becomes: jar:file:foo/bar.jar!/zeta.rb. This may be confusing to the user: File.new("file:/foo/bar.jar!/zeta.rb").path #=> "jar:file:/foo/bar.jar!/zeta.rb" but I think it's technically more correct.

Comments?

@donv

This comment has been minimized.

Copy link
Member Author

donv commented Jul 24, 2014

@ratnikov : Your points seem right to me. Allowing the sloppy URLs and converting to strict should keep compatibility and encourage correct use.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

ad 2) personally I would never needed use such construct - usually I run my code against MRI and JRuby ;) so looks like convenient feature ! internally we need to be strict since it we might need to convert to an URI object to use it.

ad 1) not sure absoluePath does not sound like something with scheme prefixed and for me sounds wrong. it always possible to hide getURI behind a second interface. and it could ensure that the URI is already valid inside the ExtendedFileResource and is only parsed once !

but I can live with absolutePath as well, just does not look nice

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

I guess what I try to say is File.new( "foo.rb" ).path #=> "file:foo.rb" looks strange. or is the scheme part on "some" file objects ?

@ratnikov

This comment has been minimized.

Copy link
Contributor

ratnikov commented Jul 24, 2014

Since "foo.jar" is a regular file, File.new( "foo.jar" ).path #=> "foo.jar" because "foo.jar" is also a valid URI and I'm assuming it'll work for classloader. If it won't, assumign that a non-absolute URI is a file is probably okay.

My comment was more about things which always have a URI, such as file:/... or things within jars, etc.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

the more I think about the more it feels wrong:
File.new( "pom.xml" ).path => "file:pom.xml"
on MRI it is
File.new( "pom.xml" ).path => "pom.xml"

but to load jar file into the JRubyClassloader you need in RegularFileResource#absolutePath the scheme part so require 'my.jar' works. but for File.new( 'my.jar').path you want to act like MRI and have 'my.jar'

so the strict schema is ONLY needed for for jar-files jar:file:some.jar!/someother.jar, or file:my.jar or classpath:/some.jar so you can add it to the JRubyClassloader.

even stranger examples can be contructed with the JRuby only scheme classpath: which is not a registered protocol in the JVM.

really I am not sure about
File.new( 'classpath:/my.jar' ).path => 'jar:file:/home/some.jar!/my.jar' but only if classpath can find a jar otherwise you see something like
File.new( 'classpath:/my.jar' ).path => 'bundle:0.16.1/my.jar'
or whatever protocol the current classloader produces.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

java.net.URL.new 'my.jar' => Java::JavaNet::MalformedURLException: no protocol: my.jar

@ratnikov

This comment has been minimized.

Copy link
Contributor

ratnikov commented Jul 24, 2014

I was considering doing it more like:

URL url;
try {
URI uri = new URI(resource.absolutePath());
if (uri.isAbsolute()){
url = uri.toURL();
} else {
url = new File(uri);
}
} catch (URISyntaxException badUri){
// treat bad uris as files, I guess
url = new File(resource.absolutePath());
}

So for:

  • regular files no protocol gets appended, and they become non-absolute uris which gets treated as file.
  • for stuff in jar, JarResource would make sure absolutePath always returns jar:file:
  • classpath: are not supported right now anyway. =/

I'm also okay about having this exception for JRubyClassLoader. In fact, maybe it makes sense to add JRubyClassLoader.addJar(FileResource resource) method that will do all this evil dark magic.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 24, 2014

probably what I do not understand is this:

when you construct a FileResource the URI is at hand - no fancy if-else-logic. and FileResource even provides a getInputStream method, which basically direct or indirect uses the URI to get the stream. then it forgets about its universal-resource-identifier and later you what to pass a FileResource into the JRubyClassloader to retrieve the URI from the absolutePath info and this string must be somehow convertible to an URI.

anyways - I guess tomorrow I need to find out why classpath:/META-INF/jruby.home/ruby/shared/jopenssl.jar does not work anymore.

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 25, 2014

before I get the feeling that I am just not able to get my point understood I created a PR to demonstrate:

#1857

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 25, 2014

let's continue from there - maybe that is a starting point to meet each other @ratnikov

@mkristian

This comment has been minimized.

Copy link
Member

mkristian commented Jul 27, 2014

I "fixed" it with 3e88f44

the main reason here is that we still have plenty of issues hanging regarding some "embedded" jars not loaded with JRubyClassloader and debug.loadService says it found the jar and with some extra println you see that the new URL(...) gets passed into the classloader. so everything looks OK but still it does not work. one possible reason is something like this:

new URL(new URL("file:/s"), "b",new URLStreamHandler() {     
        protected URLConnection openConnection( URL u ) throws IOException
        {
            return new File( "something").toURI().toURL().openConnection();
        }})

i.e. new URL(url.toString).openStream != url.openStream

please if you have suggestions to improve this commit please tell me.

@enebo enebo modified the milestones: JRuby 1.7.14, JRuby 1.7.15 Aug 27, 2014

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.