Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import com.itsaky.androidide.lookup.Lookup
import com.itsaky.androidide.projects.builder.BuildService
import com.itsaky.androidide.resources.R
import com.itsaky.androidide.utils.flashError
import com.itsaky.androidide.utils.requestBuildCancellation
import org.slf4j.Logger
import org.slf4j.LoggerFactory

Expand Down Expand Up @@ -77,34 +78,13 @@ abstract class AbstractCancellableRunAction(
protected abstract fun doExec(data: ActionData): Any

protected fun cancelBuild(): Boolean {
log.info("Sending build cancellation request...")
val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE)
if (builder?.isToolingServerStarted() != true) {
flashError(com.itsaky.androidide.projects.R.string.msg_tooling_server_unavailable)
return false
}

builder.cancelCurrentBuild().whenComplete {
result,
error,
->
if (error != null) {
log.error("Failed to send build cancellation request", error)
return@whenComplete
}

if (!result.wasEnqueued) {
log.warn(
"Unable to enqueue cancellation request reason={} reason.message={}",
result.failureReason,
result.failureReason!!.message,
)
return@whenComplete
}

log.info("Build cancellation request was successfully enqueued...")
}

requestBuildCancellation(builder)
return true
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ import com.itsaky.androidide.tooling.api.messages.BuildRunType
import com.itsaky.androidide.tooling.api.messages.InitializeProjectParams
import com.itsaky.androidide.tooling.api.messages.result.InitializeResult
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.BUILD_CANCELLED
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.CACHE_READ_ERROR
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_DIRECTORY_INACCESSIBLE
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult.Failure.PROJECT_NOT_DIRECTORY
Expand All @@ -95,6 +96,7 @@ import com.itsaky.androidide.utils.DialogUtils.showRestartPrompt
import com.itsaky.androidide.utils.RecursiveFileSearcher
import com.itsaky.androidide.utils.dpToPx
import com.itsaky.androidide.utils.flashError
import com.itsaky.androidide.utils.flashInfo
import com.itsaky.androidide.utils.flashSuccess
import com.itsaky.androidide.utils.flashbarBuilder
import com.itsaky.androidide.utils.onLongPress
Expand Down Expand Up @@ -797,6 +799,18 @@ abstract class ProjectHandlerActivity : BaseEditorActivity() {
manager.projectDir.name
}

// A sync the user stopped is not a failure, and arrives here through the same callback
// as one. ADFA-5542 fixed that for builds and missed this path, which is the one a
// cancelled *sync* takes: the user pressed Stop and got an indefinite red "Project
// initialization failed" for doing so.
if (failure == BUILD_CANCELLED) {
val cancelled = getString(string.info_build_cancelled)
setStatus(cancelled)
flashInfo(cancelled)
editorViewModel.isInitializing = false
return
}

val initFailed =
if (projectName.isNotEmpty()) {
getString(string.msg_project_initialization_failed_with_name, projectName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,11 @@ class BuildCompletedMetric(
putString("build_type", buildType)
putBoolean("success", isSuccess)
putLong("duration_ms", buildResult.durationMs)
// Why it was not successful, which the metric used to drop. A build the user stopped
// and a build that broke both arrive as success=false, so without this the two are
// indistinguishable and every build-success rate counts deliberate cancels as
// failures. isSuccess keeps its meaning -- a cancelled build did not succeed -- and a
// consumer that wants the rate excluding cancels can now compute it.
buildResult.failure?.let { putString("failure_reason", it.name) }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import com.itsaky.androidide.models.Position
import com.itsaky.androidide.models.Range
import com.itsaky.androidide.projects.builder.BuildService
import com.itsaky.androidide.ui.CodeEditorView
import com.itsaky.androidide.utils.requestBuildCancellation
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
Expand Down Expand Up @@ -114,7 +115,9 @@ class EditorPanelDockableContent(
private fun cancelBuild() {
val builder = Lookup.getDefault().lookup(BuildService.KEY_BUILD_SERVICE)
if (builder?.isToolingServerStarted() == true) {
builder.cancelCurrentBuild()
// Through the shared reporter, because this copy threw the result away entirely: a
// Stop the server refused here said nothing at all, not even to the log.
requestBuildCancellation(builder)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@ import com.itsaky.androidide.projects.builder.LaunchResult
import com.itsaky.androidide.resources.R.string
import com.itsaky.androidide.services.builder.GradleBuildService
import com.itsaky.androidide.tooling.api.messages.result.BuildInfo
import com.itsaky.androidide.tooling.api.messages.result.TaskExecutionResult
import com.itsaky.androidide.tooling.events.ProgressEvent
import com.itsaky.androidide.tooling.events.configuration.ProjectConfigurationStartEvent
import com.itsaky.androidide.tooling.events.task.TaskFinishEvent
import com.itsaky.androidide.tooling.events.task.TaskStartEvent
import com.itsaky.androidide.utils.MetricsAnnotationStore
import com.itsaky.androidide.utils.flashError
import com.itsaky.androidide.utils.flashInfo
import com.itsaky.androidide.utils.flashSuccess
import com.itsaky.androidide.viewmodel.BuildOutputViewModel
import org.slf4j.LoggerFactory
Expand All @@ -49,13 +51,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener {
private var buildStartTimeMs: Long = System.currentTimeMillis()
private var lastOutputTimeMs: Long = SystemClock.elapsedRealtime()

/**
* Set when the user asks for the running build to stop, so [onBuildFailed] can tell a cancel
* from a real failure. Cleared as each build is prepared.
*/
@VisibleForTesting
internal var cancelRequested = false

/**
* Whether the build now running drew a "Build started" marker.
*
Expand Down Expand Up @@ -100,10 +95,8 @@ class EditorBuildEventListener : GradleBuildService.EventListener {

override fun prepareBuild(buildInfo: BuildInfo) {
// Before the activity check, not after: this listener outlives any one activity, so a
// build whose outcome arrived with none attached would otherwise leave both flags set for
// the next build to inherit -- a stale cancel mislabelling a real failure, or a stale
// pairing drawing a finish for a build that never started.
cancelRequested = false
// build whose outcome arrived with none attached would otherwise leave the flag set for
// the next build to inherit and draw a finish for a build that never started.
annotatedBuild = false

val act = checkActivity("prepareBuild") ?: return
Expand Down Expand Up @@ -143,6 +136,54 @@ class EditorBuildEventListener : GradleBuildService.EventListener {
}
}

/**
* Whether [failure] is the user's own Stop rather than something going wrong.
*
* One definition, because this callback used to ask the same question three times -- once in
* [failureMessage], once in [outcomeKind] and once inline -- which is how the chart and the
* messages beside it came to disagree in the first place.
*/
@VisibleForTesting
internal fun isCancelled(failure: TaskExecutionResult.Failure?): Boolean = failure == TaskExecutionResult.Failure.BUILD_CANCELLED

/**
* What a failed build is reported as, to the plugins and in the result the editor posts.
*
* [cancelledText] is passed in rather than resolved here so this can be asserted without an
* activity, for the same reason [outcomeKind] is separate: [onBuildFailed] returns early
* without one, so anything decided inside it is unreachable from a test.
*/
@VisibleForTesting
internal fun failureMessage(
failure: TaskExecutionResult.Failure?,
cancelledText: String,
): String =
when {
isCancelled(failure) -> cancelledText
lastStatusLine.contains("BUILD FAILED") -> lastStatusLine
else -> "Build failed. Check build output for details."
}

/**
* Which marker a failed build gets: the user's own cancel, or a real failure (ADFA-5542).
*
* [failure] is the server's own classification of the throwable Gradle raised. The listener
* used to answer this from a flag it set when the cancel was requested, which meant deciding
* from the order two main-thread runnables happened to run in -- and a cancel that overtook
* [prepareBuild] was cleared by it, so the build the user stopped was reported back to them as
* an error.
*
* Separated from [onBuildFailed] so the decision can be tested: that method needs a live
* activity before it reaches this point, and returns early without one.
*/
@VisibleForTesting
internal fun outcomeKind(failure: TaskExecutionResult.Failure?): MetricsAnnotationStore.Kind =
if (isCancelled(failure)) {
MetricsAnnotationStore.Kind.BUILD_CANCELLED
} else {
MetricsAnnotationStore.Kind.BUILD_FAILED
}

private fun resetBuildTimers() {
buildStartTimeMs = System.currentTimeMillis()
lastOutputTimeMs = SystemClock.elapsedRealtime()
Expand Down Expand Up @@ -183,10 +224,6 @@ class EditorBuildEventListener : GradleBuildService.EventListener {
lastStatusLine = ""
}

override fun onBuildCancelRequested() {
cancelRequested = true
}

override fun onProgressEvent(event: ProgressEvent) {
val act = checkActivity("onProgressEvent") ?: return

Expand All @@ -212,31 +249,44 @@ class EditorBuildEventListener : GradleBuildService.EventListener {
@VisibleForTesting
internal fun isAnnotated(event: ProgressEvent): Boolean = event is TaskStartEvent || event is TaskFinishEvent

override fun onBuildFailed(tasks: List<String?>) {
override fun onBuildFailed(
tasks: List<String?>,
failure: TaskExecutionResult.Failure?,
) {
val act = checkActivity("onBuildFailed") ?: return

val cancelled = isCancelled(failure)

if (annotatedBuild) {
// A build the user stopped arrives through this same callback. Marking it as a failure
// would report their own deliberate action back to them in the error colour.
act.recordBuildAnnotation(
if (cancelRequested) {
MetricsAnnotationStore.Kind.BUILD_CANCELLED
} else {
MetricsAnnotationStore.Kind.BUILD_FAILED
},
)
act.recordBuildAnnotation(outcomeKind(failure))
}
annotatedBuild = false
cancelRequested = false

analyzeCurrentFile()
GeneralPreferences.isFirstBuild = false
act.editorViewModel.isBuildInProgress = false
act.flashError(R.string.build_status_failed)
// Everything this method says, not only the chart marker. The annotation was fixed first
// and the three reports beside it were not, so a user who pressed Stop still got a red
// "Build failed" bar, a "Build failed" notification and an isSuccess=false result -- their
// own action read back to them as an error in every place but one.
val cancelledText = act.getString(R.string.info_build_cancelled)
if (cancelled) {
act.flashInfo(R.string.info_build_cancelled)
// The status line under the output too. Gradle prints "BUILD FAILED" for a cancelled
// build like any other, and [onOutput] copies that line into the label, so the label
// sat there contradicting the bar that had just said the build was stopped. This runs
// after onOutput, so it has the last word.
act.setStatus(cancelledText)
} else {
act.flashError(R.string.build_status_failed)
}

val message =
if (lastStatusLine.contains("BUILD FAILED")) lastStatusLine else "Build failed. Check build output for details."
val message = failureMessage(failure, cancelledText)

// The plugin API has no way to say "cancelled" -- IdeServices.onBuildFailed takes an error
// string and nothing else -- so the message is the whole of what a plugin can be told.
pluginBuildService?.notifyBuildFailed(message)

act.notifyBuildResult(BuildResult(isSuccess = false, message = message, launchResult = null))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,6 @@ class GradleBuildService :
null
} else {
object : EventListener {
override fun onBuildCancelRequested() {
runOnUiThread { listener.onBuildCancelRequested() }
}

override fun prepareBuild(buildInfo: BuildInfo) {
runOnUiThread { listener.prepareBuild(buildInfo) }
}
Expand All @@ -197,8 +193,11 @@ class GradleBuildService :
runOnUiThread { listener.onProgressEvent(event) }
}

override fun onBuildFailed(tasks: List<String?>) {
runOnUiThread { listener.onBuildFailed(tasks) }
override fun onBuildFailed(
tasks: List<String?>,
failure: TaskExecutionResult.Failure?,
) {
runOnUiThread { listener.onBuildFailed(tasks, failure) }
}

override fun onOutput(line: String?) {
Expand Down Expand Up @@ -460,11 +459,29 @@ class GradleBuildService :
eventListener?.onBuildSuccessful(result.tasks)
}

/**
* What the notification in the shade says about a build that did not succeed.
*
* Extracted so it can be asserted: [onBuildFailed] needs a live service before it reaches this
* point, which is the same reason [EditorBuildEventListener] separates its own two decisions.
* Without a test here, deleting the cancelled arm left every test green while a build the user
* stopped went back to saying "Build failed" in the shade.
*/
@VisibleForTesting
internal fun notificationStatusFor(failure: TaskExecutionResult.Failure?): Int =
if (failure == TaskExecutionResult.Failure.BUILD_CANCELLED) {
R.string.info_build_cancelled
} else {
R.string.build_status_failed
}

override fun onBuildFailed(result: BuildResult) {
updateNotification(getString(R.string.build_status_failed), false)
// The notification too, not only what reaches the listener: a build the user stopped left
// "Build failed" in the shade whatever the chart said (ADFA-5542).
updateNotification(getString(notificationStatusFor(result.failure)), false)

dispatchBuildResult(result, false)
eventListener?.onBuildFailed(result.tasks)
eventListener?.onBuildFailed(result.tasks, result.failure)
}

private fun dispatchBuildResult(
Expand Down Expand Up @@ -665,9 +682,6 @@ class GradleBuildService :

override fun cancelCurrentBuild(): CompletableFuture<BuildCancellationRequestResult> {
checkServerStarted()
// Before delegating: the cancellation surfaces as a build failure, and the listener needs
// to know it was asked for rather than reporting the user's own action as an error.
eventListener?.onBuildCancelRequested()
return server!!.cancelCurrentBuild()
}

Expand Down Expand Up @@ -816,19 +830,6 @@ class GradleBuildService :

/** Handles events received from a Gradle build. */
interface EventListener {
/**
* Called when the user asks for the running build to stop.
*
* The tooling API reports a cancelled build through [onBuildFailed], so a listener that
* wants to tell the two apart has to be told here.
*
* Deliberately not defaulted. It was, and the forwarding wrapper in [GradleBuildService]
* then quietly inherited the no-op instead of passing it on -- so the cancel never reached
* the real listener, and a build the user stopped went on being annotated as a failure. A
* member with no default cannot be forgotten by a wrapper; the compiler asks for it.
*/
fun onBuildCancelRequested()

/**
* Called just before a build is started.
*
Expand All @@ -855,10 +856,21 @@ class GradleBuildService :
/**
* Called when a build fails.
*
* A build the user cancelled arrives here too, and [failure] is what tells the two apart.
* It comes from the server, which classifies the throwable Gradle raised -- the only place
* the answer is known rather than inferred (ADFA-5542).
*
* @param tasks The tasks that were run.
* @param failure Why the build failed. Never null from this server, which classifies every
* failure before reporting it; nullable because the wire type allows a server that does
* not. A null is treated as an ordinary failure, which is the safe reading -- reporting
* a real failure as a cancel would hide it.
* @see IToolingApiClient.onBuildFailed
*/
fun onBuildFailed(tasks: List<String?>)
fun onBuildFailed(
tasks: List<String?>,
failure: TaskExecutionResult.Failure?,
)

/**
* Called when the output line is received.
Expand Down
Loading
Loading