mill icon indicating copy to clipboard operation
mill copied to clipboard

[WIP] Make `Evaluator` only available to `Task.Command(exclusive = true)`

Open lihaoyi opened this issue 1 year ago • 3 comments

This is a breaking change that tightens things up a bit following the introduction of Task.Command(exclusive = true): now that we run commands in parallel, running multiple evaluator commands in parallel is dangerous, so they should be limited to exclusive commands

It's another step toward https://github.com/com-lihaoyi/mill/issues/502, by declaring that exclusive commands are different from normal commands, and thus need special treatment

Despite being binary compatible, we might not want to land this in 0.12.0, because it would break basically all usages of Evaluator commands in third-party plugins and force them to be updated, which defeats the purpose of maintaining bincompat. Probably better to wait till 0.13.0 when third-party plugins need to be fixed/recompiled anyway, and we can change the API (e.g. move to Task.evaluator) to force people to update their commands rather than happily compiling and then failing later at runtime

Had to jump through some hoops to do this without breaking bincompat:

  • We inject the Evaluator parameter during the resolution phase, before evaluation starts,
  • At that time, we do not yet know whether a command is exclusive or not.
  • The solution is to instead pass a ProxyEvaluator that exposes all the same API, but looks up the current Evaluator.currentEvaluatorSafe on-demand
  • Evaluator.currentEvaluatorSafe is a version of Evaluator.currentEvaluator that only exists within exclusive commands, so we can fail with a nice error message if someone tries to use the evaluator inside a non-exclusive command
  • We cannot remove other usages of Evaluator.currentEvaluator,
    • e.g. in the mill.main.Tasks.TokensReader actually needs to resolve the tasks early on and return a Seq, which we cannot easily stub out.
    • But that's probably OK, because it doesn't expose the entire Evaluator, and the tasks themselves are immutable and probably safe to use concurrency

lihaoyi avatar Oct 11 '24 08:10 lihaoyi

This might not be possible without breaking bincompat due to the fact that we inject the Evaluator parameter during the resolution phase, before evaluation starts, during which we do not yet know whether a command is exclusive or not.

lihaoyi avatar Oct 11 '24 08:10 lihaoyi

This would defeat the initial use case, that let us to introduce the exclusive tag in the first place.

  • https://github.com/com-lihaoyi/mill/issues/3359

Dependency/showUpdates is an evaluator command, that is non-exclusive but profits heavily from being parallel-runnable.

lefou avatar Oct 12 '24 09:10 lefou

@lefou If I'm not mistaken Dependency/showUpdates should be fine, since only the top-level mill.scalalib.Dependency/showUpdates command would need to be exclusive, and indeed only one showUpdates task is running at a time. Internally, showUpdates ends up calling a bunch of javaModule.* targets, and those both are neither exclusive nor are they evaluator commands. So I think it should continue to work unchange even if we land this limitation

lihaoyi avatar Oct 14 '24 04:10 lihaoyi