-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
[REPL] Stop old classpath calculation on the base classloader #4363
Conversation
f1a7916
to
6893731
Compare
6893731
to
a07c7a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some info in the commit message, e.g. "since the mechanism of the recursive classpath checking is intended for the "inner" scripting/REPL classloaders, and should not touch anything beyond."
@@ -139,17 +140,17 @@ private fun CompiledScript.makeClassLoaderFromDependencies(baseClassLoader: Clas | |||
val processedClasspathElements = mutableSetOf<URL>() | |||
fun recursiveClassPath(res: Sequence<URL>, classLoader: ClassLoader?): Sequence<URL> = | |||
when (classLoader) { | |||
null -> res | |||
null, baseClassLoader -> res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still process the null
case, e.g. generating an error if we've got null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it should be after baseClassLoader
check, since it could be null
, and it is a valid situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ligee I'm not sure that fallbackClassloader of DualClassLoader should necessarily have base classloader as a parent. Is it really so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I can see from usages, it is always a thread context classloader that is a base classloader at the same time, so there is no appropriate test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it should convert to the baseClassLoader, but since it is possible to replace it, it is safer to leave the code as you wrote it.
About the tests - there are some tests with baseClassLoader set to null, and that's probably it.
Merged manually: bab5d16 |
Motivation for this change is given in the comment.