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

Fix #370 Use ExtClassLoader instead of null as parent #1848

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

lolgab
Copy link
Member

@lolgab lolgab commented Apr 23, 2022

No description provided.

@lolgab lolgab marked this pull request as ready for review April 23, 2022 11:46
@lolgab lolgab requested review from lefou and lihaoyi April 25, 2022 17:56
Comment on lines 72 to 84
} else {
// With Java 8 we want a clean classloader that still contains classes
// coming from com.sun.* etc.
// We get the application classloader parent which happens to be of
// type sun.misc.Launcher$ExtClassLoader
// We can't call the method directly since it doesn't exist in Java 9
// So we load it via reflection.
val launcherClass = getClass.getClassLoader().loadClass("sun.misc.Launcher")
val getLauncherMethod = launcherClass.getMethod("getLauncher")
val launcher = getLauncherMethod.invoke(null)
val getClassLoaderMethod = launcher.getClass().getMethod("getClassLoader")
val appClassLoader = getClassLoaderMethod.invoke(launcher).asInstanceOf[ClassLoader]
appClassLoader.getParent()
Copy link
Member

@lefou lefou Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we load this code via reflection, when we are only supposed to run it on Java 8? Is this more the kind of compile-time work-around?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code doesn't compile in Java 9+. I used reflection to allow compilation of Mill in Java 9+ since the sun.misc package doesn't exist in Java 9+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying. I think the comment is kind of abḿbigous. Maybe, you can add, that reflection solved an issue at compilation-time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rephrased the commend a bit.

@lolgab lolgab requested a review from lefou April 26, 2022 10:01
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Now, we need to find out. why this isn't working on Windows. 🤔

@lolgab lolgab requested a review from lefou April 26, 2022 18:15
@lolgab
Copy link
Member Author

lolgab commented Apr 26, 2022

Looks good to me. Now, we need to find out. why this isn't working on Windows. 🤔

It was not happy with * in the test name. It seems fine now.

@lefou lefou merged commit 59cb2fd into com-lihaoyi:main Apr 26, 2022
@lefou lefou added this to the after 0.10.3 milestone Apr 26, 2022
@lolgab lolgab deleted the fix-test-classloader branch April 26, 2022 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants