Use original Evaluators to preserve their association with modules in BSP to avoid using the wrong evaluator
#2692
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lihaoyi
changed the title
Evaluators to preserve their association with modules in BSP to avoid using the wrong evaluator
lefou
reviewed
Aug 14, 2023
| disableCallgraphInvalidation = disableCallgraphInvalidation, | ||
| needBuildSc = true | ||
| ).evaluate() | ||
| lazy val rootModules = evaluators.map(_.rootModule) |
lefou
reviewed
Aug 14, 2023
Comment on lines
45
to
47
| implicit def millAllEvaluatorsTokenReader[T]: mainargs.TokensReader[Seq[Evaluator]] = | ||
| new mill.main.AllEvaluatorsTokenReader[T]() | ||
|
|
There was a problem hiding this comment.
Using a Seq[Evaluator] to express the wish to get all evaluators is a bit vague. A Seq can hold almost everything, also nothing. Should we introduce a dedicated type to hold all evaluators instead?
There was a problem hiding this comment.
Sure, I'll wrap it in Evaluator.AllBootstrapEvaluators(value: Seq[Evaluator])
This was referenced Aug 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Should fix #2643.
The basic problem is previously BSP always used the
frame(0)evaluator passed in toupdateEvaluator, which is meant specifically for evaluating user tasks in the root./build.sc. However, BSP also querying of modules and tasks both inbuild.scas well as meta-builds inmill-build/build.scand nested meta-builds. This ends up evaluating the meta-builds in the sameout/folder as the main build, rather than in theout/mill-build/folders they should be evaluated in.This PR avoids using the
Evaluatorgiven inupdateEvaluator, and instead preserves theEvaluators we already construct as part ofMillBuildBootstrapinmill.bsp.worker.State, and passes them as aSeq[Evaluator]from the enclosingMillBuildBootstrapcode all the way to the Mill-BSP code so we can associate eachModulewith its respectiveEaluator. We then use those associatedEvaluators to perform the actual evaluation throughout theMill{Scala,Java,Jvm,}BuildServerimplementations. There's some additional re-shaping, so that in case a single BSP API call wants to query modules/tasks from multiple evaluators at once we properly group the tasks/modules together according to their respective evaluators for evaluation.By passing the
Seq[Evaluator]from the enclosingMillBuildBootstrap, this also lets us remove the now-redundant call toMillBuildBootstrapburied withinState.scala. We already have all the necessary data structures from the enclosingMillBuildBootstrapand its evaluators, so we don't need to re-run the bootstrap process and re-construct all of it from scratch just for BSPI haven't managed to reproduce the original error, so confirming the fix will have to wait until this lands in master and people can try it out. But I have manually tested using VSCode to load and jump around
example/basic/1-simple-scala/andexample/scalabuilds/10-scala-realistic/, and it seems all the references and modules work and are set up correctly. So at least we have some confidence that this doesn't cause any regressions.This should also supersede #2648, since we now no longer need to re-run the
MillBuildBootstrap/RootModuleFinderlogic at all since we just take the existing list ofEvaluators from the enclosing bootstrap process