diff --git a/AGENTS.md b/AGENTS.md index e43c5ad..1f521ac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -551,6 +551,67 @@ mutable at runtime from the phone. process marked its own errors -- and sequences with no meaning on a phone are consumed rather than printed, so a cursor movement cannot arrive looking like a corrupted log. +- **A project's row has three buttons -- Pull, Update, Remove -- and the + first two are the same act at two lengths.** Pull takes the commits and + stops (`?build=false` on the pull route, `BuildState::pull_only`); + Update pulls, builds what that brought in, and installs every APK it + produced. Iris asked for exactly this pair on 2026-09-02, replacing + "Pull & Build" beside a project-wide Rebuild. The point of splitting + them is that bringing a checkout up to date is cheap and something + somebody does while looking, where building and installing is the thing + they press when they want it on the phone -- the same reasoning that + stopped a checkout building. + Update is composed in the app out of the two things it already does + rather than given a route of its own: the pull-and-build reports in the + project's row exactly as Pull does, and each install reports in its own + component's row exactly as that component's own Update does. It skips a + component whose build just failed -- that row already says so, and + asking again would run the same failing command to say it twice -- and + it hands the installer **one** APK at a time. An install intent is + modal and takes the screen, so a project building two APKs would + otherwise have the second intent replace the first and one component + would silently never be installed; the rest wait in + `ComponentState.ReadyToInstall`, which `continuePendingInstalls` + offers when the screen comes back, through the same machinery and the + same two triggers as a download waiting for a wrongly-signed copy to be + removed. + A component's own install button says **Install** rather than "Build" + where nothing is built yet: with the right-hand button now saying + "Build" for a component that is not current, the two read as one word + meaning two things in the same row. It is named for what you end up + with, which is also the half it has that the Build beside it does not. + The project-wide Rebuild is gone with it (each component still has its + own), and with it `ProjectState.Rebuilding` -- the two remaining + project-wide states collapsed into one `Working(what, status)` that + carries its own words, because Pull, Update and moving the checkout + differ in nothing but the label and the old one said "Pulling and + building" for all three. + +- **One predicate decides both the "Up to date" heading and whether + Update can be pressed** (`hasWorkWaiting` in `UpdaterScreen.kt`): they + are the same question, since Update is the button that clears + everything the heading is about, and a card filed under "Up to date" + above a pressable Update is the list disagreeing with itself. Three + ways to have work waiting -- commits on the remote, an APK newer than + the copy installed here, and **a component whose build no longer + matches the checkout**. The third was missing, so switching branches + from the phone left the card sitting under "Up to date" with only the + small note in one component's row to say otherwise. Iris found it by + switching ai-app onto another branch and seeing nothing flagged. + +- **A moved checkout re-asks its remote, because the last answer was + about the branch it left.** `RemoteChecks::recheck` forgets the cached + answer and starts a fresh check, called from `move_checkout` where the + thing that invalidated it happens. Without the forgetting the card + answers for the previous branch with `checkPending` false -- so a + branch one commit behind its upstream reads as one with nothing to + pull, and Pull is disabled about it with nothing on screen saying the + answer predates the move. Measured, not argued: git said `[behind 1]` + while the card said `newCommits: false`. `Checks::forget` is the + primitive, and it deliberately leaves `in_flight` alone for the reason + `update` does -- a worker already running still owns the right to write + its answer. + - **There are three sizes of refresh, and using the wrong one is what makes the list feel like it has a mind of its own.** `refresh()` drops the list to a spinner and asks every remote -- it belongs to arriving, diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt index 004e637..b8e1a05 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt @@ -113,15 +113,26 @@ private fun requestBuildStatus( } /** - * Fetches and fast-forwards the checkout on the build machine, then builds it. + * Fetches and fast-forwards the checkout on the build machine, and builds what that brought in + * unless [build] says not to. + * + * The card's two buttons are this call twice: Pull takes the commits and stops, Update pulls, + * builds, and installs what the build produced. One route rather than two because everything except + * whether a build follows is the same -- the lock on the checkout, the refusal on uncommitted work, + * the unrelated-histories report, and the status polled afterwards. * * [force] resets the checkout onto its upstream instead, throwing away every commit it has that the * remote doesn't. Only ever sent after a plain pull has come back saying the two histories are * unrelated, and only after the dialog that says so has been confirmed -- there is no fast-forward * for that case ever, so the alternative is a project that can never be pulled again. */ -fun pullAndBuild(key: String, force: Boolean = false): BuildStatus = - requestBuildStatus("/apps/$key/pull" + if (force) "?force=true" else "", "POST") +fun pullProject(key: String, force: Boolean = false, build: Boolean = true): BuildStatus { + val query = listOfNotNull("force=true".takeIf { force }, "build=false".takeIf { !build }) + return requestBuildStatus( + "/apps/$key/pull" + if (query.isEmpty()) "" else "?" + query.joinToString("&"), + "POST", + ) +} /** * Moves the checkout onto a branch or a commit on the build machine, then builds what that left diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt index 03e94f3..d7ed137 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -183,24 +183,24 @@ private sealed class ManifestState { */ private sealed class ProjectState { /** + * Something is running against the whole checkout, and [what] is what to call it. + * * [status] is the server's live progress, refreshed on every poll, so the card can say which * step is running rather than showing an unchanging spinner for the length of a build. + * + * The words are carried rather than derived from a variant per action, because the three things + * that produce this state -- Pull, Update, and moving the checkout -- differ in nothing else: + * same lock, same polling, same row. What they must not share is the label, which said "Pulling + * and building" for all of them and so described two of the three wrongly. A small lie in a + * progress bar is the kind that makes a reader stop trusting the rest of the card. */ - data class Pulling(val status: BuildStatus?) : ProjectState() - - /** - * A build somebody asked for outright, as opposed to one a pull or a download brought about. - * Its own state rather than reusing [Pulling], which is drawn as "Pulling and building" -- - * nothing was pulled here, and a bar that says otherwise is the kind of small lie that makes a - * reader stop trusting the rest. - */ - data class Rebuilding(val status: BuildStatus?) : ProjectState() + data class Working(val what: String, val status: BuildStatus?) : ProjectState() /** * Why the last thing asked of the whole project stopped. * * There is no "retry" recorded with it, because the control that would redo it is the one - * sitting beside the message: a project's actions are Pull and Rebuild, both in the card's own + * sitting beside the message: a project's actions are Pull and Update, both in the card's own * row, and both re-enabled by a failure. A component's failure is a [ComponentState.Error] and * gets Retry on its own row, which is what stops a failed pull being retried as a download. */ @@ -245,6 +245,18 @@ private sealed class ComponentState { */ data class WrongKey(val file: File, val mismatch: SigningMismatch) : ComponentState() + /** + * Downloaded, and waiting for the system installer to be free. + * + * Only a project-wide Update produces it, and only for the second and later APKs of a project + * that builds more than one: an install intent is modal and this app is not on screen while it + * is up, so firing two together means the second replaces the first and one component is + * silently never installed. Held in the component's own row instead, and offered again by + * [continuePendingInstalls] when this screen comes back -- the same machinery, and the same two + * triggers, as a download waiting for a wrongly-signed copy to be removed. + */ + data class ReadyToInstall(val file: File) : ComponentState() + /** Why the last thing this component was asked to do stopped. */ data class Error(val message: String) : ComponentState() } @@ -259,7 +271,7 @@ private sealed class ComponentState { * reloaded. */ private val ProjectState?.busy: Boolean - get() = this is ProjectState.Pulling || this is ProjectState.Rebuilding + get() = this is ProjectState.Working private val ComponentState?.busy: Boolean get() = this != null && this !is ComponentState.Error @@ -835,7 +847,7 @@ private fun AppListScreen( entry: ManifestEntry, start: suspend () -> BuildStatus, progress: (BuildStatus?) -> ProjectState, - ) { + ): BuildStatus? { setProject(entry.key, progress(null)) try { var status = withContext(Dispatchers.IO) { start() } @@ -857,7 +869,7 @@ private fun AppListScreen( // dismissible, and a failure that vanished with it would // leave the card looking as though nothing had happened. if (status.unrelatedHistories) forcePull = entry - return + return null } // Kept after the project's own state is cleared below, so a // component that failed still says so once the run it was @@ -874,13 +886,123 @@ private fun AppListScreen( // saying so. setProject(entry.key, null) } + // The run itself, for whatever wants to carry on from it. + // A component that failed inside it is not a failed run -- + // the others built, and Update installs those -- which is why + // this answers with the whole status rather than a yes or no. + return status } catch (e: DownloadServerException) { setProject(entry.key, failure(e)?.let(ProjectState::Error)) + return null } } /** - * Pull acts on the build machine: fetch, fast-forward, rebuild. + * The body of [startUpdate], as a suspending call so that a project-wide Update can run it for + * each of its APKs in turn rather than starting them all at once. + * + * [offerInstall] false stops at the downloaded file, leaving it in the component's own row for + * [continuePendingInstalls] to offer once the installer is free. Only ever false for the second + * and later APKs of one press. + * + * Answers whether it handed something to the installer, which is how a project-wide Update + * knows the installer is taken. Working that out by reading the component states instead cannot + * work, and did not: handing a file to the installer clears that component's state, so the next + * one round the loop found nothing waiting and went to the installer as well -- both intents + * fired, and the second replaced the first on screen. Caught by pressing Update on a project + * that builds two APKs, which is the only place it shows. + */ + suspend fun updateComponent( + entry: ManifestEntry, + component: String, + offerInstall: Boolean = true, + ): Boolean { + return run { + if (entry.needsBuild) { + setComponent(entry.key, component, ComponentState.Preparing(null)) + try { + var status = withContext(Dispatchers.IO) { prepareBuild(entry.key, component) } + // This component's own step, not the project's + // `building`: a sibling being built at the same time + // says yes to that one, and waiting on it would put + // this download behind a build it has nothing to do + // with -- which is the coupling this is here to end. + var started = false + while (status.component(component)?.running == true) { + started = true + setComponent(entry.key, component, ComponentState.Preparing(status)) + delay(BUILD_POLL_INTERVAL_MS) + status = withContext(Dispatchers.IO) { buildStatus(entry.key) } + } + // Only from a build this press actually started. The + // server keeps a component's last outcome until it is + // built again, so an older failure is still sitting + // there -- and reading that one would refuse a + // download because of something already dealt with. + val buildError = status.component(component)?.error?.takeIf { started } + if (buildError != null) { + setComponent(entry.key, component, ComponentState.Error(buildError)) + return@run false + } + } catch (e: DownloadServerException) { + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) + return@run false + } + } + + // Not "downloading" until something is actually coming down: + // the server may still be producing what it is about to send. + setComponent(entry.key, component, ComponentState.Fetching) + val file = + try { + withContext(Dispatchers.IO) { + downloadApk(context, entry, component) { read, total -> + val progress = if (total > 0) read.toFloat() / total else null + setComponent( + entry.key, + component, + ComponentState.Downloading(progress), + ) + } + } + } catch (e: DownloadServerException) { + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) + return@run false + } + // Android refuses a package signed by a different key than + // the copy already here, and says only "App not installed" + // about it -- so the comparison is made here, while there is + // still something on screen to say what happened and what to + // do. Only after the download, because it is the downloaded + // file's own certificate that decides it. + val installed = entry.components.firstOrNull { it.name == component }?.apk?.packageName + val mismatch = installed?.let { + withContext(Dispatchers.IO) { signingMismatch(context, file, it) } + } + if (mismatch != null) { + setComponent(entry.key, component, ComponentState.WrongKey(file, mismatch)) + return@run false + } + if (!offerInstall) { + // Downloaded and waiting its turn at the installer, said + // in this component's own row rather than by a dialog + // covering the card the other install is about. + setComponent(entry.key, component, ComponentState.ReadyToInstall(file)) + return@run false + } + setComponent(entry.key, component, null) + install(file) + true + } + } + + /** + * Pull takes the commits and stops there. + * + * The cheap half of Update, and the one somebody presses while looking: it acts on the build + * machine's checkout and changes nothing on this phone. What it leaves behind is a component + * the card reports as out of date, with its own Build beside it -- and Update, which does the + * whole thing. * * [force] is the answer to the dialog below, and nothing else ever passes it: it abandons * whatever history that checkout has of its own. @@ -889,12 +1011,57 @@ private fun AppListScreen( scope.launch { followBuild( entry, - start = { pullAndBuild(entry.key, force) }, - progress = { ProjectState.Pulling(it) }, + start = { pullProject(entry.key, force, build = false) }, + progress = { ProjectState.Working("Pulling", it) }, ) } } + /** + * Update is the whole way from the remote to this phone: pull, build what that brought in, and + * install every APK it produced. + * + * Written as the two halves in order rather than as a route of its own, because each half is + * already a thing this app does and reports: the pull-and-build reports in the project's row + * exactly as Pull does, and each install reports in its own component's row exactly as that + * component's own Update does. A third path would be a third thing to keep in step with them. + * + * The entry is read again between the halves ([followBuild] ends by re-reading it), because + * what to install is decided by what the build produced -- sizes, variants and packages all + * move under it. + */ + fun startProjectUpdate(entry: ManifestEntry, force: Boolean = false) { + scope.launch { + val built = + followBuild( + entry, + start = { pullProject(entry.key, force) }, + progress = { ProjectState.Working("Updating", it) }, + ) ?: return@launch + val fresh = + (manifestState as? ManifestState.Loaded)?.manifest?.entries?.firstOrNull { + it.key == entry.key + } ?: return@launch + var installerTaken = false + // One at a time, and the installer is only offered the first: + // the rest wait in their own rows until the system's installer + // has been dealt with, which is what `continuePendingInstalls` + // picks up when this screen comes back. Two install intents + // fired together means the second replaces the first on + // screen, and a component silently not installed is worse than + // one that says it is waiting. + for (component in fresh.components.filter { it.apk != null }) { + // Not the one whose build just failed: its row already + // says so, and asking for it again would run the same + // failing command a second time to say it twice. + if (built.component(component.name)?.error != null) continue + val handedOver = + updateComponent(fresh, component.name, offerInstall = !installerTaken) + installerTaken = installerTaken || handedOver + } + } + } + /** * Moves this project's checkout onto a branch or a commit on the build machine, and builds * nothing. @@ -909,21 +1076,7 @@ private fun AppListScreen( followBuild( entry, start = { checkoutTarget(entry.key, target) }, - progress = { ProjectState.Pulling(it) }, - ) - } - } - - /** - * Build because somebody asked, with nothing to pull and nothing looking stale -- which is the - * only way a project already current with its checkout ever records what it was built from. - */ - fun startRebuild(entry: ManifestEntry) { - scope.launch { - followBuild( - entry, - start = { buildNow(entry.key) }, - progress = { ProjectState.Rebuilding(it) }, + progress = { ProjectState.Working("Moving the checkout", it) }, ) } } @@ -931,12 +1084,11 @@ private fun AppListScreen( /** * Builds one component because somebody pressed its Rebuild, and installs nothing. * - * The component-scoped twin of [startRebuild], and the counterpart to [startUpdate] that does - * not end at the phone: Update is "get me this build", which builds only when the rules say the - * output is behind, while this is "build it again" for the cases those rules cannot see — a - * command that reads files nobody declared, an output changed underneath this server, a signing - * key replaced since the APK was made. None of them move a commit, so nothing reads as stale - * and Update does nothing at all. + * The counterpart to [startUpdate] that does not end at the phone: Update is "get me this + * build", which builds only when the rules say the output is behind, while this is "build it + * again" for the cases those rules cannot see — a command that reads files nobody declared, an + * output changed underneath this server, a signing key replaced since the APK was made. None of + * them move a commit, so nothing reads as stale and Update does nothing at all. * * Recorded against the component like every other action on one, so a sibling stays pressable * and silent throughout. @@ -984,75 +1136,7 @@ private fun AppListScreen( * project, this one's bar was drawn under both and both buttons went dead. */ fun startUpdate(entry: ManifestEntry, component: String) { - scope.launch { - if (entry.needsBuild) { - setComponent(entry.key, component, ComponentState.Preparing(null)) - try { - var status = withContext(Dispatchers.IO) { prepareBuild(entry.key, component) } - // This component's own step, not the project's - // `building`: a sibling being built at the same time - // says yes to that one, and waiting on it would put - // this download behind a build it has nothing to do - // with -- which is the coupling this is here to end. - var started = false - while (status.component(component)?.running == true) { - started = true - setComponent(entry.key, component, ComponentState.Preparing(status)) - delay(BUILD_POLL_INTERVAL_MS) - status = withContext(Dispatchers.IO) { buildStatus(entry.key) } - } - // Only from a build this press actually started. The - // server keeps a component's last outcome until it is - // built again, so an older failure is still sitting - // there -- and reading that one would refuse a - // download because of something already dealt with. - val buildError = status.component(component)?.error?.takeIf { started } - if (buildError != null) { - setComponent(entry.key, component, ComponentState.Error(buildError)) - return@launch - } - } catch (e: DownloadServerException) { - setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) - return@launch - } - } - - // Not "downloading" until something is actually coming down: - // the server may still be producing what it is about to send. - setComponent(entry.key, component, ComponentState.Fetching) - val file = - try { - withContext(Dispatchers.IO) { - downloadApk(context, entry, component) { read, total -> - val progress = if (total > 0) read.toFloat() / total else null - setComponent( - entry.key, - component, - ComponentState.Downloading(progress), - ) - } - } - } catch (e: DownloadServerException) { - setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) - return@launch - } - // Android refuses a package signed by a different key than - // the copy already here, and says only "App not installed" - // about it -- so the comparison is made here, while there is - // still something on screen to say what happened and what to - // do. Only after the download, because it is the downloaded - // file's own certificate that decides it. - val installed = entry.components.firstOrNull { it.name == component }?.apk?.packageName - val mismatch = installed?.let { - withContext(Dispatchers.IO) { signingMismatch(context, file, it) } - } - if (mismatch != null) { - setComponent(entry.key, component, ComponentState.WrongKey(file, mismatch)) - return@launch - } - setComponent(entry.key, component, null) - install(file) - } + scope.launch { updateComponent(entry, component) } } /** @@ -1068,14 +1152,32 @@ private fun AppListScreen( * offer exactly as it was, which is what a cancelled dialog should do. */ fun continuePendingInstalls() { + // One at a time, for the reason each of these states exists: the + // installer is modal and takes the screen, so offering two puts + // the second over the first and loses it. + var offered = false componentStates.forEach { (key, states) -> states.forEach { (component, state) -> - if ( - state is ComponentState.WrongKey && - !isInstalled(context, state.mismatch.packageName) - ) { + val ready = + when (state) { + // Removing the old app is what this was waiting + // for; still installed means the removal was + // backed out of, and the offer stays as it was. + is ComponentState.WrongKey -> + !isInstalled(context, state.mismatch.packageName) + is ComponentState.ReadyToInstall -> true + else -> false + } + if (ready && !offered) { + offered = true setComponent(key, component, null) - install(state.file) + install( + when (state) { + is ComponentState.WrongKey -> state.file + is ComponentState.ReadyToInstall -> state.file + else -> return@forEach + } + ) } } } @@ -1426,21 +1528,16 @@ private fun AppListScreen( projectStates[entry.key], componentStates[entry.key], ) || - !entry.built || - entry.newCommits || // Any client of the project being // behind is the project being // behind: a card with one of two // apps waiting has something // waiting. - entry.components.any { component -> - val apk = component.apk ?: return@any false - !isUpToDate( - apk, - installedTimes[entry.key]?.get(component.name), - chosenVariants[entry.key]?.get(component.name), - ) - } + hasWorkWaiting( + entry, + installedTimes[entry.key].orEmpty(), + chosenVariants[entry.key].orEmpty(), + ) } // The two groups render identically; only the up-to-date one @@ -1468,7 +1565,7 @@ private fun AppListScreen( startComponentBuild(built, component) }, onPull = { startPull(entry) }, - onRebuild = { startRebuild(entry) }, + onProjectUpdate = { startProjectUpdate(entry) }, onRefresh = { refreshOne(entry) }, onSettings = { gitIpv4 -> manage(entry) { setAppSettings(entry.key, gitIpv4) } @@ -1638,7 +1735,8 @@ private fun AppCard( /** Build this one component again, whatever the staleness rules make of it. */ onComponentBuild: (ManifestEntry, component: String) -> Unit, onPull: () -> Unit, - onRebuild: () -> Unit, + /** Pull, build what that brought in, and install every APK it produced. */ + onProjectUpdate: () -> Unit, onRefresh: () -> Unit, onSettings: (gitIpv4: Boolean) -> Unit, /** Move this project's checkout onto a branch or commit, and build what that leaves behind. */ @@ -2004,6 +2102,20 @@ private fun AppCard( // disabled, which is the one place that beats the rule // just below: absence is only ambiguous when nothing says // why, and this card says why in the paragraph above it. + // Pull and Update in that order: the same act at two + // lengths, so the cheap one reads first. Both are the + // colour of bringing something in, because that is what + // each of them does -- how far it goes is the word, not + // the colour. + // + // Not while a declaration is waiting to be read. Both + // buttons would build, and the server pulls and stops for + // an unaccepted project -- and a card asking permission + // should not also be offering to act on the thing it is + // asking about. Hidden rather than disabled, which is the + // one place that beats the rule just below: absence is + // only ambiguous when nothing says why, and this card says + // why in the paragraph above it. if (entry.canPull && !awaitingApproval) { TextButton( onClick = onPull, @@ -2022,28 +2134,23 @@ private fun AppCard( projectState is ProjectState.Error) && !projectBusy, colors = ActionTone.Primary.colors(), ) { - Text("Pull & Build") + Text("Pull") } } - if (!awaitingApproval && entry.needsBuild) { - // Held apart from both neighbours by its own weights, - // because it belongs to neither: it is not the pull, - // and it is emphatically not the remove. + if (!awaitingApproval) { Spacer(Modifier.weight(1f)) TextButton( - onClick = onRebuild, - enabled = !projectBusy, - // The colour Restart and Reinstall wear: it - // certainly does something, and what it leaves - // behind is not obvious from here. - colors = ActionTone.Caution.colors(), + onClick = onProjectUpdate, + // Exactly when the card is not up to date, which is + // the same question the heading above the list + // answers: this is the button that clears it. + // Disabled rather than absent, so what the card can + // do stays visible when there is nothing to do. + enabled = + !projectBusy && hasWorkWaiting(entry, installedTimes, chosenVariants), + colors = ActionTone.Primary.colors(), ) { - // Asked of the components this actually builds: - // one with no command of its own has no output to - // be current with anything, and counting it would - // leave every such project's button saying Build - // for ever. - Text(buildWord(entry.components.filter { it.hasBuild })) + Text("Update") } } if (awaitingApproval) { @@ -2073,11 +2180,8 @@ private fun AppCard( // APK's bar sits under its button: a bar reports on a control, // and one placed away from it belongs to nothing in particular. // This had gone missing entirely when Pull moved up here. - if (projectState is ProjectState.Pulling) { - BuildProgress("Pulling and building", projectState.status) - } - if (projectState is ProjectState.Rebuilding) { - BuildProgress("Building", projectState.status) + if (projectState is ProjectState.Working) { + BuildProgress(projectState.what, projectState.status) } // What is left to say about the card once its components have @@ -2098,7 +2202,8 @@ private fun AppCard( !entry.built && !awaitingApproval -> Text( if (entry.needsBuild) { - "Not built yet -- Build runs this project's build step." + "Not built yet -- Update runs this project's build step and " + + "installs what it produces." } else { "No build found under this project yet." }, @@ -2576,6 +2681,11 @@ private fun ApkProgress(state: ComponentState?) { ) } + // Downloaded, and waiting for the installer to be free. No bar: + // nothing is happening to it, and a bar would say otherwise for as + // long as the other install takes. + is ComponentState.ReadyToInstall -> Text("Downloaded, waiting to install...") + // A pull is the project's, not the APK's, and is drawn at the foot // of the card. Everything else here has nothing to show. else -> {} @@ -2583,7 +2693,7 @@ private fun ApkProgress(state: ComponentState?) { } /** - * Build / Update / Reinstall / Retry: the one control here that puts something on this phone. + * Install / Update / Reinstall / Retry: the one control here that puts something on this phone. * * Its own composable rather than inline in the component card, because what it should say is four * cases of its own -- and deciding the label and the colour together, in one place, is what keeps @@ -2623,9 +2733,16 @@ private fun UpdateButton( // same thing, and picking them apart is how they come to disagree. val (label, tone) = when { - // Nothing to compare against yet, so neither "Update" nor - // "Reinstall" is the honest word for it. - !built -> "Build" to ActionTone.Primary + // Nothing built here yet, so pressing this builds it *and* + // installs it -- named for what you end up with, like the case + // below, because that is the half this button has that the + // Build beside it does not. It said "Build" until the + // right-hand button started saying that too for a component + // that is not current: one row, two buttons, one word, two + // meanings. Neither "Update" nor "Reinstall" is available to + // fall back on here, since there is no build to compare the + // installed copy against. + !built -> "Install" to ActionTone.Go // Nothing here to replace, so this is a first arrival rather than a // newer one -- the same thing Start and Install are elsewhere, and // coloured to match them. @@ -2849,11 +2966,7 @@ private fun componentBuild( // this component's, so a run covering several never draws one // component's work in another's row. (state as? ComponentState.Preparing)?.status?.component(component) - ?: when (projectState) { - is ProjectState.Pulling -> projectState.status - is ProjectState.Rebuilding -> projectState.status - else -> null - }?.component(component) + ?: (projectState as? ProjectState.Working)?.status?.component(component) /** * Says that installing this build would pair it with something older. @@ -3978,12 +4091,39 @@ private fun isBuilding( projectState: ProjectState?, componentStates: Map?, ): Boolean = - projectState is ProjectState.Pulling || - projectState is ProjectState.Rebuilding || + projectState is ProjectState.Working || // Any one component being built is the card having something in // flight, the same way any one component being behind is. componentStates?.values.orEmpty().any { it is ComponentState.Preparing } +/** + * Whether anything about this card is waiting to be done -- which is both what puts it above the + * "Up to date" heading and what makes its Update button pressable. + * + * One definition for the two, because they are the same question: Update is the button that clears + * everything below, so a card it would change is a card that is not up to date, and a heading + * saying otherwise above a pressable Update is the list disagreeing with itself. + * + * Three ways to have work waiting, and the third was missing for a while. Commits on the remote; an + * APK on the build machine newer than the copy installed here; and a component whose build no + * longer matches the checkout -- which is what a branch switch produces, and what nothing here used + * to notice, so moving the checkout left the card filed under "Up to date" with only a small note + * in one component's row to say otherwise. + */ +private fun hasWorkWaiting( + entry: ManifestEntry, + installedTimes: Map, + chosenVariants: Map, +): Boolean = + !entry.built || + entry.newCommits || + entry.components.any { component -> + component.isBehind || + component.apk?.let { + !isUpToDate(it, installedTimes[component.name], chosenVariants[component.name]) + } == true + } + private fun isUpToDate( apk: ComponentApk, installedLastUpdateTimeMillis: Long?, diff --git a/server/src/build_state.rs b/server/src/build_state.rs index e6a6d51..03d1c17 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -756,6 +756,18 @@ impl BuildState { ); } + /// Takes the commits and stops there. + /// + /// The Pull button, as against Update. Splitting them is what lets + /// "bring the checkout up to date" be a cheap thing somebody can do + /// while looking, with building and installing behind the button that + /// says it does those -- and it is the same reasoning that stopped a + /// checkout building. What it leaves behind is a component the card + /// reports as out of date, with its own Build beside it. + pub fn pull_only(self: &Arc, force: bool) { + self.after_moving(move |this| this.pull(force), None); + } + /// Moves this checkout onto `target` -- a branch or a commit -- and /// builds nothing. /// @@ -781,6 +793,14 @@ impl BuildState { this.begin_project("checking out"); crate::git::checkout(&this.project_path, &target)?; tracing::info!("checked out {target} in {}", this.project_path.display()); + // The new-commits answer was about the branch just left, + // and nothing downstream can tell that from an answer + // about this one -- so it is dropped and asked again here, + // where what invalidated it happened. + this.shared.remote_checks.recheck(&crate::git::Checkout { + path: this.project_path.clone(), + ipv4: this.git_ipv4, + }); Ok(true) }, None, @@ -2109,6 +2129,70 @@ mod tests { } } + /// Moving the checkout re-asks the remote, because the last answer was + /// about the branch just left. + /// + /// The answer is cached per project, and the only things that used to + /// start a check were the whole-list refresh and a card's own Refresh + /// -- so after a move the card reported the previous branch's answer + /// with nothing marking it as stale. Measured symptom: a branch one + /// commit behind its upstream, with Pull disabled and no way to find + /// out but pressing Refresh. + #[tokio::test] + async fn moving_the_checkout_asks_the_remote_again() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let work = root.join("work"); + std::fs::create_dir_all(&work).expect("mkdir"); + let components = two_component_checkout(&work); + let origin = root.join("origin.git"); + run(root, &["git", "init", "-q", "--bare", "origin.git"]); + run( + &work, + &[ + "git", + "remote", + "add", + "origin", + &origin.display().to_string(), + ], + ); + run(&work, &["git", "push", "-q", "-u", "origin", "main"]); + // A branch whose upstream has one commit this checkout does not, + // which is exactly what Pull is for. + run(&work, &["git", "checkout", "-qb", "topic"]); + std::fs::write(work.join("app/main.kt"), "two").expect("write"); + run(&work, &["git", "commit", "-qam", "two"]); + run(&work, &["git", "push", "-q", "-u", "origin", "topic"]); + std::fs::write(work.join("app/main.kt"), "three").expect("write"); + run(&work, &["git", "commit", "-qam", "three"]); + run(&work, &["git", "push", "-q", "origin", "topic"]); + run(&work, &["git", "reset", "--hard", "-q", "HEAD~1"]); + run(&work, &["git", "checkout", "-q", "main"]); + + let state = state_for(&work, components); + // What a check on `main` leaves behind: nothing to pull, said as + // settled fact. + state.shared.remote_checks.mark_current(&work); + assert!(!state.shared.remote_checks.new_commits(&work)); + + state.move_checkout("topic".to_string()); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while !state.shared.remote_checks.new_commits(&work) { + assert!( + std::time::Instant::now() < deadline, + "the card still reports the branch that was left: no commits to pull, on a \ + branch that is one behind", + ); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert_eq!( + crate::git::status(&work).expect("a checkout").branch, + "topic", + ); + } + /// A component with nothing built is not "current with the checkout", /// however still the commits are. /// diff --git a/server/src/checks.rs b/server/src/checks.rs index c873802..afe5a54 100644 --- a/server/src/checks.rs +++ b/server/src/checks.rs @@ -173,6 +173,29 @@ impl Checks(&self, key: &Q) + where + K: Borrow, + Q: Hash + Eq + ?Sized, + { + if let Some(entry) = self.0.lock().unwrap().get_mut(key) { + entry.answer = None; + entry.error = None; + } + } + /// Forgets everything whose key `keep` rejects, for something being /// removed. pub fn retain(&self, keep: impl FnMut(&K) -> bool) { diff --git a/server/src/git.rs b/server/src/git.rs index 3fba9fc..685472a 100644 --- a/server/src/git.rs +++ b/server/src/git.rs @@ -861,6 +861,20 @@ impl RemoteChecks { .update(project.to_path_buf(), |new_commits| *new_commits = false); } + /// Drops what was learned about one checkout and asks again, for a + /// move that changed which branch the question is about. + /// + /// Both halves matter. Without the forgetting, the card answers for + /// the branch that was just left -- and says it with `checkPending` + /// false, so nothing on screen marks it as an answer from before the + /// move: a branch with commits waiting reads as one with none, and + /// Pull is disabled about it. Without the asking, the card would sit + /// on "still finding out" until somebody pressed Refresh. + pub fn recheck(&self, checkout: &Checkout) { + self.0.forget(&checkout.path); + self.refresh(std::slice::from_ref(checkout)); + } + /// Asks every checkout that isn't already being asked. Returns at once. pub fn refresh(&self, projects: &[Checkout]) { for checkout in projects { diff --git a/server/src/routes.rs b/server/src/routes.rs index 65edb34..072e934 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -1237,6 +1237,15 @@ struct PurgeQuery { struct PullQuery { #[serde(default)] force: bool, + /// Whether to build what the commits brought in. + /// + /// The phone's two buttons: Pull takes the commits and stops, Update + /// pulls and builds and then installs what that produced. Defaulted to + /// building rather than not, because an app older than this parameter + /// asks for the route that has always pulled *and* built -- omitting + /// it has to keep meaning what it used to mean, and it is the frozen + /// half of the contract that lets an old app fetch a new one. + build: Option, } /// Which build a download wants, and whose. @@ -1544,8 +1553,15 @@ async fn build_now( Ok(Json(build.status())) } -/// Fetches, fast-forwards, and builds -- reported through the same status -/// the phone already polls for a build, so one progress path covers both. +/// Fetches and fast-forwards, and builds unless asked not to -- reported +/// through the same status the phone already polls for a build, so one +/// progress path covers both. +/// +/// `?build=false` is the Pull button, which takes the commits and stops; +/// Update is this route with the build, followed by the phone installing +/// what it produced. Same route rather than two, because everything +/// except whether a build follows is identical -- the lock, the dirty +/// check, the unrelated-histories report and the status the phone polls. /// /// Pulling stays available while a project's build step is waiting to be /// accepted, and stops after the fast-forward: taking commits runs git, @@ -1562,6 +1578,10 @@ async fn build_pull( .build .as_ref() .ok_or_else(|| ApiError::NoBuildStep(entry.label.clone()))?; + if !query.build.unwrap_or(true) { + build.pull_only(query.force); + return Ok(Json(build.status())); + } // Asked after the fast-forward, not now: the declaration the gate // reads is the one the pull is about to rewrite, so the answer from // here is the answer for the commit being replaced.