Make run command arguments Tasks (#2452)

Fixes #1948. Should probably
be reviewed concurrently with
com-lihaoyi/mainargs#62 which it depends on

# Major Changes

1. Pull in com-lihaoyi/mainargs#62, which allows
MainArg's "aggregate leftover arguments" logic to apply to custom types
and not be hardcoded to `mainargs.Leftover`

2. Update MainArgs integration code in this repo to accomadate the
changes

3. Replace the `def run(args: String*)` with `def run(args: Task[Args] =
T.task(Args()))`.

# Minor Changes

1. I moved the `import mill.main.TokenReaders._` into the `Discover`
macro, so we can stop manually importing it everywhere.

2. I moved all the aliases from `import mill.define._` to `import
mill._`. `Task[_]` in particular is a type I believe we should start
encouraging people to use more often - e.g. annotating all command
parameter types as `Task[_]`s - and so it should be in the default
`import mill._` without needing a separate import.

3. Added a direct dependency to `com.lihaoyi::pprint`, since the
implicit dependency in `mainargs` was removed in 0.5.0 due to being
unnecessary

# Testing

1. All existing tests pass, at least those I've run locally so far
(`./mill -i -j 3 example.__.local.test`, can't use CI until the mainargs
PR is published). This includes unit tests that call `run`
programmatically and integration/example tests that call `run` via the
CLI.

4. Added a new example test `5-module-run-task`, that demonstrates and
explains how to use a `ScalaModule`'s `run` method to implement a
`task`, passing in input parameters from upstream tasks and using the
output (in this case generated files) in downstream tasks

# Notes

1. There is still some boilerplate around `run`, both defining it `args:
Task[Args] = T.task(Args())` and calling it programmatically,
`run(T.task(Args(...)))`. For now I haven't figured out how to reduce
this, as we already "used up" our implicit conversion budget with the
`T.*{...}` macros. Probably can be done, although it won't be quite as
low-boilerplate as the original `args: String*` syntax. Maybe in a
follow-up PR

2. The example test `5-module-run-task` is something I've wanted to be
able to write for years now. Nice to finally be able to do it without
having to piece together a `Jvm.runSubprocess()` call myself!

3. `5-module-run-task` is still a bit clunkier than I was hoping. Having
to `def barWorkingDir` beforehand to pass it to both the `bar.run` and
the `def sources` override is weird and unintuitive. Maybe we can change
the `def run` return type to make it easier to return the `T.dest`
folder or other metadata out of the body of `def run` so it can be used
by downstream tasks? e.g. making `def run` return `Command[PathRef]`
rather than `Command[Unit]`


5. IMO we should recommend that all CLI arguments to `T.command`s come
in the form of `Task[T]`s, unless there's concrete reasons otherwise
(e.g. we want to dynamically change `T.command` implementations based on
the input). That would maximise the ability to re-use commands in the
body of of tasks, which is something people probably want

---------

Co-authored-by: Tobias Roeser <le.petit.fou@web.de>

bsp/src/mill/bsp/BSP.scala

@@ -14,7 +14,6 @@ import mill.util.PrintLogger
 import os.Path
 
 object BSP extends ExternalModule with CoursierModule with BspServerStarter {
-  import mill.main.TokenReaders._
 
   lazy val millDiscover: Discover[this.type] = Discover[this.type]
 

bsp/worker/src/mill/bsp/worker/MillBuildServer.scala

@@ -58,10 +58,9 @@ import com.google.gson.JsonObject
 import mill.T
 import mill.api.{DummyTestReporter, PathRef, Result, Strict, internal}
 import mill.define.Segment.Label
-import mill.define.{Discover, ExternalModule, Module, Segments, Task}
+import mill.define.{Args, Discover, ExternalModule, Module, Segments, Task}
 import mill.eval.Evaluator
 import mill.main.{BspServerResult, MainModule}
-import mill.main.TokenReaders._
 import mill.scalalib.{JavaModule, SemanticDbJavaModule, TestModule}
 import mill.scalalib.bsp.{BspModule, JvmBuildTarget, MillBuildModule, ScalaBuildTarget}
 import mill.scalalib.internal.ModuleUtils
@@ -538,7 +537,7 @@ class MillBuildServer(
         case m: JavaModule => m
       }.get
       val args = params.getArguments.getOrElse(Seq.empty[String])
-      val runTask = module.run(args: _*)
+      val runTask = module.run(T.task(Args(args)))
       val runResult = evaluator.evaluate(
         Strict.Agg(runTask),
         Utils.getBspLoggedReporterPool(runParams.getOriginId, bspIdByModule, client),

build.sc

@@ -127,7 +127,8 @@ object Deps {
   val lambdaTest = ivy"de.tototec:de.tobiasroeser.lambdatest:0.8.0"
   val log4j2Core = ivy"org.apache.logging.log4j:log4j-core:2.20.0"
   val osLib = ivy"com.lihaoyi::os-lib:0.9.1"
-  val mainargs = ivy"com.lihaoyi::mainargs:0.4.0"
+  val pprint = ivy"com.lihaoyi::pprint:0.8.1"
+  val mainargs = ivy"com.lihaoyi::mainargs:0.5.0"
   val millModuledefsVersion = "0.10.9"
   val millModuledefsString = s"com.lihaoyi::mill-moduledefs:${millModuledefsVersion}"
   val millModuledefs = ivy"${millModuledefsString}"
@@ -668,6 +669,7 @@ object main extends MillModule {
     override def ivyDeps = Agg(
       Deps.osLib,
       Deps.upickle,
+      Deps.pprint,
       Deps.fansi,
       Deps.sbtTestInterface
     )
@@ -746,7 +748,7 @@ object main extends MillModule {
   }
 
   object testkit extends MillInternalModule with MillAutoTestSetup {
-    def moduleDeps = Seq(eval, util)
+    def moduleDeps = Seq(eval, util, main)
   }
 
   def testModuleDeps = super.testModuleDeps ++ Seq(testkit)

ci/mill-bootstrap.patch

@@ -1,8 +1,14 @@
 diff --git a/build.sc b/build.sc
-index d7275ab3de..96c5bc0d9c 100644
+index 21678b0fbe..0c96446ceb 100644
 --- a/build.sc
 +++ b/build.sc
-@@ -25,8 +25,10 @@ import mill.main.MainModule
+@@ -19,17 +19,18 @@ import com.github.lolgab.mill.mima.{
+ import coursier.maven.MavenRepository
+ import de.tobiasroeser.mill.vcs.version.VcsVersion
+ import mill._
+-import mill.define.{Command, Source, Sources, Target, Task}
+ import mill.eval.Evaluator
+ import mill.main.MainModule
  import mill.scalalib._
  import mill.scalalib.publish._
  import mill.modules.Jvm
@@ -14,8 +20,12 @@ index d7275ab3de..96c5bc0d9c 100644
 +import mill.scalalib.api.Versions
  import scala.util.control.NonFatal
  import mill.T
- import mill.define.{Discover, ExternalModule, Input, Module, Task}
-@@ -163,12 +165,8 @@ object Deps {
+-import mill.define.{Discover, ExternalModule, Input, Module, Task}
++import mill.define.{Discover, ExternalModule}
+ import mill.api.{Logger, Result}
+ import os.{CommandResult, SubprocessException}
+
+@@ -164,12 +165,8 @@ object Deps {
    val requests = ivy"com.lihaoyi::requests:0.8.0"
  }
 
@@ -30,7 +40,7 @@ index d7275ab3de..96c5bc0d9c 100644
  def millBinPlatform: T[String] = T {
    val tag = millLastTag()
    if (tag.contains("-M")) tag
-@@ -217,8 +215,8 @@ val buildBridgeScalaVersions =
+@@ -218,8 +215,8 @@ val buildBridgeScalaVersions =
    if (!buildAllCompilerBridges) Seq()
    else bridgeScalaVersions
 
@@ -41,7 +51,7 @@ index d7275ab3de..96c5bc0d9c 100644
    def scalaVersion = crossScalaVersion
    def publishVersion = bridgeVersion
    def artifactName = T { "mill-scala-compiler-bridge" }
-@@ -237,239 +235,25 @@ class BridgeModule(val crossScalaVersion: String) extends PublishModule with Cro
+@@ -238,239 +235,25 @@ class BridgeModule(val crossScalaVersion: String) extends PublishModule with Cro
    def generatedSources = T {
      import mill.scalalib.api.ZincWorkerUtil.{grepJar, scalaBinaryVersion}
      val resolvedJars = resolveDeps(
@@ -286,7 +296,7 @@ index d7275ab3de..96c5bc0d9c 100644
  def commonPomSettings(artifactName: String) = {
    PomSettings(
      description = artifactName,
-@@ -523,27 +307,8 @@ trait MillCoursierModule extends CoursierModule {
+@@ -524,27 +307,8 @@ trait MillCoursierModule extends CoursierModule {
    )
  }
 
@@ -315,7 +325,7 @@ index d7275ab3de..96c5bc0d9c 100644
  }
 
  /** A Module compiled with applied Mill-specific compiler plugins: mill-moduledefs. */
-@@ -846,7 +611,10 @@ object scalajslib extends MillModule with BuildInfo {
+@@ -852,7 +616,10 @@ object scalajslib extends MillModule with BuildInfo {
      override def ivyDeps = Agg(Deps.sbtTestInterface)
    }
    object worker extends Cross[WorkerModule]("1")
@@ -327,7 +337,7 @@ index d7275ab3de..96c5bc0d9c 100644
      def testDepPaths = T{ Seq(compile().classes) }
      override def moduleDeps = Seq(scalajslib.`worker-api`, main.client, main.api)
      override def ivyDeps = Agg(
-@@ -911,8 +679,10 @@ object contrib extends MillModule {
+@@ -917,8 +684,10 @@ object contrib extends MillModule {
 
      object api extends MillPublishModule
 
@@ -340,7 +350,7 @@ index d7275ab3de..96c5bc0d9c 100644
        override def sources = T.sources {
          // We want to avoid duplicating code as long as the Play APIs allow.
          // But if newer Play versions introduce incompatibilities,
-@@ -1074,8 +844,9 @@ object scalanativelib extends MillModule {
+@@ -1080,8 +849,9 @@ object scalanativelib extends MillModule {
      override def ivyDeps = Agg(Deps.sbtTestInterface)
    }
    object worker extends Cross[WorkerModule]("0.4")
@@ -352,7 +362,7 @@ index d7275ab3de..96c5bc0d9c 100644
      def testDepPaths = T{ Seq(compile().classes) }
      override def moduleDeps = Seq(scalanativelib.`worker-api`)
      override def ivyDeps = scalaNativeWorkerVersion match {
-@@ -1228,7 +999,9 @@ trait IntegrationTestModule extends MillScalaModule {
+@@ -1231,7 +1001,9 @@ trait IntegrationTestModule extends MillScalaModule {
    }
  }
 
@@ -363,7 +373,7 @@ index d7275ab3de..96c5bc0d9c 100644
    object local extends ModeModule{
      def testTransitiveDeps = super.testTransitiveDeps() ++ Seq(
        runner.linenumbers.testDep(),
-@@ -1254,15 +1027,15 @@ object example extends MillScalaModule {
+@@ -1252,15 +1024,15 @@ object example extends MillScalaModule {
 
    def moduleDeps = Seq(integration)
 
@@ -387,7 +397,7 @@ index d7275ab3de..96c5bc0d9c 100644
      def sources = T.sources()
      def testRepoRoot: T[PathRef] = T.source(millSourcePath)
      def compile = example.compile()
-@@ -1312,7 +1085,7 @@ object example extends MillScalaModule {
+@@ -1310,7 +1082,7 @@ object example extends MillScalaModule {
                val title =
                  if (seenCode) ""
                  else {
@@ -396,7 +406,7 @@ index d7275ab3de..96c5bc0d9c 100644
                    val exampleDashed = examplePath.segments.mkString("-")
                    val download = s"{mill-download-url}/$label-$exampleDashed.zip[download]"
                    val browse = s"{mill-example-url}/$examplePath[browse]"
-@@ -1343,9 +1116,9 @@ object example extends MillScalaModule {
+@@ -1341,9 +1113,9 @@ object example extends MillScalaModule {
  }
 
  object integration extends MillScalaModule {
@@ -409,7 +419,18 @@ index d7275ab3de..96c5bc0d9c 100644
 
    def moduleDeps = Seq(scalalib, scalajslib, scalanativelib, runner.test)
 
-@@ -1677,67 +1450,11 @@ object docs extends Module {
+@@ -1640,8 +1412,8 @@ object dev extends MillModule {
+     mill.modules.Jvm.createJar(Agg(), mill.modules.Jvm.JarManifest(manifestEntries))
+   }
+
+-  def run(args: String*) = T.command {
+-    args match {
++  def run(args: Task[Args] = T.task(Args())) = T.command {
++    args().value match {
+       case Nil => mill.api.Result.Failure("Need to pass in cwd as first argument to dev.run")
+       case wd0 +: rest =>
+         val wd = os.Path(wd0, T.workspace)
+@@ -1668,67 +1440,11 @@ object docs extends Module {
      def moduleDeps = build.millInternal.modules.collect { case m: MillApiModule => m }
 
      def unidocSourceUrl = T {
@@ -478,7 +499,7 @@ index d7275ab3de..96c5bc0d9c 100644
    private val npmExe = if (scala.util.Properties.isWin) "npm.cmd" else "npm"
    private val antoraExe = if (scala.util.Properties.isWin) "antora.cmd" else "antora"
    def npmBase: T[os.Path] = T.persistent { T.dest }
-@@ -1790,7 +1507,7 @@ object docs extends Module {
+@@ -1781,7 +1497,7 @@ object docs extends Module {
 
      val contribReadmes = T.traverse(contrib.contribModules)(m =>
        T.task {
@@ -487,7 +508,7 @@ index d7275ab3de..96c5bc0d9c 100644
        }
      )()
 
-@@ -2019,7 +1736,7 @@ def exampleZips: Target[Seq[PathRef]] = T {
+@@ -2011,7 +1727,7 @@ def exampleZips: Target[Seq[PathRef]] = T {
      examplePath = exampleMod.millSourcePath
    } yield {
      val example = examplePath.subRelativeTo(T.workspace)
@@ -496,7 +517,7 @@ index d7275ab3de..96c5bc0d9c 100644
      os.copy(examplePath, T.dest / exampleStr, createFolders = true)
      os.copy(launcher().path, T.dest / exampleStr / "mill")
      val zip = T.dest / s"$exampleStr.zip"
-@@ -2028,49 +1745,7 @@ def exampleZips: Target[Seq[PathRef]] = T {
+@@ -2020,49 +1736,7 @@ def exampleZips: Target[Seq[PathRef]] = T {
    }
  }
 

contrib/artifactory/src/mill/contrib/artifactory/ArtifactoryPublishModule.scala

@@ -94,7 +94,5 @@ object ArtifactoryPublishModule extends ExternalModule {
     }
   }
 
-  import mill.main.TokenReaders._
-
   lazy val millDiscover: mill.define.Discover[this.type] = mill.define.Discover[this.type]
 }

contrib/bintray/src/mill/contrib/bintray/BintrayPublishModule.scala

@@ -100,7 +100,5 @@ object BintrayPublishModule extends ExternalModule {
     }
   }
 
-  import mill.main.TokenReaders._
-
   lazy val millDiscover: mill.define.Discover[this.type] = mill.define.Discover[this.type]
 }

contrib/bloop/src/mill/contrib/bloop/BloopImpl.scala

@@ -3,7 +3,7 @@ package mill.contrib.bloop
 import bloop.config.{Config => BloopConfig, Tag => BloopTag}
 import mill._
 import mill.api.Result
-import mill.define.{Module => MillModule, _}
+import mill.define.{Module => MillModule, ExternalModule, Discover}
 import mill.eval.Evaluator
 import mill.scalalib.internal.ModuleUtils
 import mill.scalajslib.ScalaJSModule

contrib/buildinfo/test/src/mill/contrib/buildinfo/BuildInfoTests.scala

@@ -159,7 +159,7 @@ object BuildInfoTests extends TestSuite {
     "run" - workspaceTest(BuildInfoPlain, "scala") { eval =>
       val runResult = eval.outPath / "hello-mill"
       val Right((result, evalCount)) =
-        eval.apply(BuildInfoPlain.run(runResult.toString))
+        eval.apply(BuildInfoPlain.run(T.task(Args(runResult.toString))))
 
       assert(
         os.exists(runResult),
@@ -173,7 +173,7 @@ object BuildInfoTests extends TestSuite {
       val runResult = eval.outPath / "hello-mill"
 
       val Right((result2, evalCount2)) =
-        eval.apply(BuildInfoStatic.run(runResult.toString))
+        eval.apply(BuildInfoStatic.run(T.task(Args(runResult.toString))))
 
       assert(os.exists(buildInfoSourcePath(eval)))
       assert(!os.exists(buildInfoResourcePath(eval)))
@@ -184,7 +184,7 @@ object BuildInfoTests extends TestSuite {
     "java" - workspaceTest(BuildInfoJava, "java") { eval =>
       val runResult = eval.outPath / "hello-mill"
       val Right((result, evalCount)) =
-        eval.apply(BuildInfoJava.run(runResult.toString))
+        eval.apply(BuildInfoJava.run(T.task(Args(runResult.toString))))
 
       assert(
         os.exists(runResult),

contrib/codeartifact/src/mill/contrib/codeartifact/CodeartifactPublishModule.scala

@@ -58,8 +58,6 @@ object CodeartifactPublishModule extends ExternalModule {
       )
     }
 
-  import mill.main.TokenReaders._
-
   lazy val millDiscover: mill.define.Discover[this.type] =
     mill.define.Discover[this.type]
 }

contrib/gitlab/src/mill/contrib/gitlab/GitlabPublishModule.scala

@@ -79,7 +79,5 @@ object GitlabPublishModule extends ExternalModule {
     )
   }
 
-  import mill.main.TokenReaders._
-
   lazy val millDiscover: mill.define.Discover[this.type] = mill.define.Discover[this.type]
 }

contrib/playlib/src/mill/playlib/PlayModule.scala

@@ -1,8 +1,9 @@
 package mill.playlib
 
+import mill.define.Task
 import mill.playlib.api.Versions
 import mill.scalalib._
-import mill.{Agg, T}
+import mill.{Agg, Args, T}
 
 trait PlayApiModule extends Dependencies with Router with Server {
   trait PlayTests extends super.Tests with TestModule.ScalaTest {
@@ -17,7 +18,7 @@ trait PlayApiModule extends Dependencies with Router with Server {
     override def sources = T.sources { millSourcePath }
   }
 
-  def start(args: String*) = T.command { run(args: _*) }
+  def start(args: Task[Args] = T.task(Args())) = T.command { run(args) }
 
 }
 trait PlayModule extends PlayApiModule with Static with Twirl

contrib/proguard/src/mill/contrib/proguard/Proguard.scala

@@ -1,17 +1,12 @@
 package mill.contrib.proguard
 
-import scala.util.Properties
-
-import coursier.Repositories
 import mill.java9rtexport.Export
 import mill.T
 import mill.Agg
-import mill.api.{Logger, Loose, PathRef, Result}
-import mill.define.{Sources, Target}
+import mill.api.{Loose, PathRef}
 import mill.modules.Jvm
-import mill.scalalib.Lib.resolveDependencies
-import mill.scalalib.{Dep, DepSyntax, Lib, ScalaModule}
-import os.{Path, PathChunk, Shellable, proc}
+import mill.scalalib.{DepSyntax, ScalaModule}
+import os.{Path, Shellable}
 
 /**
  * Adds proguard capabilities when mixed-in to a module

contrib/scalapblib/src/mill/contrib/scalapblib/ScalaPBModule.scala

@@ -3,7 +3,6 @@ package contrib.scalapblib
 
 import coursier.MavenRepository
 import coursier.core.Version
-import mill.define.Sources
 import mill.api.{IO, Loose, PathRef}
 import mill.scalalib.Lib.resolveDependencies
 import mill.scalalib._

contrib/scalapblib/test/src/mill/contrib/scalapblib/TutorialTests.scala

@@ -1,8 +1,7 @@
 package mill.contrib.scalapblib
 
-import mill.T
+import mill._
 import mill.api.{PathRef, Result}
-import mill.define.Sources
 import mill.util.{TestEvaluator, TestUtil}
 import utest.framework.TestPath
 import utest.{TestSuite, Tests, assert, _}

contrib/scoverage/src/mill/contrib/scoverage/ScoverageModule.scala

@@ -4,7 +4,6 @@ import coursier.Repository
 import mill._
 import mill.api.{Loose, PathRef}
 import mill.contrib.scoverage.api.ScoverageReportWorkerApi.ReportType
-import mill.define.{Command, Persistent, Sources, Target, Task}
 import mill.scalalib.api.ZincWorkerUtil
 import mill.scalalib.{Dep, DepSyntax, JavaModule, ScalaModule}
 import mill.api.Result

contrib/twirllib/src/mill/twirllib/TwirlModule.scala

@@ -2,7 +2,6 @@ package mill
 package twirllib
 
 import coursier.{Dependency, Repository}
-import mill.define.{Sources, Task}
 import mill.api.PathRef
 import mill.scalalib._
 import mill.api.Loose

contrib/versionfile/src/mill/contrib/versionfile/Version.scala

@@ -69,8 +69,9 @@ object Version {
   implicit val readWriter: ReadWriter[Version] =
     readwriter[String].bimap(_.toString, Version.of)
 
-  implicit val read: mainargs.TokensReader[Version] = new mainargs.TokensReader[Version](
-    "<version>",
-    s => Right(Version.of(s.last))
-  )
+  implicit val read: mainargs.TokensReader.Simple[Version] =
+    new mainargs.TokensReader.Simple[Version] {
+      def shortName = "<version>"
+      def read(s: Seq[String]) = Right(Version.of(s.last))
+    }
 }

contrib/versionfile/src/mill/contrib/versionfile/VersionFileModule.scala

@@ -5,7 +5,7 @@ import mill._, scalalib._
 trait VersionFileModule extends Module {
 
   /** The file containing the current version. */
-  def versionFile: define.Source = T.source(millSourcePath / "version")
+  def versionFile: Source = T.source(millSourcePath / "version")
 
   /** The current version. */
   def currentVersion: T[Version] = T { Version.of(os.read(versionFile().path).trim) }
@@ -90,7 +90,5 @@ object VersionFileModule extends define.ExternalModule {
     } yield proc.call()
   }
 
-  import mill.main.TokenReaders._
-
   lazy val millDiscover: mill.define.Discover[this.type] = mill.define.Discover[this.type]
 }