fix assembly path handling, swap out Jvm.scala's custom subprocess handling with os.proc

10 files changed, 121 insertions, 123 deletions

diff --git a/.travis.yml b/.travis.yml
index 897c4712..785b79e6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -9,7 +9,7 @@ matrix:
     jdk: oraclejdk8
   - stage: build
     env: CI_SCRIPT=ci/test-mill-release.sh
-    jdk: oraclejdk9
+    jdk: openjdk10
 
   - stage: build
     env: CI_SCRIPT=ci/test-mill-dev.sh
@@ -19,7 +19,7 @@ matrix:
     jdk: oraclejdk9
   - stage: build
     env: CI_SCRIPT=ci/test-mill-bootstrap.sh
-    jdk: oraclejdk9
+    jdk: openjdk10
 
   - stage: build
     env: CI_SCRIPT=ci/test-mill-0.sh
@@ -29,7 +29,7 @@ matrix:
     jdk: oraclejdk8
   - stage: build
     env: CI_SCRIPT=ci/test-mill-2.sh
-    jdk: oraclejdk8
+    jdk: openjdk10
 
 
   - stage: release
diff --git a/integration/test/resources/caffeine/build.sc b/integration/test/resources/caffeine/build.sc
index a4967f31..3defeb50 100644
--- a/integration/test/resources/caffeine/build.sc
+++ b/integration/test/resources/caffeine/build.sc
@@ -44,7 +44,7 @@ object caffeine extends CaffeineModule {
       "com.github.benmanes.caffeine.cache.NodeFactoryGenerator",
       "com.github.benmanes.caffeine.cache.LocalCacheFactoryGenerator",
     )
-    for(mainCls <- mains) Jvm.interactiveSubprocess(
+    for(mainCls <- mains) Jvm.runSubprocess(
       mainCls,
       javaPoet.runClasspath().map(_.path),
       javaPoet.forkArgs(),
diff --git a/integration/test/resources/play-json/jmh.sc b/integration/test/resources/play-json/jmh.sc
index 3c2c8411..d67e14d1 100644
--- a/integration/test/resources/play-json/jmh.sc
+++ b/integration/test/resources/play-json/jmh.sc
@@ -7,7 +7,7 @@ trait Jmh extends ScalaModule {
 
   def runJmh(args: String*) = T.command {
     val (_, resources) = generateBenchmarkSources()
-    Jvm.interactiveSubprocess(
+    Jvm.runSubprocess(
       "org.openjdk.jmh.Main",
       classPath = (runClasspath() ++ generatorDeps()).map(_.path) ++
         Seq(compileGeneratedSources().path, resources),
@@ -41,7 +41,7 @@ trait Jmh extends ScalaModule {
     os.remove.all(resourcesDir)
     os.makeDir.all(resourcesDir)
 
-    Jvm.subprocess(
+    Jvm.runSubprocess(
       "org.openjdk.jmh.generators.bytecode.JmhBytecodeGenerator",
       (runClasspath() ++ generatorDeps()).map(_.path),
       mainArgs = Array(
diff --git a/main/core/src/mill/eval/Evaluator.scala b/main/core/src/mill/eval/Evaluator.scala
index 7cf55fdb..563d7b63 100644
--- a/main/core/src/mill/eval/Evaluator.scala
+++ b/main/core/src/mill/eval/Evaluator.scala
@@ -268,13 +268,7 @@ case class Evaluator(home: os.Path,
     for (task <- nonEvaluatedTargets) {
       newEvaluated.append(task)
       val targetInputValues = task.inputs
-        .map{x =>
-          val res = newResults.getOrElse(x, results(x))
-          if (!res.isInstanceOf[Result.Success[_]]){
-            println("FAILURE " + x)
-          }
-          res
-        }
+        .map{x => newResults.getOrElse(x, results(x))}
         .collect{ case Result.Success((v, hashCode)) => v }
 
       val res =
diff --git a/main/src/mill/modules/Jvm.scala b/main/src/mill/modules/Jvm.scala
index b4a55de3..8d2c4de4 100644
--- a/main/src/mill/modules/Jvm.scala
+++ b/main/src/mill/modules/Jvm.scala
@@ -20,67 +20,115 @@ import scala.collection.mutable
 import scala.collection.JavaConverters._
 
 object Jvm {
+  /**
+    * Runs a JVM subprocess with the given configuration and returns a
+    * [[os.CommandResult]] with it's aggregated output and error streams
+    */
+  def callSubprocess(mainClass: String,
+                     classPath: Agg[os.Path],
+                     jvmArgs: Seq[String] = Seq.empty,
+                     envArgs: Map[String, String] = Map.empty,
+                     mainArgs: Seq[String] = Seq.empty,
+                     workingDir: os.Path = null,
+                     streamOut: Boolean = true)
+                    (implicit ctx: Ctx) = {
+
+    val commandArgs =
+      Vector("java") ++
+      jvmArgs ++
+      Vector("-cp", classPath.mkString(File.pathSeparator), mainClass) ++
+      mainArgs
+
+    val workingDir1 = Option(workingDir).getOrElse(ctx.dest)
+    os.makeDir.all(workingDir1)
 
-  def interactiveSubprocess(mainClass: String,
-                            classPath: Agg[os.Path],
-                            jvmArgs: Seq[String] = Seq.empty,
-                            envArgs: Map[String, String] = Map.empty,
-                            mainArgs: Seq[String] = Seq.empty,
-                            workingDir: os.Path = null,
-                            background: Boolean = false): Unit = {
+    os.proc(commandArgs).call(cwd = workingDir1, env = envArgs)
+  }
+
+  /**
+    * Runs a JVM subprocess with the given configuration and streams
+    * it's stdout and stderr to the console.
+    */
+  def runSubprocess(mainClass: String,
+                    classPath: Agg[os.Path],
+                    jvmArgs: Seq[String] = Seq.empty,
+                    envArgs: Map[String, String] = Map.empty,
+                    mainArgs: Seq[String] = Seq.empty,
+                    workingDir: os.Path = null,
+                    background: Boolean = false): Unit = {
     val args =
       Vector("java") ++
       jvmArgs ++
       Vector("-cp", classPath.mkString(File.pathSeparator), mainClass) ++
       mainArgs
 
-    if (background) baseInteractiveSubprocess0(args, envArgs, workingDir)
-    else baseInteractiveSubprocess(args, envArgs, workingDir)
+    if (background) spawnSubprocess(args, envArgs, workingDir)
+    else runSubprocess(args, envArgs, workingDir)
   }
 
+  @deprecated("Use runSubprocess instead")
   def baseInteractiveSubprocess(commandArgs: Seq[String],
                                 envArgs: Map[String, String],
                                 workingDir: os.Path) = {
-    val process = baseInteractiveSubprocess0(commandArgs, envArgs, workingDir)
+    runSubprocess(commandArgs, envArgs, workingDir)
+  }
 
-    val exitCode = process.waitFor()
-    if (exitCode == 0) ()
+  /**
+    * Runs a generic subprocess and waits for it to terminate.
+    */
+  def runSubprocess(commandArgs: Seq[String],
+                    envArgs: Map[String, String],
+                    workingDir: os.Path) = {
+    val process = spawnSubprocess(commandArgs, envArgs, workingDir)
+
+    process.waitFor()
+    if (process.exitCode() == 0) ()
     else throw new Exception("Interactive Subprocess Failed")
   }
-  def baseInteractiveSubprocess0(commandArgs: Seq[String],
-                                 envArgs: Map[String, String],
-                                 workingDir: os.Path) = {
-    val builder = new java.lang.ProcessBuilder()
-
-    for ((k, v) <- envArgs){
-      if (v != null) builder.environment().put(k, v)
-      else builder.environment().remove(k)
-    }
-    builder.directory(workingDir.toIO)
 
+  /**
+    * Spawns a generic subprocess, streaming the stdout and stderr to the
+    * console. If the System.out/System.err have been substituted, makes sure
+    * that the subprocess's stdout and stderr streams go to the subtituted
+    * streams
+    */
+  def spawnSubprocess(commandArgs: Seq[String],
+                      envArgs: Map[String, String],
+                      workingDir: os.Path) = {
+    // If System.in is fake, then we pump output manually rather than relying
+    // on `os.Inherit`. That is because `os.Inherit` does not follow changes
+    // to System.in/System.out/System.err, so the subprocess's streams get sent
+    // to the parent process's origin outputs even if we want to direct them
+    // elsewhere
     if (System.in.isInstanceOf[ByteArrayInputStream]){
-
-      val process = builder
-        .command(commandArgs:_*)
-        .start()
+      val process = os.proc(commandArgs).spawn(
+        cwd = workingDir,
+        env = envArgs,
+        stdin = os.Pipe,
+        stdout = os.Pipe,
+        stderr = os.Pipe
+      )
 
       val sources = Seq(
-        process.getInputStream -> System.out,
-        process.getErrorStream -> System.err,
-        System.in -> process.getOutputStream
+        process.stdout -> System.out,
+        process.stderr -> System.err,
+        System.in -> process.stdin
       )
 
       for((std, dest) <- sources){
         new Thread(new InputPumper(std, dest, false)).start()
       }
+
       process
     }else{
-      builder
-        .command(commandArgs:_*)
-        .inheritIO()
-        .start()
+      os.proc(commandArgs).spawn(
+        cwd = workingDir,
+        env = envArgs,
+        stdin = os.Inherit,
+        stdout = os.Inherit,
+        stderr = os.Inherit
+      )
     }
-
   }
 
 
@@ -108,7 +156,6 @@ object Jvm {
   }
 
 
-
   def inprocess[T](classPath: Agg[os.Path],
                    classLoaderOverrideSbtTesting: Boolean,
                    isolated: Boolean,
@@ -141,62 +188,6 @@ object Jvm {
     }
   }
 
-  def subprocess(mainClass: String,
-                 classPath: Agg[os.Path],
-                 jvmArgs: Seq[String] = Seq.empty,
-                 envArgs: Map[String, String] = Map.empty,
-                 mainArgs: Seq[String] = Seq.empty,
-                 workingDir: os.Path = null)
-                (implicit ctx: Ctx) = {
-
-    val commandArgs =
-      Vector("java") ++
-      jvmArgs ++
-      Vector("-cp", classPath.mkString(File.pathSeparator), mainClass) ++
-      mainArgs
-
-    val workingDir1 = Option(workingDir).getOrElse(ctx.dest)
-    os.makeDir.all(workingDir1)
-    val builder =
-      new java.lang.ProcessBuilder()
-        .directory(workingDir1.toIO)
-        .command(commandArgs:_*)
-        .redirectOutput(ProcessBuilder.Redirect.PIPE)
-        .redirectError(ProcessBuilder.Redirect.PIPE)
-
-    for((k, v) <- envArgs) builder.environment().put(k, v)
-    val proc = builder.start()
-    val stdout = proc.getInputStream
-    val stderr = proc.getErrorStream
-    val sources = Seq(
-      (stdout, Left(_: os.Bytes), ctx.log.outputStream),
-      (stderr, Right(_: os.Bytes),ctx.log.errorStream )
-    )
-    val chunks = mutable.Buffer.empty[Either[os.Bytes, os.Bytes]]
-    while(
-    // Process.isAlive doesn't exist on JDK 7 =/
-      util.Try(proc.exitValue).isFailure ||
-        stdout.available() > 0 ||
-        stderr.available() > 0
-    ){
-      var readSomething = false
-      for ((subStream, wrapper, parentStream) <- sources){
-        while (subStream.available() > 0){
-          readSomething = true
-          val array = new Array[Byte](subStream.available())
-          val actuallyRead = subStream.read(array)
-          chunks.append(wrapper(new os.Bytes(array)))
-          parentStream.write(array, 0, actuallyRead)
-        }
-      }
-      // if we did not read anything sleep briefly to avoid spinning
-      if(!readSomething)
-        Thread.sleep(2)
-    }
-
-    if (proc.exitValue() != 0) throw new Exception("Subprocess failed")
-    else os.CommandResult(proc.exitValue(), chunks)
-  }
 
   private def createManifest(mainClass: Option[String]) = {
     val m = new java.util.jar.Manifest()
@@ -291,14 +282,14 @@ object Jvm {
     Assembly.groupAssemblyEntries(inputPaths, assemblyRules).view
       .foreach {
         case (mapping, AppendEntry(entries)) =>
-          val path = zipFs.getPath(mapping)
+          val path = zipFs.getPath(mapping).toAbsolutePath
           val concatenated = new SequenceInputStream(
             Collections.enumeration(entries.map(_.inputStream).asJava))
-          writeEntry(os.Path(path), concatenated, append = true)
+          writeEntry(path, concatenated, append = true)
         case (mapping, WriteOnceEntry(entry)) =>
-          val path = zipFs.getPath(mapping)
+          val path = zipFs.getPath(mapping).toAbsolutePath
           if (Files.notExists(path)) {
-            writeEntry(os.Path(path), entry.inputStream, append = false)
+            writeEntry(path, entry.inputStream, append = false)
           }
       }
 
@@ -331,14 +322,17 @@ object Jvm {
     PathRef(output)
   }
 
-  private def writeEntry(p: os.Path, is: InputStream, append: Boolean): Unit = {
-    if (p.toNIO.getParent != null) Files.createDirectories(p.toNIO.getParent)
-    if (append) os.write(p, is)
-    else os.write.append(p, is)
+  private def writeEntry(p: java.nio.file.Path, is: InputStream, append: Boolean): Unit = {
+    if (p.getParent != null) Files.createDirectories(p.getParent)
+    val options =
+      if(append) Seq(StandardOpenOption.APPEND, StandardOpenOption.CREATE)
+      else Seq(StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.CREATE)
 
+    val outputStream = java.nio.file.Files.newOutputStream(p, options:_*)
+    IO.stream(is, outputStream)
+    outputStream.close()
     is.close()
   }
-
   def universalScript(shellCommands: String,
                       cmdCommands: String,
                       shebang: Boolean = false): String = {
diff --git a/main/test/src/mill/util/ScriptTestSuite.scala b/main/test/src/mill/util/ScriptTestSuite.scala
index b324f94c..8a639f92 100644
--- a/main/test/src/mill/util/ScriptTestSuite.scala
+++ b/main/test/src/mill/util/ScriptTestSuite.scala
@@ -24,7 +24,12 @@ abstract class ScriptTestSuite(fork: Boolean) extends TestSuite{
     if (!fork) runner.runScript(workspacePath / buildPath , s.toList)
     else{
       try {
-        os.proc(os.home / "mill-release", "-i", s).call(wd)
+        os.proc(os.home / "mill-release", "-i", s).call(
+          wd,
+          stdin = os.Inherit,
+          stdout = os.Inherit,
+          stderr = os.Inherit,
+        )
         true
       }catch{case e: Throwable => false}
     }
diff --git a/readme.md b/readme.md
index a87fba0d..95942322 100644
--- a/readme.md
+++ b/readme.md
@@ -151,6 +151,11 @@ optimizer without classpath conflicts.
 
 ## Changelog
 
+### master
+
+- Mill is now bundled with [OS-Lib](https://github.com/lihaoyi/os-lib),
+  providing a simpler way of dealing with filesystem APIs and subprocesses
+
 ### 0.3.3
 
 - Added new `debug` method to context logger, to log additional debug info into the
diff --git a/scalalib/src/mill/scalalib/JavaModule.scala b/scalalib/src/mill/scalalib/JavaModule.scala
index 387011ec..53677c53 100644
--- a/scalalib/src/mill/scalalib/JavaModule.scala
+++ b/scalalib/src/mill/scalalib/JavaModule.scala
@@ -400,7 +400,7 @@ trait JavaModule extends mill.Module with TaskModule { outer =>
     * Runs this module's code in a subprocess and waits for it to finish
     */
   def run(args: String*) = T.command{
-    try Result.Success(Jvm.interactiveSubprocess(
+    try Result.Success(Jvm.runSubprocess(
       finalMainClass(),
       runClasspath().map(_.path),
       forkArgs(),
@@ -458,7 +458,7 @@ trait JavaModule extends mill.Module with TaskModule { outer =>
     */
   def runBackground(args: String*) = T.command{
     val (procId, procTombstone, token) = backgroundSetup(T.ctx().dest)
-    try Result.Success(Jvm.interactiveSubprocess(
+    try Result.Success(Jvm.runSubprocess(
       "mill.scalalib.backgroundwrapper.BackgroundWrapper",
       (runClasspath() ++ zincWorker.backgroundWrapperClasspath()).map(_.path),
       forkArgs(),
@@ -476,7 +476,7 @@ trait JavaModule extends mill.Module with TaskModule { outer =>
     */
   def runMainBackground(mainClass: String, args: String*) = T.command{
     val (procId, procTombstone, token) = backgroundSetup(T.ctx().dest)
-    try Result.Success(Jvm.interactiveSubprocess(
+    try Result.Success(Jvm.runSubprocess(
       "mill.scalalib.backgroundwrapper.BackgroundWrapper",
       (runClasspath() ++ zincWorker.backgroundWrapperClasspath()).map(_.path),
       forkArgs(),
@@ -504,7 +504,7 @@ trait JavaModule extends mill.Module with TaskModule { outer =>
     * Same as `run`, but lets you specify a main class to run
     */
   def runMain(mainClass: String, args: String*) = T.command{
-    try Result.Success(Jvm.interactiveSubprocess(
+    try Result.Success(Jvm.runSubprocess(
       mainClass,
       runClasspath().map(_.path),
       forkArgs(),
@@ -545,7 +545,7 @@ trait TestModule extends JavaModule with TaskModule {
   def test(args: String*) = T.command{
     val outputPath = T.ctx().dest/"out.json"
 
-    Jvm.subprocess(
+    Jvm.runSubprocess(
       mainClass = "mill.scalalib.TestRunner",
       classPath = zincWorker.scalalibClasspath().map(_.path),
       jvmArgs = forkArgs(),
diff --git a/scalalib/src/mill/scalalib/ScalaModule.scala b/scalalib/src/mill/scalalib/ScalaModule.scala
index 80caf6a9..38a5a8ea 100644
--- a/scalalib/src/mill/scalalib/ScalaModule.scala
+++ b/scalalib/src/mill/scalalib/ScalaModule.scala
@@ -5,7 +5,7 @@ import coursier.Repository
 import mill.define.{Target, Task, TaskModule}
 import mill.eval.{PathRef, Result}
 import mill.modules.Jvm
-import mill.modules.Jvm.{createJar, subprocess}
+import mill.modules.Jvm.createJar
 import Dep.isDotty
 import Lib._
 import mill.util.Loose.Agg
@@ -206,7 +206,7 @@ trait ScalaModule extends JavaModule { outer =>
     if (T.ctx().log.inStream == DummyInputStream){
       Result.Failure("repl needs to be run with the -i/--interactive flag")
     }else{
-      Jvm.interactiveSubprocess(
+      Jvm.runSubprocess(
         mainClass =
           if (isDotty(scalaVersion()))
             "dotty.tools.repl.Main"
@@ -241,7 +241,7 @@ trait ScalaModule extends JavaModule { outer =>
     if (T.ctx().log.inStream == DummyInputStream){
       Result.Failure("repl needs to be run with the -i/--interactive flag")
     }else{
-      Jvm.interactiveSubprocess(
+      Jvm.runSubprocess(
         mainClass = "ammonite.Main",
         classPath = ammoniteReplClasspath().map(_.path),
         mainArgs = replOptions,
diff --git a/scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala b/scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala
index 41fad45b..d6992fc8 100644
--- a/scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala
+++ b/scalalib/src/mill/scalalib/scalafmt/ScalafmtWorker.scala
@@ -47,7 +47,7 @@ private[scalafmt] class ScalafmtWorker {
                              classpath: Agg[os.Path])(implicit ctx: Ctx) = {
     val configFlags =
       if (os.exists(config)) Seq("--config", config.toString) else Seq.empty
-    Jvm.subprocess(
+    Jvm.runSubprocess(
       "org.scalafmt.cli.Cli",
       classpath,
       mainArgs = toFormat.map(_.toString) ++ configFlags ++ cliFlags