Consolidate jvm subprocess and inprocess functions #4456

sake92 · 2025-02-02T10:59:57Z

Changes:

added functions based on Clean up Jvm.* subprocess/inprocess APIs (500USD Bounty) #3772 description
- the process related ones now return os-lib process result or subprocess handle
- callClassLoader uses a loan pattern function and closes the classloader after usage
added mill.util.ProcessUtil.toResult(processResult) to handle process result in same way https://github.com/com-lihaoyi/mill/blob/main/main/util/src/mill/util/Jvm.scala#L352-L357

Some minor questions/suggestions:

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

Closes #3772

…assLoader, runClassLoader}. Remove JavaModule.launcher

…ctions

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

lihaoyi · 2025-02-02T11:12:36Z

The naming is pulled a bit in two directions

On one hand, we want classloader methods to be consistent with the equivalent subprocess methods, which are named spawn and call
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 · 2025-02-02T11:14:13Z

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 · 2025-02-02T11:14:41Z

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 · 2025-02-02T11:17:33Z

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 · 2025-02-02T11:19:32Z

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 · 2025-02-02T11:24:33Z

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

…Result

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

sake92 · 2025-02-02T11:27:59Z

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.

…assLoader] (parent cl)

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

sake92 · 2025-02-06T17:45:29Z

In the last commit I only changed a comment.
But the previous commit tests worked fine..

main/util/src/mill/util/Jvm.scala

scalalib/test/src/mill/scalalib/AssemblyTestUtils.scala

scalalib/src/mill/scalalib/JavaModule.scala

pythonlib/src/mill/pythonlib/PythonModule.scala

lihaoyi · 2025-02-07T02:50:35Z

Last few comments, after that we can merge it! Test failures are probably flakes and can be ignored.

sake92 · 2025-02-07T07:50:17Z

Should be good now.

lihaoyi · 2025-02-07T08:39:14Z

@sake92 looks like there's a compile error somewhere

…ctions

lihaoyi · 2025-02-07T10:20:02Z

Thanks @sake92! Failures look unrelated so I'll merge this first and deal with failures asynchronously. Could you help open a PR backporting this to 0.12.x? Then I'll separately remove the deprecated methods from main and transfer the bounty using the same bank details as before

Changes: - added functions based on com-lihaoyi#3772 description - the process related ones now return os-lib process result or subprocess handle - `callClassLoader` uses a loan pattern function and closes the classloader after usage - added `mill.util.ProcessUtil.toResult(processResult)` to handle process result in same way https://github.com/com-lihaoyi/mill/blob/main/main/util/src/mill/util/Jvm.scala#L352-L357 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? 1. suggestion to name `spawnClassLoader` as `createClassLoader` 1. suggestion to name `callClassloader` as `withClassloader` (common loan pattern name) 1. `getMainMethod` is not public so `runLocal` can't be removed.. so expose `getMainMethod` or leave `runLocal` as is? 1. Some calls are inside build.mill files. I have to wait for new version of Mill with these changes included, and then update them? 1. Not sure if `closeClassLoaderWhenDone` is needed, I don't understand why it is left unclosed in few places. Closes com-lihaoyi#3772 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

… to 0.12 (#4504) Backport of #4456 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

ref: com-lihaoyi/mill@f35464c com-lihaoyi/mill#4456 Signed-off-by: unlsycn <unlsycn@unlsycn.com>

@sake92

Fixes #4863 Seems like it was broken by #4837 which replaced `Jvm.runSubprocess` with `os.call`, which unfortunately had different stream forwarding defaults. This class of problem should be impossible in future thanks to #4456 from @sake92 which standardizing the stream forwarding defaults of `Jvm.*Process` APIs with `os.*`

sake92 added 9 commits February 2, 2025 08:01

Replace Jvm.callSubprocess with Jvm.call

e6e771c

Replace Jvm.runSubprocess with Jvm.spawn and os.call

f5c788a

Replace Jvm.{spawnClassloader, inprocess, runLocal} with Jvm.{spawnCl…

d6d5dfa

…assLoader, runClassLoader}. Remove JavaModule.launcher

Add missing deprecate annotation

2a67c68

Revert accidental changes

21fe680

Revert accidental changes

048909f

Merge branch 'main' into consolidate-jvm-subprocess-and-inprocess-fun…

7076c93

…ctions

Fix annotation

d06977c

Merge branch 'consolidate-jvm-subprocess-and-inprocess-functions' of …

5e07181

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

[autofix.ci] apply automated fixes

f112c4f

autofix-ci bot and others added 3 commits February 2, 2025 11:25

[autofix.ci] apply automated fixes (attempt 2/3)

1ff7748

Copy getMainMethod into RunModule. Replace one more runSubprocessWith…

2130b99

…Result

Merge branch 'consolidate-jvm-subprocess-and-inprocess-functions' of …

833b99e

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

sake92 and others added 10 commits February 2, 2025 12:30

Always close classloader

d9bfbdf

[autofix.ci] apply automated fixes

4a74944

Remove closeClassLoaderWhenDone. Replace isolated flag with Option[Cl…

053aedc

…assLoader] (parent cl)

Merge branch 'consolidate-jvm-subprocess-and-inprocess-functions' of …

722af17

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

Try fix

ac57f8d

[autofix.ci] apply automated fixes

0b7e826

Try fix

18c6108

Merge branch 'consolidate-jvm-subprocess-and-inprocess-functions' of …

118d8f9

…github.com:sake92/mill into consolidate-jvm-subprocess-and-inprocess-functions

Remove commented code

beb4b97

Cleanup

eea2f0f

Remove commented code

32f6b8c