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

Introduce Task.Command(exclusive = true) and convert Task.Persistent to Task(persistent = true) #3617

Merged
merged 7 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/playlib/src/mill/playlib/RouterModule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ trait RouterModule extends ScalaModule with Version {

protected val routeCompilerWorker: RouteCompilerWorkerModule = RouteCompilerWorkerModule

def compileRouter: T[CompilationResult] = Task.Persistent {
def compileRouter: T[CompilationResult] = Task(persistent = true) {
T.log.debug(s"compiling play routes with ${playVersion()} worker")
routeCompilerWorker.routeCompilerWorker().compile(
routerClasspath = playRouterToolsClasspath(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ trait ScalaPBModule extends ScalaModule {
)
}

def compileScalaPB: T[PathRef] = Task.Persistent {
def compileScalaPB: T[PathRef] = Task(persistent = true) {
ScalaPBWorkerApi.scalaPBWorker()
.compile(
scalaPBClasspath(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ trait ScoverageModule extends ScalaModule { outer: ScalaModule =>
* The persistent data dir used to store scoverage coverage data.
* Use to store coverage data at compile-time and by the various report targets.
*/
def data: T[PathRef] = Task.Persistent {
def data: T[PathRef] = Task(persistent = true) {
// via the persistent target, we ensure, the dest dir doesn't get cleared
PathRef(T.dest)
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/twirllib/src/mill/twirllib/TwirlModule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ trait TwirlModule extends mill.Module { twirlModule =>

def twirlInclusiveDot: Boolean = false

def compileTwirl: T[mill.scalalib.api.CompilationResult] = Task.Persistent {
def compileTwirl: T[mill.scalalib.api.CompilationResult] = Task(persistent = true) {
TwirlWorkerApi.twirlWorker
.compile(
twirlClasspath(),
Expand Down
2 changes: 1 addition & 1 deletion example/depth/tasks/5-persistent-targets/build.mill
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import java.util.zip.GZIPOutputStream

def data = Task.Source(millSourcePath / "data")

def compressedData = Task.Persistent{
def compressedData = Task(persistent = true) {
println("Evaluating compressedData")
os.makeDir.all(Task.dest / "cache")
os.remove.all(Task.dest / "compressed")
Expand Down
6 changes: 6 additions & 0 deletions main/define/src/mill/define/NamedParameterOnlyDummy.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package mill.define

/**
* Dummy class used to mark parameters that come after it as named only parameters
*/
class NamedParameterOnlyDummy private[mill] ()
130 changes: 103 additions & 27 deletions main/define/src/mill/define/Task.scala
Original file line number Diff line number Diff line change
Expand Up @@ -45,22 +45,6 @@ abstract class Task[+T] extends Task.Ops[T] with Applyable[Task, T] {

object Task extends TaskBase {

/**
* [[PersistentImpl]] are a flavor of [[TargetImpl]], normally defined using
* the `Task.Persistent{...}` syntax. The main difference is that while
* [[TargetImpl]] deletes the `T.dest` folder in between runs,
* [[PersistentImpl]] preserves it. This lets the user make use of files on
* disk that persistent between runs of the task, e.g. to implement their own
* fine-grained caching beyond what Mill provides by default.
*
* Note that the user defining a `Task.Persistent` task is taking on the
* responsibility of ensuring that their implementation is idempotent, i.e.
* that it computes the same result whether or not there is data in `T.dest`.
* Violating that invariant can result in confusing mis-behaviors
*/
def Persistent[T](t: Result[T])(implicit rw: RW[T], ctx: mill.define.Ctx): Target[T] =
macro Target.Internal.persistentImpl[T]

/**
* A specialization of [[InputImpl]] defined via `Task.Sources`, [[SourcesImpl]]
* uses [[PathRef]]s to compute a signature for a set of source files and
Expand Down Expand Up @@ -122,6 +106,18 @@ object Task extends TaskBase {
cls: EnclosingClass
): Command[T] = macro Target.Internal.commandImpl[T]

def Command(
t: NamedParameterOnlyDummy = new NamedParameterOnlyDummy,
exclusive: Boolean = false
): CommandFactory = new CommandFactory(exclusive)
class CommandFactory private[mill] (val exclusive: Boolean) extends TaskBase.TraverseCtxHolder {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why shouldn't this be a curried function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC Scala 2 macros had an issue where they didnt allow you to call their parameters with explicit names. Actually I'm not sure if that still applies in 2.13.15 or 3.5.0, but it did at some point. Lemme try again to verify

Copy link
Contributor

@bishabosha bishabosha Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works in scala 3.5 anyway:

scala> def mcr[T: Type](fn: Expr[Either[String, T]], caller: Expr[String], exclusive: Expr[Boolean])(using Quotes): Expr[Either[String, T]] = {
     |   import scala.concurrent.Future
     |   import scala.concurrent.ExecutionContext.Implicits.global
     |   import scala.concurrent.Await
     |   import scala.concurrent.duration.Duration
     |
     |   val exc = exclusive.valueOrAbort
     |   if exc then '{ println($caller); println("EXCLUSIVE"); $fn }
     |   else '{ println($caller); val fut = Future({Thread.sleep(1000); $fn}); Await.result(fut, Duration.Inf) }
     | }

scala> {
     | inline def task[T](inline op: Either[String, T]) = ${ mcr[T]('op, '{"original"}, 'false) }
     | inline def task[T](ignore: Int = -1, inline exclusive: Boolean = false)(inline op: Either[String, T]) = ${ mcr[T]('op, '{"overload"}, 'exclusive) }
     | }

scala> task(exclusive = true) { println(1 + 1); Right(23) }
overload
EXCLUSIVE
2

scala> task(exclusive = false) { println(1 + 1); Right(23) }
overload
2

scala> task{ println(1 + 1); Right(23) }
original
2
val res21: Either[String, Int] = Right(23)

Copy link
Member Author

@lihaoyi lihaoyi Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks like it's still a limitation in 2.13.15

[14186] [error] /Users/lihaoyi/Github/mill/main/src/mill/main/MainModule.scala:403:17: macro applications do not support named and/or default arguments
[14186] [error]     Task.Command(exclusive = true) {
[14186] [error]                 ^

So likely we'll go with the boilerplate-y CommandFactory so this can land in 0.12.0, and in 0.13.0 we can convert to normal curried functions as one of the benefits of being on Scala 3.x. Doesn't need to be immediately, since the CommandFactory approach should work in Scala 3 as well I think

def apply[T](t: Result[T])(implicit
w: W[T],
ctx: mill.define.Ctx,
cls: EnclosingClass
): Command[T] = macro Target.Internal.serialCommandImpl[T]
}

/**
* [[Worker]] is a [[NamedTask]] that lives entirely in-memory, defined using
* `Task.Worker{...}`. The value returned by `Task.Worker{...}` is long-lived,
Expand Down Expand Up @@ -160,6 +156,30 @@ object Task extends TaskBase {
def apply[T](t: Result[T])(implicit rw: RW[T], ctx: mill.define.Ctx): Target[T] =
macro Target.Internal.targetResultImpl[T]

/**
* Persistent tasks are defined using
* the `Task(persistent = true){...}` syntax. The main difference is that while
* [[TargetImpl]] deletes the `T.dest` folder in between runs,
* [[PersistentImpl]] preserves it. This lets the user make use of files on
* disk that persistent between runs of the task, e.g. to implement their own
* fine-grained caching beyond what Mill provides by default.
*
* Note that the user defining a `Task(persistent = true)` task is taking on the
* responsibility of ensuring that their implementation is idempotent, i.e.
* that it computes the same result whether or not there is data in `T.dest`.
* Violating that invariant can result in confusing mis-behaviors
*/
def apply(
t: NamedParameterOnlyDummy = new NamedParameterOnlyDummy,
persistent: Boolean = false
): ApplyFactory = new ApplyFactory(persistent)
class ApplyFactory private[mill] (val persistent: Boolean) extends TaskBase.TraverseCtxHolder {
def apply[T](t: Result[T])(implicit
rw: RW[T],
ctx: mill.define.Ctx
): Target[T] = macro Target.Internal.persistentTargetResultImpl[T]
}

abstract class Ops[+T] { this: Task[T] =>
def map[V](f: T => V): Task[V] = new Task.Mapped(this, f)
def filter(f: T => Boolean): Task[T] = this
Expand Down Expand Up @@ -238,7 +258,7 @@ trait NamedTask[+T] extends Task[T] {
trait Target[+T] extends NamedTask[T]

object Target extends TaskBase {
@deprecated("Use Task.Persistent instead", "Mill after 0.12.0-RC1")
@deprecated("Use Task(persistent = true){...} instead", "Mill after 0.12.0-RC1")
def persistent[T](t: Result[T])(implicit rw: RW[T], ctx: mill.define.Ctx): Target[T] =
macro Target.Internal.persistentImpl[T]

Expand Down Expand Up @@ -362,6 +382,28 @@ object Target extends TaskBase {
)
)
}
def persistentTargetResultImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[Result[T]])(
rw: c.Expr[RW[T]],
ctx: c.Expr[mill.define.Ctx]
): c.Expr[Target[T]] = {
import c.universe._

val taskIsPrivate = isPrivateTargetOption(c)

mill.moduledefs.Cacher.impl0[Target[T]](c)(
reify {
val s1 = Applicative.impl0[Task, T, mill.api.Ctx](c)(t.tree).splice
val c1 = ctx.splice
val r1 = rw.splice
val t1 = taskIsPrivate.splice
if (c.prefix.splice.asInstanceOf[Task.ApplyFactory].persistent) {
new PersistentImpl[T](s1, c1, r1, t1)
} else {
new TargetImpl[T](s1, c1, r1, t1)
}
}
)
}

def targetTaskImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[Task[T]])(
rw: c.Expr[RW[T]],
Expand Down Expand Up @@ -521,6 +563,27 @@ object Target extends TaskBase {
)
}

def serialCommandImpl[T: c.WeakTypeTag](c: Context)(t: c.Expr[T])(
w: c.Expr[W[T]],
ctx: c.Expr[mill.define.Ctx],
cls: c.Expr[EnclosingClass]
): c.Expr[Command[T]] = {
import c.universe._

val taskIsPrivate = isPrivateTargetOption(c)

reify(
new Command[T](
Applicative.impl[Task, T, mill.api.Ctx](c)(t).splice,
ctx.splice,
w.splice,
cls.splice.value,
taskIsPrivate.splice,
exclusive = c.prefix.splice.asInstanceOf[Task.CommandFactory].exclusive
)
)
}

def workerImpl1[T: c.WeakTypeTag](c: Context)(t: c.Expr[Task[T]])(ctx: c.Expr[mill.define.Ctx])
: c.Expr[Worker[T]] = {
import c.universe._
Expand Down Expand Up @@ -581,7 +644,8 @@ object Target extends TaskBase {
* define the tasks, while methods like `T.`[[dest]], `T.`[[log]] or
* `T.`[[env]] provide the core APIs that are provided to a task implementation
*/
class TaskBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] {
class TaskBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx]
with TaskBase.TraverseCtxHolder {

/**
* `T.dest` is a unique `os.Path` (e.g. `out/classFiles.dest/` or `out/run.dest/`)
Expand Down Expand Up @@ -656,16 +720,20 @@ class TaskBase extends Applicative.Applyer[Task, Task, Result, mill.api.Ctx] {
def traverse[T, V](source: Seq[T])(f: T => Task[V]): Task[Seq[V]] = {
new Task.Sequence[V](source.map(f))
}
}

/**
* A variant of [[traverse]] that also provides the [[mill.api.Ctx]] to the
* function [[f]]
*/
def traverseCtx[I, R](xs: Seq[Task[I]])(f: (IndexedSeq[I], mill.api.Ctx) => Result[R])
: Task[R] = {
new Task.TraverseCtx[I, R](xs, f)
}
object TaskBase {
trait TraverseCtxHolder {

/**
* A variant of [[traverse]] that also provides the [[mill.api.Ctx]] to the
* function [[f]]
*/
def traverseCtx[I, R](xs: Seq[Task[I]])(f: (IndexedSeq[I], mill.api.Ctx) => Result[R])
: Task[R] = {
new Task.TraverseCtx[I, R](xs, f)
}
}
}

class TargetImpl[+T](
Expand Down Expand Up @@ -693,8 +761,16 @@ class Command[+T](
val ctx0: mill.define.Ctx,
val writer: W[_],
val cls: Class[_],
val isPrivate: Option[Boolean]
val isPrivate: Option[Boolean],
val exclusive: Boolean
) extends NamedTask[T] {
def this(
t: Task[T],
ctx0: mill.define.Ctx,
writer: W[_],
cls: Class[_],
isPrivate: Option[Boolean]
) = this(t, ctx0, writer, cls, isPrivate, false)
override def asCommand: Some[Command[T]] = Some(this)
// FIXME: deprecated return type: Change to Option
override def writerOpt: Some[W[_]] = Some(writer)
Expand Down
2 changes: 1 addition & 1 deletion main/define/test/src/mill/define/MacroErrorTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ object MacroErrorTests extends TestSuite {
test("persistent") {
val e = compileError("""
object foo extends TestBaseModule{
def a() = Task.Persistent{1}
def a() = Task(persistent = true){1}
}
mill.define.Discover[foo.type]
""")
Expand Down
11 changes: 8 additions & 3 deletions main/eval/src/mill/eval/EvaluatorCore.scala
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,13 @@ private[mill] trait EvaluatorCore extends GroupEvaluator {
val (_, tasksTransitive0) = Plan.plan(Agg.from(tasks0.map(_.task)))

val tasksTransitive = tasksTransitive0.toSet
val (tasks, leafCommands) = terminals0.partition {
case Terminal.Labelled(t, _) if tasksTransitive.contains(t) => true
val (tasks, leafSerialCommands) = terminals0.partition {
case Terminal.Labelled(t, _) =>
if (tasksTransitive.contains(t)) true
else t match {
case t: Command[_] => !t.exclusive
case _ => false
Copy link
Member

@lefou lefou Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lihaoyi Shouldn't this be a true or a !serialCommandExec?

Maybe, serialCommandExec isn't need anymore, since we now have fine control over exclusive execution.

}
case _ => !serialCommandExec
}

Expand All @@ -194,7 +199,7 @@ private[mill] trait EvaluatorCore extends GroupEvaluator {
if (sys.env.contains(EnvVars.MILL_TEST_SUITE)) _ => ""
else contextLoggerMsg0
)(ec)
evaluateTerminals(leafCommands, _ => "")(ExecutionContexts.RunNow)
evaluateTerminals(leafSerialCommands, _ => "")(ExecutionContexts.RunNow)

logger.clearAllTickers()
val finishedOptsMap = terminals0
Expand Down
2 changes: 1 addition & 1 deletion main/eval/src/mill/eval/Terminal.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import mill.define.{NamedTask, Segment, Segments}
/**
* A terminal or terminal target is some important work unit, that in most cases has a name (Right[Labelled])
* or was directly called by the user (Left[Task]).
* It's a T, Task.Worker, Task.Input, Task.Source, Task.Sources, Task.Persistent
* It's a Task, Task.Worker, Task.Input, Task.Source, Task.Sources, Task.Command
*/
sealed trait Terminal {
def render: String
Expand Down
2 changes: 1 addition & 1 deletion main/eval/test/src/mill/eval/TaskTests.scala
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ trait TaskTests extends TestSuite {
def taskInput = Task { input() }
def taskNoInput = Task { task() }

def persistent = Task.Persistent {
def persistent = Task(persistent = true) {
input() // force re-computation
os.makeDir.all(T.dest)
os.write.append(T.dest / "count", "hello\n")
Expand Down
Loading
Loading