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

Add some hints to using os.proc with Mill #2060

Closed
lefou opened this issue Oct 7, 2022 · 6 comments
Closed

Add some hints to using os.proc with Mill #2060

lefou opened this issue Oct 7, 2022 · 6 comments
Labels
good-first-issue The fix/solution for the issue is considered easy. A good starting point for newbies.
Milestone

Comments

@lefou
Copy link
Member

lefou commented Oct 7, 2022

Using os.Inherit for output is known to not show up when in client-server-mode. We should mention the use of mill.modules.Jvm.runSubprocess and also give suggestions which settings work best in Mill.

Also, all links to os-lib in the documentation should go through a dedicated Mill-specific page, so that readers following it will have an option to read about those best practices. This could be the existing https://com-lihaoyi.github.io/mill/mill/External_References.html#_os_lib page, or some more dedicated.

Related:

@lefou lefou changed the title Add some tips to using os.proc with Mill Add some hints to using os.proc with Mill Oct 7, 2022
@nafg
Copy link
Contributor

nafg commented Nov 27, 2022

I've been doing something like

stdout = os.ProcessOutput.Readlines(line => T.ctx().log.debug("[prefix] " + line))

where prefix is a name for what program I'm running.

@nafg
Copy link
Contributor

nafg commented Nov 27, 2022

Although, I guess you'd want to do that for stderr too (except maybe with log.warn)...

Too bad os.proc doesn't have an easy way to reuse groups of parameters, that's something I could use often.

@nafg
Copy link
Contributor

nafg commented Nov 27, 2022

Another good place to mention Jvm.runSubprocess is https://com-lihaoyi.github.io/mill/mill/Extending_Mill.html#_custom_targets_commands

which currently says

For subprocess/filesystem operations, you can use the os-lib library that comes bundled with Mill, or even plain java.nio/java.lang.Process. Each target gets its own T.dest folder that you can use to place files without worrying about colliding with other targets.

@nafg
Copy link
Contributor

nafg commented Nov 27, 2022

One thing that's worse about Jvm.runSubprocess is that if it exits with a nonzero exit code, it throws a generic Exception that doesn't say the command line that failed. With os.proc I was doing

    catch {
      case e: os.SubprocessException =>
        throw new RuntimeException(
          "rollup failed with exit code " + e.result.exitCode,
          e
        )
    }

etc. Otherwise if a few commands run and their output is not so different-looking from each other, it's hard to tell which one failed. But since runSubprocess throws java.lang.Exception it's not as clean to catch.

A different option would be to just log at the beginning the command line that will be run.

@nafg
Copy link
Contributor

nafg commented Nov 27, 2022

I switched to

    catch {
      case e: Exception =>
        throw new RuntimeException("Error running webpack", e)
    }

but then the output is harder to scan:

main.test.fastLinkJSTest java.lang.RuntimeException: Error running webpack
    io.github.nafg.millbundler.ScalaJSWebpackModule.$anonfun$bundle$3(ScalaJSWebpackModule.scala:90)
    io.github.nafg.millbundler.ScalaJSBundleModule$Test.$anonfun$fastLinkJSTest$2(ScalaJSBundleModule.scala:33)
    mill.define.Task$TraverseCtx.evaluate(Task.scala:380)
java.lang.Exception: Interactive Subprocess Failed (exit code 1)
    mill.modules.Jvm$.runSubprocess(Jvm.scala:196)
    io.github.nafg.millbundler.ScalaJSWebpackModule.$anonfun$bundle$3(ScalaJSWebpackModule.scala:86)
    io.github.nafg.millbundler.ScalaJSBundleModule$Test.$anonfun$fastLinkJSTest$2(ScalaJSBundleModule.scala:33)
    mill.define.Task$TraverseCtx.evaluate(Task.scala:380)

@lefou lefou added the good-first-issue The fix/solution for the issue is considered easy. A good starting point for newbies. label Dec 19, 2022
@lihaoyi
Copy link
Member

lihaoyi commented Jul 22, 2024

I think this can be closed since #3275 makes os.proc.call/os.Inherit do the right thing by default

@lihaoyi lihaoyi closed this as completed Jul 22, 2024
@lefou lefou added this to the 0.11.10 milestone Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue The fix/solution for the issue is considered easy. A good starting point for newbies.
Projects
None yet
Development

No branches or pull requests

3 participants