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

Fix recent regressions not allowing Metals to pass its tests #2363

Merged
merged 2 commits into from
Jul 4, 2024
Merged

Fix recent regressions not allowing Metals to pass its tests #2363

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
50ec183
Fix regression introduced in the best effort PR
jchyb Jul 4, 2024
5c804c4
Fix second regression
jchyb Jul 4, 2024
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
4 changes: 2 additions & 2 deletions frontend/src/main/scala/bloop/bsp/BloopBspServices.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ final class BloopBspServices(
}

val isPipeline = compileArgs.exists(_ == "--pipeline")
val isBestEffort = compileArgs.exists(_ == "--best-effort")
val bestEffortAllowed = compileArgs.exists(_ == "--best-effort")
def compile(projects: List[Project]): Task[State] = {
val config = ReporterConfig.defaultFormat.copy(reverseOrder = false)

Expand Down Expand Up @@ -486,7 +486,7 @@ final class BloopBspServices(
dag,
createReporter,
isPipeline,
isBestEffort,
bestEffortAllowed,
cancelCompilation,
store,
logger
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/main/scala/bloop/engine/Interpreter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ object Interpreter {
dag,
createReporter,
cmd.pipeline,
bestEffort = false,
bestEffortAllowed = false,
Promise[Unit](),
CompileClientStore.NoStore,
state.logger
Expand Down
119 changes: 60 additions & 59 deletions frontend/src/main/scala/bloop/engine/tasks/CompileTask.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ object CompileTask {
dag: Dag[Project],
createReporter: ReporterInputs[UseSiteLogger] => Reporter,
pipeline: Boolean,
bestEffort: Boolean,
bestEffortAllowed: Boolean,
cancelCompilation: Promise[Unit],
store: CompileClientStore,
rawLogger: UseSiteLogger
Expand Down Expand Up @@ -276,73 +276,74 @@ object CompileTask {
}

val client = state.client
CompileGraph.traverse(dag, client, store, bestEffort, setup(_), compile).flatMap { pdag =>
val partialResults = Dag.dfs(pdag, mode = Dag.PreOrder)
val finalResults = partialResults.map(r => PartialCompileResult.toFinalResult(r))
Task.gatherUnordered(finalResults).map(_.flatten).flatMap { results =>
val cleanUpTasksToRunInBackground =
markUnusedClassesDirAndCollectCleanUpTasks(results, state, rawLogger)

val failures = results.flatMap {
case FinalNormalCompileResult(p, results) =>
results.fromCompiler match {
case Compiler.Result.NotOk(_) => List(p)
// Consider success with reported fatal warnings as error to simulate -Xfatal-warnings
case s: Compiler.Result.Success if s.reportedFatalWarnings => List(p)
case _ => Nil
}
case _ => Nil
}
CompileGraph.traverse(dag, client, store, bestEffortAllowed, setup(_), compile).flatMap {
pdag =>
val partialResults = Dag.dfs(pdag, mode = Dag.PreOrder)
val finalResults = partialResults.map(r => PartialCompileResult.toFinalResult(r))
Task.gatherUnordered(finalResults).map(_.flatten).flatMap { results =>
val cleanUpTasksToRunInBackground =
markUnusedClassesDirAndCollectCleanUpTasks(results, state, rawLogger)

val failures = results.flatMap {
case FinalNormalCompileResult(p, results) =>
results.fromCompiler match {
case Compiler.Result.NotOk(_) => List(p)
// Consider success with reported fatal warnings as error to simulate -Xfatal-warnings
case s: Compiler.Result.Success if s.reportedFatalWarnings => List(p)
case _ => Nil
}
case _ => Nil
}

val newState: State = {
val stateWithResults = state.copy(results = state.results.addFinalResults(results))
if (failures.isEmpty) {
stateWithResults.copy(status = ExitStatus.Ok)
} else {
results.foreach {
case FinalNormalCompileResult.HasException(project, err) =>
val errMsg = err.fold(identity, Logger.prettyPrintException)
rawLogger.error(s"Unexpected error when compiling ${project.name}: $errMsg")
case _ => () // Do nothing when the final compilation result is not an actual error
}
val newState: State = {
val stateWithResults = state.copy(results = state.results.addFinalResults(results))
if (failures.isEmpty) {
stateWithResults.copy(status = ExitStatus.Ok)
} else {
results.foreach {
case FinalNormalCompileResult.HasException(project, err) =>
val errMsg = err.fold(identity, Logger.prettyPrintException)
rawLogger.error(s"Unexpected error when compiling ${project.name}: $errMsg")
case _ => () // Do nothing when the final compilation result is not an actual error
}

client match {
case _: ClientInfo.CliClientInfo =>
// Reverse list of failed projects to get ~correct order of failure
val projectsFailedToCompile = failures.map(p => s"'${p.name}'").reverse
val failureMessage =
if (failures.size <= 2) projectsFailedToCompile.mkString(",")
else {
s"${projectsFailedToCompile.take(2).mkString(", ")} and ${projectsFailedToCompile.size - 2} more projects"
}
client match {
case _: ClientInfo.CliClientInfo =>
// Reverse list of failed projects to get ~correct order of failure
val projectsFailedToCompile = failures.map(p => s"'${p.name}'").reverse
val failureMessage =
if (failures.size <= 2) projectsFailedToCompile.mkString(",")
else {
s"${projectsFailedToCompile.take(2).mkString(", ")} and ${projectsFailedToCompile.size - 2} more projects"
}

rawLogger.error("Failed to compile " + failureMessage)
case _: ClientInfo.BspClientInfo => () // Don't report if bsp client
}
rawLogger.error("Failed to compile " + failureMessage)
case _: ClientInfo.BspClientInfo => () // Don't report if bsp client
}

stateWithResults.copy(status = ExitStatus.CompilationError)
stateWithResults.copy(status = ExitStatus.CompilationError)
}
}
}

// Schedule to run clean-up tasks in the background
runIOTasksInParallel(cleanUpTasksToRunInBackground)
// Schedule to run clean-up tasks in the background
runIOTasksInParallel(cleanUpTasksToRunInBackground)

val runningTasksRequiredForCorrectness = Task.sequence {
results.flatMap {
case FinalNormalCompileResult(_, result) =>
val tasksAtEndOfBuildCompilation =
Task.fromFuture(result.runningBackgroundTasks)
List(tasksAtEndOfBuildCompilation)
case _ => Nil
val runningTasksRequiredForCorrectness = Task.sequence {
results.flatMap {
case FinalNormalCompileResult(_, result) =>
val tasksAtEndOfBuildCompilation =
Task.fromFuture(result.runningBackgroundTasks)
List(tasksAtEndOfBuildCompilation)
case _ => Nil
}
}
}

// Block on all background task that are running and are required for correctness
runningTasksRequiredForCorrectness
.executeOn(ExecutionContext.ioScheduler)
.map(_ => newState)
.doOnFinish(_ => Task(rootTracer.terminate()))
}
// Block on all background task that are running and are required for correctness
runningTasksRequiredForCorrectness
.executeOn(ExecutionContext.ioScheduler)
.map(_ => newState)
.doOnFinish(_ => Task(rootTracer.terminate()))
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/main/scala/bloop/engine/tasks/TestTask.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ object TestTask {
taskDefs.map {
case TaskDefWithFramework(taskDef, _) =>
selectedTests.get(taskDef.fullyQualifiedName()) match {
case None =>
case None | Some(Nil) =>
Copy link
Contributor Author

@jchyb jchyb Jul 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This fixes the DebugProtocolSuite tests (previously it was

 selectedTests.get(taskDef.fullyQualifiedName()).getOrElse(Nil) match {
              case Nil =>
              case selectedTests =>

and I think the Some(Nil) got lost in the refactor)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ach! Good catch!

taskDef
case Some(value) =>
new TaskDef(
Expand Down
10 changes: 5 additions & 5 deletions frontend/src/main/scala/bloop/engine/tasks/compilation/CompileGraph.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ object CompileGraph {
dag: Dag[Project],
client: ClientInfo,
store: CompileClientStore,
bestEffort: Boolean,
bestEffortAllowed: Boolean,
computeBundle: BundleInputs => Task[CompileBundle],
compile: (Inputs, Boolean, Boolean) => Task[ResultBundle]
): CompileTraversal = {
Expand Down Expand Up @@ -393,7 +393,7 @@ object CompileGraph {
case Leaf(project) =>
val bundleInputs = BundleInputs(project, dag, Map.empty)
setupAndDeduplicate(client, bundleInputs, computeBundle) { bundle =>
compile(Inputs(bundle, Map.empty), bestEffort, isBestEffortDep).map { results =>
compile(Inputs(bundle, Map.empty), bestEffortAllowed && project.isBestEffort, isBestEffortDep).map { results =>
results.fromCompiler match {
case Compiler.Result.Ok(_) => Leaf(partialSuccess(bundle, results))
case _ => Leaf(toPartialFailure(bundle, results))
Expand Down Expand Up @@ -428,8 +428,8 @@ object CompileGraph {
case (_, ResultBundle(f: Compiler.Result.Failed, _, _, _)) => f.bestEffortProducts.isEmpty
case _ => false
}
val continue = bestEffort && depsSupportBestEffort && successfulBestEffort || failed.isEmpty
val dependsOnBestEffort = failed.nonEmpty && bestEffort && depsSupportBestEffort || isBestEffortDep
val continue = bestEffortAllowed && depsSupportBestEffort && successfulBestEffort || failed.isEmpty
val dependsOnBestEffort = failed.nonEmpty && bestEffortAllowed && depsSupportBestEffort || isBestEffortDep

if (!continue) {
// Register the name of the projects we're blocked on (intransitively)
Expand Down Expand Up @@ -459,7 +459,7 @@ object CompileGraph {
val bundleInputs = BundleInputs(project, dag, dependentProducts.toMap)
setupAndDeduplicate(client, bundleInputs, computeBundle) { bundle =>
val inputs = Inputs(bundle, resultsMap)
compile(inputs, bestEffort, dependsOnBestEffort).map { results =>
compile(inputs, bestEffortAllowed && project.isBestEffort, dependsOnBestEffort).map { results =>
results.fromCompiler match {
case Compiler.Result.Ok(_) if failed.isEmpty =>
Parent(partialSuccess(bundle, results), dagResults)
Expand Down
Loading