Add ability to watch T.inputs and interp.watchValues

[lihaoyi]

Fixes #838 and also fixes #909

We add the ability to re-run T.input{...} blocks during the polling loop, just like we re-stat the os.Paths reported by T.source and T.sources. This involves some complexity, since T.input{...} blocks are not just a single lambda but instead a wrapper for a part of the Task graph that feeds into it. We capture the body of evaluateGroup in a lambda so we can call it later

Major Changes

1. Modify Evaluator.Results#results from a Map[Task, Result] to Map[Task, TaskResult], where TaskResult[T] is a wrapper of Result[T] that also contains a recalcOpt: Option[() => Result[T]] callback

2. Wrap the bulk of Evaluator#evaluateGroup in a function, that we evaluate once to populate TaskResult#result and store in a callback to populate TaskResult#recalcOpt

3. Update RunScript.evaluateNamed to handle InputImpls, in addition to the current handling of SourceImpls and SourcesImpls, and translate the recalcOpt callback into a Watchable.Value

4. The Watchable.Value is then propagated to MillBuildBootstrap.evaluateWithWatches, which ends up feeding it into Watching.scala where it is processed the same way as the Watchable.Values we get from interp.watchValue

5. Overhauled the implementation of show; rather than returning a Watched wrapper, we now call back to interp.evalWatch0 to register the things we need to watch. I also consolidated the common code in show and showNamed into one shared helper.

6. I wrapped most of the Anys or _s in Evaluator with a new case class Val(value: Any) wrapper. This should considerably increase type safety: e.g. you can no longer pass an Option[Val] where a Val was expected, unless you manually wrap it in Val(...) which is hard to do accidentally. This makes it much easier to ensure the various data structures inside Evaluator are passed around correctly and not mis-used

Minor Changes

1. Cleaned up RunScripts considerably, now there's only two methods left and no copy-pasty code

2. Removed some T.inputs from MillBuildRootModule: def unmanagedClasspath can be a T.sources, and def parseBuildFiles is now a normal target that depends on allBuildFiles which is a T.sources. That should improve the --watch status message in the terminal and reduce the amount of work that needs to be done every 100ms while polling for changes

Testing

Tested manually with the following build.sc; verified with -w bar that bar re-runs every 2s, and the script re-evaluates every 10s

import mill._

interp.watchValue(System.currentTimeMillis() / 10000)

println("Setting up build.sc")
def foo = T.input{ System.currentTimeMillis() / 2000 }

def bar = T{ foo() + " seconds since the epoch" }

Also added integration/feature/watch-source-input/ suite that starts up a --watch in a background thread, makes changes to the files on disk, and ensures the build.sc and tasks within it re-evaluate as expected.

Notes

1. I'm not sure if the new way show registers watches is better than the old one. The old way with Watched can be made to work even in failure cases if we pass it as the optional second parameter to Result.Failure

2. The Val change is not strictly necessary, and is pretty invasive. But it was very hard to make the necessary changes to Evaluator without it due to bugs passing the wrong Anys around. So I think it's a reasonable time to introduce Val to support future work inside Evaluator with increased compiler support and type safety

3. The recalc logic would be a lot simpler if inputs couldn't have upstream tasks, since we could just re-run the block rather than re-evaluating the task group. I had some thoughts about restricting T.inputs to allow this, but couldn't convince myself that was the right thing to do, so left the T.input semantics in place and just made it work by capturing a callback that can evaluate the entire task group

[lefou]

Happy to see the Watched removed. But I don't get the recalc thing yet.

[lefou]

This version does not catch exceptions and converts them into Failing. Is this on purpose?

[lefou]

Can you elaborate on the purpose of recalc and add some API doc?

I only have a fuzzy idea how I should fill the recalc arg of a newly created Success.

[Ammonite-Bot]

Yeah this is still pretty rough, not totally happy with it yet

…

On Tue, 2 May 2023 at 2:53 AM, Tobias Roeser ***@***.***> wrote: ***@***.**** commented on this pull request. Happy to see the Watched removed. But I don't get the recalc thing yet. ------------------------------ In main/src/mill/main/VisualizeModule.scala <#2489 (comment)>: > @@ -43,7 +43,7 @@ trait VisualizeModule extends mill.define.TaskModule { ) val visualizeThread = new java.lang.Thread(() => while (true) { - val res = Result.create { + val res = Result.Success{ This version does not catch exceptions and converts them into Failing. Is this on purpose? ------------------------------ In main/api/src/mill/api/Result.scala <#2489 (comment)>: > @@ -27,11 +27,14 @@ object Result { * @param value The value computed by the task. * @tparam T The result type of the computed task. */ - case class Success[+T](value: T) extends Result[T] { - def map[V](f: T => V): Success[V] = Result.Success(f(value)) + case class Success[+T](value: T, recalc: () => T) extends Result[T] { Can you elaborate on the purpose of recalc and add some API doc? I only have a fuzzy idea how I should fill the recalc arg of a newly created Success. — Reply to this email directly, view it on GitHub <#2489 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE5HBDDLYKXZSKQDBPST6Z3XEABCLANCNFSM6AAAAAAXR5UAAM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[lefou]

Is this version no longer supported?

[lefou]

Are these versions no longer supported? AFAIK, those partly changed/extended their API, and those tests make sure we can still build against them.

[lihaoyi]

I thought we could kill them, but I'll revert this and leave it for another PR

[lihaoyi]

@lefou this should be ready for another round of review I think, CI is finally green

[lefou]

Making Evaluator even more complex will probably hit use later (at least me, as complex code tends to see less (necessary) changes), but I also think that T.input and in general a more robust watch implementation is a very nice thing.

I was hoping, that we somehow could track watchable values directly from Evaluator and keep it in some context, as the current infrastructure you build up in MainModule to support show and showNamed still looks a bit fragile and specific. I wonder whether each evaluator target needs to deal with this kind of handling at some point?

I think, this change is OK to merge.

[lihaoyi]

Yeah Evaluator has definitely grown a bit messy. I can see if I can refactor it a bit in a follow up