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

Consolidate jvm subprocess and inprocess functions #4456

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

sake92
Copy link
Contributor

@sake92 sake92 commented Feb 2, 2025

Some minor questions/suggestions:

  1. current spawnClassloader is more flexible, you can pass your own classloader rather that use getClass.getClassLoader, this is ok I guess?
  2. suggestion to name spawnClassLoader as createClassLoader
  3. suggestion to name callClassloader as withClassloader (common loan pattern name)
  4. getMainMethod is not public so runLocal can't be removed.. so expose getMainMethod or leave runLocal as is?
  5. Some calls are inside build.mill files.
    I have to wait for new version of Mill with these changes included, and then update them?
  6. Not sure if closeClassLoaderWhenDone is needed, I don't understand why it is left unclosed in few places.

Closes #3772

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

The naming is pulled a bit in two directions

  1. On one hand, we want classloader methods to be consistent with the equivalent subprocess methods, which are named spawn and call
  2. On the other hand, we want classloader methods to be consistent with the equivalent Scala patterns, which call them create and with

TBH I'm not sure which one is better. @lefou @lolgab not sure if you guys have any opinions here?

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

current spawnClassloader is more flexible, you can pass your own classloader rather that use getClass.getClassLoader, this is ok I guess?

This is to let you select the parent classloader; at the minimum it lets you decide whether or not to include the current Mill classloader on the child classpath, or have the child classpath entirely isolated. So we probably need to keep it as necessary complexity in the API

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

Some calls are inside build.mill files.
I have to wait for new version of Mill with these changes included, and then update them?

That's right. For now can just leave them in place

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

getMainMethod is not public so runLocal can't be removed.. so expose getMainMethod or leave runLocal as is?

Seems that Jvm.runLocal is only called in one place in RunModule.scala, so maybe we can move getMainMethod there?

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

Not sure if closeClassLoaderWhenDone is needed, I don't understand why it is left unclosed in few places.

Maybe we can try closing it all the time and see if any tests break? I don't understand why we wouldn't close it either haha

@lihaoyi
Copy link
Member

lihaoyi commented Feb 2, 2025

The name Jvm is also somewhat generic, could we come up with a more specific name? e.g. maybe Classpath.withClassLoader/Classpath.spawnSubprocess?

@sake92
Copy link
Contributor Author

sake92 commented Feb 2, 2025

getMainMethod is not public so runLocal can't be removed.. so expose getMainMethod or leave runLocal as is?

Seems that Jvm.runLocal is only called in one place in RunModule.scala, so maybe we can move getMainMethod there?

Good point.
Copied it to RunModule, we can remove the one from Jvm when we remove these deprecated functions.

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.

Clean up Jvm.* subprocess/inprocess APIs (500USD Bounty)
2 participants