(maint) Fix classpath stuff for Java 9 #99
Conversation
| [] | ||
| (let [classloader (.. Thread currentThread getContextClassLoader)] | ||
| (when-not (last (filter modifiable-classloader? (classloader-hierarchy classloader))) | ||
| (let [new-cl (clojure.lang.DynamicClassLoader. classloader)] |
There was a problem hiding this comment.
Is it okay to just use the clojure one for this? We could implement our own pretty easily but it seemed to have what was needed.
There was a problem hiding this comment.
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/DynamicClassLoader.java#L33-L38 isn't super readable, but it seems fine and hasn't been touched in years.
|
Commit description might be more useful if it said why we can't rely on having a modifiable classloader (even if it just says "because Java 9 no longer guarantees that" or something) |
| (classpath-urls (.getContextClassLoader (Thread/currentThread))))] | ||
| ;; Called for side effect of ensuring there's a modifiable classloader | ||
| ;; without needing to make that function public | ||
| (add-classpath "file:/nil") |
There was a problem hiding this comment.
This part seems weird. How does add-classpath get called otherwise in real use?
There was a problem hiding this comment.
It gets called on its own in trapperkeeper. It does not normally need to be called before calling the next line, but this test wants to use some url methods after with-additional-classpath-entries removes the temporary classloader it puts there, so this makes that possible. It's not entirely clean.
There was a problem hiding this comment.
the way this test works doesn't make that much sense to begin with
There was a problem hiding this comment.
cleaned this up so it's not confusing anymore
AppClassLoader is no longer a URLClassLoader in Java 9 so you can't tell it to add URLs anymore. Instead we can just add a classloader that we can modify when we need it.
The implementation of this test was not ideal, and made worse by Java 9 changes to classloaders. Instead of querying URLs, just add a resource to a directory that's not normally in the class path and test that we can resolve it when it's added and that we can't after it gets cleaned up.
|
CLA signed by all contributors. |
|
Can we add Java 9 to the test matrix? travis-ci/travis-ci#5520 (comment) seems to have something that works. |
Add java 9 and drop java 6.
We can't rely on having a modifiable classloader at all times, so make
sure one is put in place if needed.
Fixes #79