[BEAM-2530] Make classloader handling more compatible with JDK 9 #4497
Conversation
| @@ -37,6 +39,11 @@ | |||
| * @return A list of absolute paths to the resources the class loader uses. | |||
| */ | |||
| public static List<String> detectClassPathResourcesToStage(ClassLoader classLoader) { | |||
| if (classLoader == ClassLoader.getSystemClassLoader()) { | |||
There was a problem hiding this comment.
this logic https://github.com/apache/geronimo-xbean/blob/trunk/xbean-finder/src/main/java/org/apache/xbean/finder/ClassLoaders.java#L38 can be beneficial. High level idea is to use getResources() to determine what is in the classloader.
There was a problem hiding this comment.
Do you find in practice that all Java jars have a META-INF folder?
Also, it doesn't seem that getResources guarantees to return the same order of jars as specified on the classpath.
There was a problem hiding this comment.
That isn't guaranteed to work for all jars - jar M will create an archive that doesn't contain a META-INF folder.
There was a problem hiding this comment.
This is why it is not the only strategy but likely the most portable i saw accross years. In particular in the context of this method. Do you expect all jars to be in the cp string? Not using manifest to load jars ;)
There was a problem hiding this comment.
Neither approach is going to work in all cases, but I've had more success with this one.
There was a problem hiding this comment.
Did you test this class? getResources("") - not only metainf -
works on most jvm and supports all cases compared to classpath scanning which doesnt support a lot of bootstraps except plain mains :(.
There was a problem hiding this comment.
I have tested this logic, but I haven't been able to test this specific usage of it. I ran into a similar problem I'm fixing in GoogleCloudPlatform/DataflowJavaSDK#621 and thought it was worth fixing here too.
If you'd like to defer until we're sure it's a problem, or implement the getResources approach instead, that's fine with me :)
There was a problem hiding this comment.
I think this approach seems to solve a legitimate issue and other approaches can be implemented and added.
If you have a good idea on how to test this effectively since its easy to break this kind of functionality without knowing it.
There was a problem hiding this comment.
to share the idea here what i had in mind high level #4514
cushon
changed the title
From http://www.oracle.com/technetwork/java/javase/9-relnote-issues-3704069.html: > The application class loader is no longer an instance of > java.net.URLClassLoader (an implementation detail that was never > specified in previous releases). Code that assumes that > ClassLoader::getSytemClassLoader returns a URLClassLoader object will > need to be updated. Note that Java SE and the JDK do not provide an > API for applications or libraries to dynamically augment the class > path at run-time.
|
I brought this discussion back to the dev@ to choose between PR/4497 or PR/4514 or support both. |
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
From
http://www.oracle.com/technetwork/java/javase/9-relnote-issues-3704069.html:
Follow this checklist to help us incorporate your contribution quickly and easily:
[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.