Skip to content

Send BSP onBuildTargetDidChange notifications to clients when the build changes #4957

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

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

alexarchambault
Copy link
Collaborator

This makes Mil send onBuildTargetDidChange BSP notifications to clients when the build change. If ever the build compilation fails at some point, this tries to keep the former evaluators of build levels that can't be loaded anymore, so that there's less interruption of IDE features for end users (that is, if build.mill is broken, this keeps using an old evaluator for the main build tasks, same for the first meta-build and the main build if mill-build/build.mill is broken, etc.).

In order for the preserved evaluators to keep working, but also to help with concurrency, this makes MillBuildBootstrap copy meta-builds' classes to a temporary directory right before loading them in a class loader, so that concurrent sessions don't remove class files that we need.

Lastly, when --bsp is passed, this makes sure we don't print messages for users in it early on, only BSP messages.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Do we signal/log (over BSP) in any way when we need to fallback to the previous evaluator? If not, I think we should, so the user has a chance to verify.


def initialized = sessionInfo != null

def updateEvaluator(evaluatorsOpt: Option[Seq[EvaluatorApi]]): Unit = {
def updateEvaluator(evaluatorsOpt: Option[Seq[EvaluatorApi]], errored: Boolean): Unit = {
Copy link
Member

@lefou lefou Apr 18, 2025

Choose a reason for hiding this comment

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

Can we have ScalaDoc explaining what error means.

@@ -45,21 +45,98 @@ private class MillBuildServer(
// progresses through:
//
// Set when the client connects
protected var client: BuildClient = scala.compiletime.uninitialized
protected var client: BuildClient = null
Copy link
Member

Choose a reason for hiding this comment

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

Why is uninitialized not enough? Can we use a better type then, e.g. Option?

@lihaoyi
Copy link
Member

lihaoyi commented Apr 19, 2025

I haven't looked at the code in detail since it hasn't been cleaned up yet, but one thing we should do is try to re-use as much logic as possible from the normal --watch codepath. We already have everything necessary to watch sources (including build sources), rebootstrap the meta-builds, and perform an action on their change. We should make use of that logic for updating the BSP server, rather than coming up with our own custom logic here

@alexarchambault alexarchambault mentioned this pull request Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants