--watch does not work for T.input
#838
Milestone
Comments
|
Created a PR #839 which tackles the issue for most common types: |
|
A possible solution looks like this (copied from #839 (comment)): We want to plumb the lambda that |
lihaoyi
added a commit
that referenced
this issue
May 4, 2023
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 `InputImpl`s, in addition to the current handling of `SourceImpl`s and `SourcesImpl`s, 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.Value`s 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 `Any`s 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.input`s 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 ```scala 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 `Any`s 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 `input`s 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.input`s 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See also related issue #832 and it's fix 2b3fb05.
The text was updated successfully, but these errors were encountered: