Skip to content
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.

Sign up for GitHub

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

improvement: Only reindex workspace on onBuildTargetChanged #5737

Merged
merged 2 commits into from
Oct 18, 2023
Merged

improvement: Only reindex workspace on onBuildTargetChanged #5737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57f98dc
improvement: Only reload workspace on onBuildTargetChanged
tgodzik Oct 11, 2023
74082aa
bugfix: Move Compilers.cancel() to the end of importBuild function
tgodzik Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions metals/src/main/scala/scala/meta/internal/builds/BuildTool.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ trait BuildTool {
}

object BuildTool {

case class Found(buildTool: BuildTool, digest: String)
def copyFromResource(
tempDir: Path,
filePath: String,
Expand Down
4 changes: 2 additions & 2 deletions metals/src/main/scala/scala/meta/internal/metals/Compilations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ final class Compilations(
workspace: () => AbsolutePath,
languageClient: MetalsLanguageClient,
refreshTestSuites: () => Unit,
afterCompilation: () => Unit,
afterSuccesfulCompilation: () => Unit,
isCurrentlyFocused: b.BuildTargetIdentifier => Boolean,
compileWorksheets: Seq[AbsolutePath] => Future[Unit],
onStartCompilation: () => Unit,
Expand Down Expand Up @@ -226,7 +226,7 @@ final class Compilations(
val result = compilation.asScala
.andThen { case result =>
updateCompiledTargetState(result)
afterCompilation()
afterSuccesfulCompilation()

// See https://github.com/scalacenter/bloop/issues/1067
classes.rebuildIndex(
Expand Down
97 changes: 54 additions & 43 deletions metals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class MetalsLspService(
worksheetProvider.onBuildTargetDidCompile(target)
},
onBuildTargetDidChangeFunc = params => {
maybeQuickConnectToBuildServer(params)
onBuildTargetChanges(params)
},
bspErrorHandler,
)
Expand Down Expand Up @@ -1966,11 +1966,19 @@ class MetalsLspService(
buildTool.version,
)

def supportedBuildTool(): Future[Option[BuildTool]] = {
def supportedBuildTool(): Future[Option[BuildTool.Found]] = {
def isCompatibleVersion(buildTool: BuildTool) = {
val isCompatibleVersion = this.isCompatibleVersion(buildTool)
if (isCompatibleVersion) {
Some(buildTool)
buildTool.digest(folder) match {
case Some(digest) =>
Some(BuildTool.Found(buildTool, digest))
case None =>
scribe.warn(
s"Could not calculate checksum for ${buildTool.executableName} in $folder"
)
None
}
} else {
scribe.warn(s"Unsupported $buildTool version ${buildTool.version}")
languageClient.showMessage(
Expand All @@ -1991,10 +1999,12 @@ class MetalsLspService(
}
case buildTools =>
for {
Some(buildTool) <- buildToolSelector.checkForChosenBuildTool(
buildTool <- buildToolSelector.checkForChosenBuildTool(
buildTools
)
} yield isCompatibleVersion(buildTool)
} yield {
buildTool.flatMap(isCompatibleVersion)
}
}
}

Expand All @@ -2008,38 +2018,32 @@ class MetalsLspService(
_ == BloopServers.name
)
buildChange <- possibleBuildTool match {
case Some(buildTool) =>
(buildTool.digest(folder), buildTool) match {
case (None, _) =>
scribe.warn(s"Skipping build import, no checksum.")
Future.successful(BuildChange.None)
case (Some(_), buildTool: ScalaCliBuildTool)
if chosenBuildServer.isEmpty =>
tables.buildServers.chooseServer(ScalaCliBuildTool.name)
val scalaCliBspConfigExists =
ScalaCliBuildTool.pathsToScalaCliBsp(folder).exists(_.isFile)
if (scalaCliBspConfigExists) Future.successful(BuildChange.None)
else
buildTool
.generateBspConfig(
folder,
args =>
bspConfigGenerator.runUnconditionally(buildTool, args),
statusBar,
)
.flatMap(_ => quickConnectToBuildServer())
case (Some(digest), _) if isBloopOrEmpty =>
slowConnectToBloopServer(forceImport, buildTool, digest)
case (Some(digest), _) =>
indexer.reloadWorkspaceAndIndex(
forceImport,
buildTool,
digest,
importBuild,
case Some(BuildTool.Found(buildTool: ScalaCliBuildTool, _))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a refactor to use BuildTool.Found with checksum ready.

if chosenBuildServer.isEmpty =>
tables.buildServers.chooseServer(ScalaCliBuildTool.name)
val scalaCliBspConfigExists =
ScalaCliBuildTool.pathsToScalaCliBsp(folder).exists(_.isFile)
if (scalaCliBspConfigExists) Future.successful(BuildChange.None)
else
buildTool
.generateBspConfig(
folder,
args => bspConfigGenerator.runUnconditionally(buildTool, args),
statusBar,
)
}
.flatMap(_ => quickConnectToBuildServer())
case Some(found) if isBloopOrEmpty =>
slowConnectToBloopServer(forceImport, found.buildTool, found.digest)
case Some(found) =>
indexer.reloadWorkspaceAndIndex(
forceImport,
found.buildTool,
found.digest,
importBuild,
)
case None =>
Future.successful(BuildChange.None)

}
} yield buildChange

Expand Down Expand Up @@ -2114,9 +2118,11 @@ class MetalsLspService(
change
}

private def maybeQuickConnectToBuildServer(
private def onBuildTargetChanges(
params: b.DidChangeBuildTarget
): Unit = {
// Make sure that no compilation is running, if it is it might not get completed properly
Copy link
Contributor

Choose a reason for hiding this comment

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

How exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, I could not reproduce it reliably. My guess is that when you send one compile first and then change using directives in another compile requests a short while later, last one will get cancelled and the first one forgotten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And get stuck in the queue until next cancel.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it's a BatchedFunction thing not a scala-cli thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but it showed up here multiple times when testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

By stuck in the queue to you mean that scala-cli won't send any response to that compile and the BatchedFunction queue is forever locked? If that's the case this is solid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it seems Scala CLI doesn't always send the respond, looks like it's a race condition, but might be hard to fix.

compilations.cancel()
val (ammoniteChanges, otherChanges) =
params.getChanges.asScala.partition { change =>
val connOpt = buildTargets.buildServerOf(change.getTarget)
Expand Down Expand Up @@ -2145,13 +2151,19 @@ class MetalsLspService(
.error("Error re-importing Scala CLI build", exception)
}

if (otherChanges0.nonEmpty)
quickConnectToBuildServer().onComplete {
case Failure(e) =>
scribe.warn("Error refreshing build", e)
case Success(_) =>
scribe.info("Refreshed build after change")
if (otherChanges0.nonEmpty) {
bspSession match {
case None => scribe.warn("No build server connected")
case Some(session) =>
for {
_ <- importBuild(session)
_ <- indexer.profiledIndexWorkspace(runDoctorCheck)
} {
focusedDocument().foreach(path => compilations.compileFile(path))
}
}

}
}

def autoConnectToBuildServer(): Future[BuildChange] = {
Expand Down Expand Up @@ -2238,7 +2250,6 @@ class MetalsLspService(
}

private def importBuild(session: BspSession) = {
compilers.cancel()
val importedBuilds0 = timerProvider.timed("Imported build") {
session.importBuilds()
}
Expand All @@ -2252,7 +2263,7 @@ class MetalsLspService(
}
mainBuildTargetsData.resetConnections(idToConnection)
}
} yield ()
} yield compilers.cancel()
}

private def connectToNewBuildServer(
Expand Down
7 changes: 5 additions & 2 deletions tests/slow/src/test/scala/tests/gradle/GradleLspSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ class GradleLspSuite extends BaseImportSuite("gradle-import") {
_ <- server.server.indexingPromise.future
_ = assert(server.server.bspSession.get.main.isBloop)
buildTool <- server.server.supportedBuildTool()
_ = assertEquals(buildTool.get.executableName, "gradle")
_ = assertEquals(buildTool.get.projectRoot, workspace.resolve("inner"))
_ = assertEquals(buildTool.get.buildTool.executableName, "gradle")
_ = assertEquals(
buildTool.get.buildTool.projectRoot,
workspace.resolve("inner"),
)
_ <- server.didOpen("inner/src/main/scala/A.scala")
_ <- server.didSave("inner/src/main/scala/A.scala")(identity)
_ = assertNoDiff(
Expand Down
36 changes: 36 additions & 0 deletions tests/slow/src/test/scala/tests/scalacli/ScalaCliSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,42 @@ class ScalaCliSuite extends BaseScalaCliSuite(V.scala3) {
} yield ()
}

test("properly-reindex") {
cleanWorkspace()
server.client.importBuild = Messages.ImportBuild.yes
for {
_ <- scalaCliInitialize(useBsp = true)(
s"""|/Main.scala
|//> using scala 2.13.11
|// > using toolkit latest
|
|object Main {
| println(os.pwd)
|}
|
|""".stripMargin
)
_ <- server.server.buildServerPromise.future
_ <- server.didOpen("Main.scala")
_ = assertEquals(
server.client.workspaceDiagnostics,
"""|Main.scala:5:13: error: not found: value os
| println(os.pwd)
| ^^
|""".stripMargin,
)
_ <- server.didSave("Main.scala") { text =>
text.replace("// >", "//>")
}
// cause another compilation to wait on workspace reload, the previous gets cancelled
_ <- server.didSave("Main.scala")(identity)
_ = assertEquals(
server.client.workspaceDiagnostics,
"",
)
} yield ()
}

test("single-file-config") {
cleanWorkspace()
val msg = FileOutOfScalaCliBspScope.askToRegenerateConfigAndRestartBspMsg(
Expand Down
61 changes: 0 additions & 61 deletions tests/unit/src/test/scala/tests/BspBuildChangedSuite.scala
View file
Open in desktop

This file was deleted.

Loading