From 120ccccaab1ae26e6be4eecfbcba93f3931d3457 Mon Sep 17 00:00:00 2001 From: Adrien Piquerez Date: Thu, 29 Jul 2021 17:39:31 +0200 Subject: [PATCH] Make BSP requests robust to some target failures The request of the form buildTarget/* often take a sequence of build targets as parameter. So far if there is an error on a single build target, the entire request fails. This is not the best because the client wants the result of the other build targets anyway: For example: - workspace/buildTargets: if one build target has an invalid Scala version we still want to import the other ones - buildTarget/scalacOptions: if a dependency cannot be resolved we still want to import the build targets that do not depend on it - buildTarget/scalaMainClasses: if buildTarget does not compile we still want the main classes of the other targets ... The change is to respond to BSP requests with the successful build targets and to ignore the failed ones. This is implemented the same in Bloop since before BSP in sbt. In https://github.com/build-server-protocol/build-server-protocol/issues/204, I made a proposal to also add the failed build targets in the response. --- .../internal/server/BuildServerProtocol.scala | 71 +++++++++++++------ .../src/server-test/buildserver/build.sbt | 15 ++++ .../test/scala/testpkg/BuildServerTest.scala | 25 ++++--- 3 files changed, 77 insertions(+), 34 deletions(-) diff --git a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala index d97e77960..e5417b92f 100644 --- a/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala +++ b/main/src/main/scala/sbt/internal/server/BuildServerProtocol.scala @@ -90,16 +90,16 @@ object BuildServerProtocol { val workspace = Keys.bspFullWorkspace.value val state = Keys.state.value val allTargets = ScopeFilter.in(workspace.scopes.values.toSeq) - val sbtTargets: List[Def.Initialize[Task[BuildTarget]]] = workspace.builds.map { + val sbtTargets = workspace.builds.map { case (buildTargetIdentifier, loadedBuildUnit) => val buildFor = workspace.buildToScope.getOrElse(buildTargetIdentifier, Nil) - sbtBuildTarget(loadedBuildUnit, buildTargetIdentifier, buildFor) + sbtBuildTarget(loadedBuildUnit, buildTargetIdentifier, buildFor).result }.toList Def.task { - val buildTargets = Keys.bspBuildTarget.all(allTargets).value.toVector - val allBuildTargets = buildTargets ++ sbtTargets.join.value - state.respondEvent(WorkspaceBuildTargetsResult(allBuildTargets)) - allBuildTargets + val buildTargets = Keys.bspBuildTarget.result.all(allTargets).value + val successfulBuildTargets = anyOrThrow(buildTargets ++ sbtTargets.join.value) + state.respondEvent(WorkspaceBuildTargetsResult(successfulBuildTargets.toVector)) + successfulBuildTargets } }.value, // https://github.com/build-server-protocol/build-server-protocol/blob/master/docs/specification.md#build-target-sources-request @@ -110,8 +110,8 @@ object BuildServerProtocol { val filter = ScopeFilter.in(workspace.scopes.values.toList) // run the worker task concurrently Def.task { - val items = bspBuildTargetSourcesItem.all(filter).value - val buildItems = workspace.builds.toVector.map { + val items = bspBuildTargetSourcesItem.result.all(filter).value + val buildItems = workspace.builds.map { case (id, loadedBuildUnit) => val base = loadedBuildUnit.localBase val sbtFiles = configurationSources(base) @@ -130,9 +130,10 @@ object BuildServerProtocol { add(pluginData.managedSourceDirectories, SourceItemKind.Directory, generated = true) add(pluginData.managedSources, SourceItemKind.File, generated = true) add(sbtFiles, SourceItemKind.File, generated = false) - SourcesItem(id, all.result()) + Value(SourcesItem(id, all.result())) } - val result = SourcesResult((items ++ buildItems).toVector) + val successfulItems = anyOrThrow(items ++ buildItems) + val result = SourcesResult(successfulItems.toVector) s.respondEvent(result) } }.evaluated, @@ -145,8 +146,9 @@ object BuildServerProtocol { val filter = ScopeFilter.in(workspace.scopes.values.toList) // run the worker task concurrently Def.task { - val items = bspBuildTargetResourcesItem.all(filter).value - val result = ResourcesResult(items.toVector) + val items = bspBuildTargetResourcesItem.result.all(filter).value + val successfulItems = anyOrThrow(items) + val result = ResourcesResult(successfulItems.toVector) s.respondEvent(result) } }.evaluated, @@ -159,8 +161,9 @@ object BuildServerProtocol { // run the worker task concurrently Def.task { import sbt.internal.bsp.codec.JsonProtocol._ - val items = bspBuildTargetDependencySourcesItem.all(filter).value - val result = DependencySourcesResult(items.toVector) + val items = bspBuildTargetDependencySourcesItem.result.all(filter).value + val successfulItems = anyOrThrow(items) + val result = DependencySourcesResult(successfulItems.toVector) s.respondEvent(result) } }.evaluated, @@ -172,8 +175,12 @@ object BuildServerProtocol { workspace.warnIfBuildsNonEmpty(Method.Compile, s.log) val filter = ScopeFilter.in(workspace.scopes.values.toList) Def.task { - val statusCode = Keys.bspBuildTargetCompileItem.all(filter).value.max - s.respondEvent(BspCompileResult(None, statusCode)) + val statusCodes = Keys.bspBuildTargetCompileItem.result.all(filter).value + val aggregatedStatusCode = allOrThrow(statusCodes) match { + case Seq() => StatusCode.Success + case codes => codes.max + } + s.respondEvent(BspCompileResult(None, aggregatedStatusCode)) } }.evaluated, bspBuildTargetCompile / aggregate := false, @@ -188,7 +195,7 @@ object BuildServerProtocol { val filter = ScopeFilter.in(workspace.scopes.values.toList) Def.task { - val items = bspBuildTargetScalacOptionsItem.all(filter).value + val items = bspBuildTargetScalacOptionsItem.result.all(filter).value val appProvider = appConfiguration.value.provider() val sbtJars = appProvider.mainClasspath() val buildItems = builds.map { @@ -197,14 +204,16 @@ object BuildServerProtocol { val scalacOptions = plugins.pluginData.scalacOptions val pluginClassPath = plugins.classpath val classpath = (pluginClassPath ++ sbtJars).map(_.toURI).toVector - ScalacOptionsItem( + val item = ScalacOptionsItem( build._1, scalacOptions.toVector, classpath, new File(build._2.localBase, "project/target").toURI ) + Value(item) } - val result = ScalacOptionsResult((items ++ buildItems).toVector) + val successfulItems = anyOrThrow(items ++ buildItems) + val result = ScalacOptionsResult(successfulItems.toVector) s.respondEvent(result) } }.evaluated, @@ -216,8 +225,9 @@ object BuildServerProtocol { workspace.warnIfBuildsNonEmpty(Method.ScalaTestClasses, s.log) val filter = ScopeFilter.in(workspace.scopes.values.toList) Def.task { - val items = bspScalaTestClassesItem.all(filter).value - val result = ScalaTestClassesResult(items.toVector, None) + val items = bspScalaTestClassesItem.result.all(filter).value + val successfulItems = anyOrThrow(items) + val result = ScalaTestClassesResult(successfulItems.toVector, None) s.respondEvent(result) } }.evaluated, @@ -228,8 +238,9 @@ object BuildServerProtocol { workspace.warnIfBuildsNonEmpty(Method.ScalaMainClasses, s.log) val filter = ScopeFilter.in(workspace.scopes.values.toList) Def.task { - val items = bspScalaMainClassesItem.all(filter).value - val result = ScalaMainClassesResult(items.toVector, None) + val items = bspScalaMainClassesItem.result.all(filter).value + val successfulItems = anyOrThrow(items) + val result = ScalaMainClassesResult(successfulItems.toVector, None) s.respondEvent(result) } }.evaluated, @@ -846,6 +857,20 @@ object BuildServerProtocol { case _ => sys.error(s"unexpected $ref") } + private def anyOrThrow[T](results: Seq[Result[T]]): Seq[T] = { + val successes = results.collect { case Value(v) => v } + val errors = results.collect { case Inc(cause) => cause } + if (successes.nonEmpty || errors.isEmpty) successes + else throw Incomplete(None, causes = errors) + } + + private def allOrThrow[T](results: Seq[Result[T]]): Seq[T] = { + val successes = results.collect { case Value(v) => v } + val errors = results.collect { case Inc(cause) => cause } + if (errors.isEmpty) successes + else throw Incomplete(None, causes = errors) + } + private case class SemanticVersion(major: Int, minor: Int) extends Ordered[SemanticVersion] { override def compare(that: SemanticVersion): Int = { if (that.major != major) major.compare(that.major) diff --git a/server-test/src/server-test/buildserver/build.sbt b/server-test/src/server-test/buildserver/build.sbt index a3299215f..52691a0a5 100644 --- a/server-test/src/server-test/buildserver/build.sbt +++ b/server-test/src/server-test/buildserver/build.sbt @@ -25,3 +25,18 @@ lazy val respondError = project.in(file("respond-error")) ) lazy val util = project + +def somethingBad = throw new MessageOnlyException("I am a bad build target") +// other build targets should not be affected by this bad build target +lazy val badBuildTarget = project.in(file("bad-build-target")) + .settings( + Compile / bspBuildTarget := somethingBad, + Compile / bspBuildTargetSourcesItem := somethingBad, + Compile / bspBuildTargetResourcesItem := somethingBad, + Compile / bspBuildTargetDependencySourcesItem := somethingBad, + Compile / bspBuildTargetScalacOptionsItem := somethingBad, + Compile / bspBuildTargetCompileItem := somethingBad, + Compile / bspScalaMainClasses := somethingBad, + Test / bspBuildTarget := somethingBad, + Test / bspScalaTestClasses := somethingBad, + ) diff --git a/server-test/src/test/scala/testpkg/BuildServerTest.scala b/server-test/src/test/scala/testpkg/BuildServerTest.scala index f9c31ecb7..c7ec754e7 100644 --- a/server-test/src/test/scala/testpkg/BuildServerTest.scala +++ b/server-test/src/test/scala/testpkg/BuildServerTest.scala @@ -7,8 +7,7 @@ package testpkg -import sbt.internal.bsp.SourcesResult -import sbt.internal.bsp.WorkspaceBuildTargetsResult +import sbt.internal.bsp.{ BspCompileResult, SourcesResult, StatusCode, WorkspaceBuildTargetsResult } import sbt.internal.langserver.ErrorCodes import sbt.IO @@ -41,13 +40,15 @@ object BuildServerTest extends AbstractServerTest { val buildServerBuildTarget = result.targets.find(_.displayName.contains("buildserver-build")).get assert(buildServerBuildTarget.id.uri.toString.endsWith("#buildserver-build")) + assert(!result.targets.exists(_.displayName.contains("badBuildTarget"))) } test("buildTarget/sources") { _ => val buildTarget = buildTargetUri("util", "Compile") + val badBuildTarget = buildTargetUri("badBuildTarget", "Compile") svr.sendJsonRpc( s"""{ "jsonrpc": "2.0", "id": "24", "method": "buildTarget/sources", "params": { - | "targets": [{ "uri": "$buildTarget" }] + | "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }] |} }""".stripMargin ) assert(processing("buildTarget/sources")) @@ -88,17 +89,16 @@ object BuildServerTest extends AbstractServerTest { |} }""".stripMargin ) assert(processing("buildTarget/compile")) - assert(svr.waitForString(10.seconds) { s => - (s contains """"id":"32"""") && - (s contains """"statusCode":1""") - }) + val res = svr.waitFor[BspCompileResult](10.seconds) + assert(res.statusCode == StatusCode.Success) } test("buildTarget/scalacOptions") { _ => val buildTarget = buildTargetUri("util", "Compile") + val badBuildTarget = buildTargetUri("badBuildTarget", "Compile") svr.sendJsonRpc( s"""{ "jsonrpc": "2.0", "id": "40", "method": "buildTarget/scalacOptions", "params": { - | "targets": [{ "uri": "$buildTarget" }] + | "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }] |} }""".stripMargin ) assert(processing("buildTarget/scalacOptions")) @@ -176,9 +176,10 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/scalaMainClasses") { _ => val buildTarget = buildTargetUri("runAndTest", "Compile") + val badBuildTarget = buildTargetUri("badBuildTarget", "Compile") svr.sendJsonRpc( s"""{ "jsonrpc": "2.0", "id": "56", "method": "buildTarget/scalaMainClasses", "params": { - | "targets": [{ "uri": "$buildTarget" }] + | "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }] |} }""".stripMargin ) assert(processing("buildTarget/scalaMainClasses")) @@ -210,9 +211,10 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/scalaTestClasses") { _ => val buildTarget = buildTargetUri("runAndTest", "Test") + val badBuildTarget = buildTargetUri("badBuildTarget", "Test") svr.sendJsonRpc( s"""{ "jsonrpc": "2.0", "id": "72", "method": "buildTarget/scalaTestClasses", "params": { - | "targets": [{ "uri": "$buildTarget" }] + | "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }] |} }""".stripMargin ) assert(processing("buildTarget/scalaTestClasses")) @@ -305,9 +307,10 @@ object BuildServerTest extends AbstractServerTest { test("buildTarget/resources") { _ => val buildTarget = buildTargetUri("util", "Compile") + val badBuildTarget = buildTargetUri("badBuildTarget", "Compile") svr.sendJsonRpc( s"""{ "jsonrpc": "2.0", "id": "96", "method": "buildTarget/resources", "params": { - | "targets": [{ "uri": "$buildTarget" }] + | "targets": [{ "uri": "$buildTarget" }, { "uri": "$badBuildTarget" }] |} }""".stripMargin ) assert(processing("buildTarget/resources"))