diff --git a/.dev-updater.ron b/.dev-updater.ron index f56f03e..4ec486b 100644 --- a/.dev-updater.ron +++ b/.dev-updater.ron @@ -28,19 +28,50 @@ components: [ // started anywhere else this server finds no checkout of its own, // and its card silently loses the branch line, its commit count // and the Pull button. - build: "cargo build --release --manifest-path server/Cargo.toml", + // The ways this component can be built, declared in one place. + // Release first, because the first is the default: what this + // server serves to a phone should be the optimised build unless + // somebody says otherwise, and debug is here for the times when + // a stack trace matters more than the speed. + modes: ["release", "debug"], + // Written per mode rather than handed the mode as an argument, + // which is the escape hatch and this is what it is for: cargo + // takes `--release` or nothing, and its *profile* for the + // unoptimised build is called `dev` while the directory it + // writes is called `debug`. So no single word can serve as both + // the flag and the path, and a project that wants one would have + // to carry a wrapper script to translate. + build: { + "release": "cargo build --release --manifest-path server/Cargo.toml", + "debug": "cargo build --manifest-path server/Cargo.toml", + }, // Managed, like anything else that just wants its binary kept // running. Nothing about restarting *this* server lives in the // script -- that is `restart.rs`, which defers the hand-over past // the reply and spawns it detached. So there was nothing left for // a script of its own to say. - service: Managed("server/target/release/dev-updater"), + // And the same for the binary, which is the pairing that makes + // per-mode worth having at all: built with `--release` and + // started from `target/debug/`, this server would go on running + // last week's build with nothing anywhere saying so. + service: Managed({ + "release": "server/target/release/dev-updater", + "debug": "server/target/debug/dev-updater", + }), ), Apk( name: "app", // The command resolves against the project root and `cwd` says // where to run it -- two different things, which is why this is // not `./build-apk.sh`. + // + // Deliberately one mode. A release build of *this* app would be + // signed with a different key from the debug one, and Android + // refuses to install a differently-signed APK over an installed + // package -- which for this app means the copy on the phone can + // no longer be updated through the server it updates itself + // from. Every other project can switch freely; this is the one + // that cannot, because it is the way back. build: "app/build-apk.sh", cwd: "app", ), diff --git a/AGENTS.md b/AGENTS.md index 8aef175..958bb91 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,7 +36,9 @@ mutable at runtime from the phone. list). `discover.rs` is the scanner, `config.rs` the persisted schema and the RON both config files are in, `apkinfo.rs` the `aapt2` reads, - `strip.rs` the slim-APK pipeline, `sdk.rs` the SDK/NDK tool lookups. + `strip.rs` the slim-APK pipeline, `sdk.rs` the SDK/NDK tool lookups, + `script.rs` the one runner for a project's short declared commands + (`resources:`, `enroll:`) -- stdin closed, a deadline, stdout kept. - `app/` — Kotlin + Compose, a single `:androidApp` module. `UpdaterScreen.kt` is the list, `AddAppScreen.kt` the add/settings screen, `AppsApi.kt` the management calls, `UpdateManifest.kt` the read side, `ApkInstaller.kt` / @@ -618,6 +620,108 @@ mutable at runtime from the phone. the declaration build anyway. Pulling stays allowed while a request is unaccepted, since taking commits runs git rather than the project's command, and it is how the new request arrives to be read. +- **A component declares its build modes in one list, and that list is the + only place a mode is declared.** `modes: ["release", "debug"]` on the + component; the first is the default, which is why a project puts the one + it wants built by default first. Every command the component runs is + then handed the mode **as its last argument** -- so a project whose + script takes `release` or `debug` names that script once and nothing is + written twice. The escape hatch is writing a field *per mode* + (`build: {"release": ..., "debug": ...}`, `service: Managed({...})`), + and it exists because some commands cannot take the word: cargo takes + `--release` or nothing, and its profile for the unoptimised build is + called `dev` while the directory it writes is called `debug`, so no + single word serves as both the flag and the path. Gradle is the same + shape (`assembleRelease` needs capitalising). + **A command written per mode is not handed the mode as well** -- it + already is the answer, and appending the word would pass a stray + argument to a binary that never asked for one, which for a service is a + process that will not start. `ByMode::mode_argument` is the one place + that rule lives. + Having *one* list is what makes "this part has a mode the other part + never heard of" unsayable rather than something to detect: every + per-mode map is checked against it (`config::mode_problem`), so a + missing entry is named as missing instead of resolving to some other + mode's command. A map on a component that declares no modes is the same + mistake from the other side and is refused with a message saying what + to write. A declaration that fails the check is discarded exactly as an + unknown field is, and the card says so. + **`ByMode` holds a `Command` and nothing else, and that is load-bearing + rather than incidental.** Telling the single form from the map needs + `deserialize_any`, and RON reports a struct `(path: "a")` as a *map* -- + so a struct-valued field behind it would read its own field names as + mode names. Worse, measured: RON **discards the variant name** under + `deserialize_any`, so `Cargo("server")` and the array command + `["server"]` arrive as the same one-element sequence. That is why the + by-mode map lives *inside* `Service` rather than around it, and why a + `Cargo(...)`-style shorthand cannot be added to `build:` beside the + bare-string form without a format migration. + +- **Which mode to build in, and whether to strip, are the build + machine's choices; which finished build to install is the phone's.** + There is one checkout and one set of outputs, so a per-device mode + would have two enrolled phones rebuilding over each other with nothing + on either screen to say why -- the settings sheet says as much out + loud. They are stored on the component in `config.ron` (`mode`, + `stripHere`), set through `PUT /apps/{key}/components/{name}/settings`, + and excluded from `Component::same_declaration`: choosing one of the + declared modes is not the project asking for something new, and + including it would make every settings change re-open the acceptance + gate. Which means both halves of the carry-across have to be written -- + `registry::chosen_settings`/`restore_chosen_settings`, called by + `approve_declaration` **and** `reconcile_self`. Without the second, a + mode chosen for this server's own component would last until the next + restart, and restarting is how this server is updated. + `strip` stays declarable by the project and `stripHere` overrides it, + rather than being copied in at acceptance: copied, a later change to + the declaration would be ignored on every machine, silently. + **`BuildState::matches` compares the effective mode as well as the + declaration**, because that state holds a *snapshot* of the components + it builds from -- reused across a mode change it would go on running + the old mode's command from a card reporting the new one. + +- **A mode switch moves no commit, so `builtMode` is recorded beside + `builtFrom`.** Without it a component built in `debug` and switched to + `release` reads as current, offers nothing to press, and serves the + debug build for ever -- and for an `Apk` nothing else would notice, + because the "never built at all" check finds *any* variant under the + component's directory, so the debug APK sitting there is enough to + satisfy it. `Component::built_in_another_mode` answers it and both + `freshness` and `component_is_stale` ask, the first reporting `Behind` + *before* consulting the checkout at all: a clean tree at the very + commit the debug build was made from still does not make that build a + release one. It stays quiet until this server has built the component + once, so a project built by hand is not told it is out of date. + +- **Each component card has a settings sheet, and the variant picker + lives in it.** The gear sits at the row's right-hand end beside the log + button, drawn unconditionally so its presence is never the signal. The + sheet holds the build mode, which of the discovered builds this phone + installs, whether to strip, and Enrol; a section with nothing to offer + says so rather than vanishing, since "this project declares one way of + building" and "we could not tell" must not look alike. The variant + picker moved here from the foot of the card because on the card the two + read as one choice -- both say `debug` and `release` -- and they are + not: one decides what the build machine *builds*, the other which + finished build this phone takes. Everything applies on Save except + Enrol, which is an action and happens on the press. + **The row's text is inside one weighted child so it can never push the + controls off the edge**, and every reading in it truncates with an + ellipsis. A control that leaves because the text grew is one the reader + cannot get back to. + +- **`enroll:` is a command whose one line of stdout is a URL for the + phone to open after installing.** Deliberately "a URL to open" rather + than anything named after enrolment -- a route that knew what enrolling + was would be a special case of itself. Run **per press** + (`POST /apps/{key}/components/{name}/enroll-link`), never cached: the + link a project mints is ordinarily one-shot and carries a credential, + so a stored one would be both stale and a secret sitting in a file. In + the acceptance gate like `build`, because it runs on the build machine. + `crate::script::capture` is the shared runner it and `resources` both + use -- stdin closed, a deadline, stdout kept, and a failure naming the + command and the first line of its stderr. + - **Which build variant to serve is the phone's choice, not the server's.** It arrives as `?variant=` on the download, beside the `?component=` that says whose build it is, and is validated against *that component's* diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt index 299843a..beb6b06 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/AppsApi.kt @@ -203,6 +203,53 @@ data class ComponentLog( * Given the manifest's timeout rather than the default: a large log is a real request that takes * real time, and timing it out would report the server as unreachable when it is merely reading. */ +/** + * This machine's preferences for one component: which declared build mode to build it in, and + * whether to strip what it serves. + * + * On the build machine rather than on this device, unlike the chosen variant. A mode decides what + * gets *built*, and there is one checkout there -- two phones holding different answers would + * rebuild over each other with nothing on either screen to say why. Which of the finished builds + * this phone installs stays this phone's business, and stays in SharedPreferences. + * + * A null mode means "no choice, take the first the project declared", and a null strip means the + * same about the project's own declaration. Both are real answers rather than missing fields. + */ +fun setComponentSettings(key: String, component: String, mode: String?, strip: Boolean?) { + // Explicit JSON nulls rather than omitted keys, so "put this back to + // what the project says" is something this can express at all. + val body = + JSONObject().put("mode", mode ?: JSONObject.NULL).put("strip", strip ?: JSONObject.NULL) + requestFromServer( + "/apps/$key/components/$component/settings", + method = "PUT", + jsonBody = body.toString(), + ) {} +} + +/** + * Asks the build machine to run this component's `enroll:` command and hand back the URL it + * printed, for this phone to open. + * + * Run on every press rather than fetched once with the manifest: the link a project mints is + * ordinarily one-shot and carries a credential, so a cached one would be both stale and a secret + * sitting in a list this app redraws constantly. + */ +fun enrollmentLink(key: String, component: String): String = + requestFromServer( + "/apps/$key/components/$component/enroll-link", + method = "POST", + readTimeoutMs = ENROLL_TIMEOUT_MS, + ) { connection -> + JSONObject(connection.inputStream.bufferedReader().readText()).getString("url") + } + +// Longer than an ordinary call: the command runs on the build machine and +// may have to touch a keystore or write a file on the way. Timing it out +// early would report "couldn't reach the server" about a server that is +// working. +private const val ENROLL_TIMEOUT_MS = 25000 + fun componentLog( key: String, component: String, diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt index 6073403..e166b08 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdateManifest.kt @@ -143,6 +143,24 @@ data class ProjectComponent( // The last is the ordinary case and not a fault. val resourcesChecking: Boolean, val resourcesError: String?, + // Every way this component can be built, in the order the project + // declared them, with the first the default. Empty for a component + // that declares one way of building, which is not a choice -- the + // settings sheet says so rather than drawing a picker holding one + // entry that cannot be changed. + val modes: List, + // Which of them the build machine is set to build it in. Null exactly + // when [modes] is empty. Not a per-device preference like the chosen + // variant: there is one checkout and one set of build outputs on that + // machine, so this is what *everyone* gets, which is why the sheet + // says so out loud. + val mode: String?, + // The project declares a command that prints a link for this phone to + // open after installing -- an enrolment link, for the apps that need + // one. What the link does is the project's business; the button just + // opens it. False while a declaration is waiting to be accepted, when + // the command would not run. + val hasEnrollLink: Boolean, // What there is to install, for a component that produces one. Null // for a server, which builds nothing this phone installs. val apk: ComponentApk?, @@ -197,6 +215,14 @@ data class ComponentApk( // build freshness is the only meaningful signal, not a version code. val mtime: Double, val size: Long, + // Whether the build machine serves a stripped copy: what it settled + // on, which is what a download will actually do. + val strip: Boolean, + // What the *project* asks for, which differs only when somebody + // overrode it on that machine. Both are sent so the sheet can say the + // two disagree, rather than showing a switch that silently contradicts + // the checkout. + val stripDeclared: Boolean, // Every build discovered under this component, so a different one can // be selected without another round trip. val variants: List, @@ -370,6 +396,12 @@ private fun readEntry(entry: JSONObject): ManifestEntry { configPresent = component.optBoolean("configPresent", false), resourcesChecking = component.optBoolean("resourcesChecking", false), resourcesError = component.optString("resourcesError").ifEmpty { null }, + modes = + component.optJSONArray("modes").let { modes -> + (0 until (modes?.length() ?: 0)).map { modes!!.getString(it) } + }, + mode = component.optString("mode").ifEmpty { null }, + hasEnrollLink = component.optBoolean("hasEnrollLink", false), apk = component.optJSONObject("apk")?.let(::readApk), ) }, @@ -386,6 +418,8 @@ private fun readApk(apk: JSONObject): ComponentApk { built = apk.getBoolean("built"), mtime = apk.getDouble("mtime"), size = apk.getLong("size"), + strip = apk.optBoolean("strip", false), + stripDeclared = apk.optBoolean("stripDeclared", false), variants = (0 until variants.length()).map { j -> val variant = variants.getJSONObject(j) 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 96855b5..8c5d93d 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -1,5 +1,7 @@ package com.example.devupdater +import android.content.ActivityNotFoundException +import android.content.Intent import android.content.pm.PackageManager import android.net.Uri import android.os.SystemClock @@ -219,6 +221,16 @@ private sealed class ComponentState { /** [progress] is null when the response gave no length to measure against. */ data class Downloading(val progress: Float?) : ComponentState() + /** + * A short call to the build machine that is neither a build nor a download -- fetching an + * enrolment link is the one so far. + * + * Carries its own words because the card has no other way to say which call it is waiting on, + * and "working" over a bar that could be any of three things is what makes a screen feel like + * it is doing something at random. + */ + data class Busy(val what: String) : ComponentState() + /** Why the last thing this component was asked to do stopped. */ data class Error(val message: String) : ComponentState() } @@ -1013,6 +1025,61 @@ private fun AppListScreen( } } + /** + * The same as [manage], for a call that is about one component rather than the whole project. + * + * A failure belongs where the press happened: reported on the card, one component's settings + * write would blame the project and go quiet on the row somebody was actually looking at. + */ + fun manageComponent(entry: ManifestEntry, component: String, action: () -> Unit) { + scope.launch { + try { + withContext(Dispatchers.IO) { action() } + setComponent(entry.key, component, null) + applyOne(entry.key) + } catch (e: DownloadServerException) { + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) + } + } + } + + /** + * Asks the build machine for this component's link and opens it here. + * + * Fetched on every press rather than carried on the manifest: what a project mints is + * ordinarily one-shot and carries a credential, so a link this app had been holding since the + * last refresh would be the wrong one. + * + * Nothing here inspects the URL. Which app answers it is Android's business, and a link the + * phone has nothing to open is reported as such rather than swallowed -- an action whose whole + * effect is elsewhere has to say when it did not happen. + */ + fun openEnrollmentLink(entry: ManifestEntry, component: String) { + scope.launch { + setComponent(entry.key, component, ComponentState.Busy("Getting the link")) + try { + val url = withContext(Dispatchers.IO) { enrollmentLink(entry.key, component) } + try { + context.startActivity( + Intent(Intent.ACTION_VIEW, Uri.parse(url)) + .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) + ) + setComponent(entry.key, component, null) + } catch (e: ActivityNotFoundException) { + setComponent( + entry.key, + component, + ComponentState.Error( + "Nothing on this phone opens $url" + (e.message?.let { " ($it)" } ?: "") + ), + ) + } + } catch (e: DownloadServerException) { + setComponent(entry.key, component, failure(e)?.let(ComponentState::Error)) + } + } + } + /** * Runs one of a server component's actions on the build machine. * @@ -1356,6 +1423,24 @@ private fun AppListScreen( (component to variant.path) }) }, + // The build machine's, not this + // device's: a mode decides what + // gets built there, so it is a + // round trip and a refetch rather + // than a preference written here. + onComponentSettings = { component, mode, strip -> + manageComponent(entry, component) { + setComponentSettings( + entry.key, + component, + mode, + strip, + ) + } + }, + onEnroll = { component -> + openEnrollmentLink(entry, component) + }, serviceBusy = serviceBusy[entry.key], onServiceAction = { component, action, purge -> runServiceAction(entry, component, action, purge) @@ -1471,6 +1556,10 @@ private fun AppCard( onApprove: () -> Unit, onRemove: () -> Unit, onSelectVariant: (component: String, ApkVariant?) -> Unit, + /** This machine's settings for one component: which mode it builds in, and whether to strip. */ + onComponentSettings: (component: String, mode: String?, strip: Boolean?) -> Unit, + /** Fetch this component's link from the build machine and open it here. */ + onEnroll: (component: String) -> Unit, // Which component this card is running a service action for, if any -- // so the one being acted on is the one that shows it, rather than // every row going quiet together. @@ -1692,6 +1781,10 @@ private fun AppCard( }, chosenVariantPath = chosenVariantPath, onSelectVariant = { onSelectVariant(component.name, it) }, + onSettings = { mode, strip -> + onComponentSettings(component.name, mode, strip) + }, + onEnroll = { onEnroll(component.name) }, // This app reaches the server through this server. // Stopping or uninstalling it is the one action // here that cannot be undone from the phone. @@ -2020,6 +2113,12 @@ private fun ApkProgress(state: ComponentState?) { Text("Preparing the download...") } + is ComponentState.Busy -> { + ProgressBar() + Spacer(Modifier.height(4.dp)) + Text("${state.what}...") + } + is ComponentState.Downloading -> { val progress = state.progress if (progress == null) { @@ -2356,6 +2455,10 @@ private fun ComponentCard( /** Which of this component's builds this device is pinned to, if any. */ chosenVariantPath: String? = null, onSelectVariant: (ApkVariant?) -> Unit = {}, + /** Save this machine's settings for this component. */ + onSettings: (mode: String?, strip: Boolean?) -> Unit = { _, _ -> }, + /** Ask the build machine for this component's link and open it. */ + onEnroll: () -> Unit = {}, isOwnServer: Boolean, /** This component's part of a build in progress, if it has one. */ build: ComponentBuild?, @@ -2387,6 +2490,7 @@ private fun ComponentCard( // a second thing to remember to clear. var confirming by remember { mutableStateOf(null) } var showingLog by remember { mutableStateOf(false) } + var showingSettings by remember { mutableStateOf(false) } // What Uninstall has been asked to take away as well. Logs start // ticked and the other two do not: the dialog's defaults, deliberately // different from `Purge()`'s, which is what a caller with no dialog @@ -2407,116 +2511,149 @@ private fun ComponentCard( // same size as a line of this text anyway, so the row comes out // the same height without being told. Row(verticalAlignment = Alignment.CenterVertically) { - // Both kinds get the same square, so a server's glyph and - // an app's icon are the same size as each other -- one - // drawn smaller than the other reads as the row meaning - // less, rather than as a different kind of thing. - Box( - Modifier.size(COMPONENT_ICON_SIZE), - contentAlignment = Alignment.Center, + // Everything the row *says* lives inside one weighted + // child, so it can only ever have the space the controls + // on the right are not using. Without it a long name or a + // long status pushed the log and settings buttons off the + // edge -- and a control that leaves because the text grew + // is one the reader cannot get back to. + Row( + Modifier.weight(1f), + verticalAlignment = Alignment.CenterVertically, ) { - if (component.isServer) { - Text( - SERVER_GLYPH, - fontFamily = NerdIcons, - // Larger than the square in font terms: a glyph - // is drawn well inside its line box, so matching - // the numbers would draw it noticeably smaller - // than the icon beside it. - fontSize = COMPONENT_GLYPH_SIZE, - color = MaterialTheme.colorScheme.onSurfaceVariant, - // Which means its line box is taller than the - // square, and text clips to the height it is - // given. Measured unbounded and drawn centred - // instead: the row keeps the icon's height and - // the glyph keeps all of itself. - modifier = Modifier.wrapContentSize(unbounded = true), - ) - } else { - // The app's own icon, the same one the project card - // shows: this row is about the thing that gets - // installed, and that is what it looks like. - AppIcon( - packageName, - size = COMPONENT_ICON_SIZE, - glyphSize = COMPONENT_GLYPH_SIZE, - ) + // Both kinds get the same square, so a server's glyph and + // an app's icon are the same size as each other -- one + // drawn smaller than the other reads as the row meaning + // less, rather than as a different kind of thing. + Box( + Modifier.size(COMPONENT_ICON_SIZE), + contentAlignment = Alignment.Center, + ) { + if (component.isServer) { + Text( + SERVER_GLYPH, + fontFamily = NerdIcons, + // Larger than the square in font terms: a glyph + // is drawn well inside its line box, so matching + // the numbers would draw it noticeably smaller + // than the icon beside it. + fontSize = COMPONENT_GLYPH_SIZE, + color = MaterialTheme.colorScheme.onSurfaceVariant, + // Which means its line box is taller than the + // square, and text clips to the height it is + // given. Measured unbounded and drawn centred + // instead: the row keeps the icon's height and + // the glyph keeps all of itself. + modifier = Modifier.wrapContentSize(unbounded = true), + ) + } else { + // The app's own icon, the same one the project card + // shows: this row is about the thing that gets + // installed, and that is what it looks like. + AppIcon( + packageName, + size = COMPONENT_ICON_SIZE, + glyphSize = COMPONENT_GLYPH_SIZE, + ) + } } - } - Spacer(Modifier.width(6.dp)) - Text( - component.name, - style = MaterialTheme.typography.bodyMedium, - color = MaterialTheme.colorScheme.onSurfaceVariant, - ) - // Nothing at all until its script has been asked -- an - // unknown state is not a state, and a dot introducing - // nothing is worse than no dot. - // Said in words, not by colour alone: "behind the - // checkout" is a difference in kind from "running", and a - // reader has no way to learn a colour that means it. - val behind = component.isBehind - val status = - when { - !component.isServer -> null - component.error != null -> "couldn't check" - component.state == null -> null - component.isRunning -> "running" - // Not folded into "stopped": stopped is a state - // somebody chose, and calling a crash that sends - // the reader looking for who chose it. - component.isFailed -> "failed" - component.isInstalled -> "stopped" - // Nothing for a service that isn't installed: the row - // offers Install and nothing else, which says it more - // plainly than a state would, and saying both makes the - // absence of a thing look like a condition it is in. - else -> null - } - status?.let { - Separator() + Spacer(Modifier.width(6.dp)) Text( - it, - style = MaterialTheme.typography.bodyMedium, - color = - when { - component.isRunning -> runningColor - component.isFailed -> failedColor - else -> MaterialTheme.colorScheme.onSurfaceVariant - }, - ) - } - if (behind) { - Separator() - Text( - "out of date", - style = MaterialTheme.typography.bodyMedium, - // The colour Pull & Build wears, because that is - // the button this is telling you to press. - color = ActionTone.Primary.color, - ) - } - sizeText?.let { - Separator() - Text( - it, + component.name, style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, + // One line and an ellipsis, on this and on every + // reading beside it: a row that wraps grows the + // card, and one that overflows silently loses its + // right-hand end without saying it was cut. + maxLines = 1, + overflow = TextOverflow.Ellipsis, ) + // Nothing at all until its script has been asked -- an + // unknown state is not a state, and a dot introducing + // nothing is worse than no dot. + // Said in words, not by colour alone: "behind the + // checkout" is a difference in kind from "running", and a + // reader has no way to learn a colour that means it. + val behind = component.isBehind + val status = + when { + !component.isServer -> null + component.error != null -> "couldn't check" + component.state == null -> null + component.isRunning -> "running" + // Not folded into "stopped": stopped is a state + // somebody chose, and calling a crash that sends + // the reader looking for who chose it. + component.isFailed -> "failed" + component.isInstalled -> "stopped" + // Nothing for a service that isn't installed: the row + // offers Install and nothing else, which says it more + // plainly than a state would, and saying both makes the + // absence of a thing look like a condition it is in. + else -> null + } + status?.let { + Separator() + Text( + it, + style = MaterialTheme.typography.bodyMedium, + color = + when { + component.isRunning -> runningColor + component.isFailed -> failedColor + else -> MaterialTheme.colorScheme.onSurfaceVariant + }, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + } + if (behind) { + Separator() + Text( + "out of date", + style = MaterialTheme.typography.bodyMedium, + // The colour Pull & Build wears, because that is + // the button this is telling you to press. + color = ActionTone.Primary.color, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + } + sizeText?.let { + Separator() + Text( + it, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + } } + // Outside the weighted text, so the spinner is never the + // thing that gets truncated: it is the one mark on the row + // that says something is happening right now. if (working || busy || component.checking) { Spacer(Modifier.width(6.dp)) Working() } + // These belong to the component, not to whatever the row + // happens to say about it, so they hold the same corner + // whatever the text does. Unconditional, so the settings + // button sits in the same place whether or not there is a + // log beside it -- the log's own absence must not move it. if (component.hasLogs) { - // Pushed to the far edge rather than following the - // text: it belongs to the component, not to whatever - // the row happens to say about it, and a control that - // slides about as the state changes is harder to find - // than one always in the same corner. - Spacer(Modifier.weight(1f)) IconGlyphButton(LOG_GLYPH, "Show ${component.name}'s log") { showingLog = true } } + // Always drawn, including for a component with a single + // build mode and nothing to strip: what a component can be + // told is part of what it is, and a control that comes and + // goes makes its own presence the signal. What it opens + // says which of its settings this component has. + IconGlyphButton(SETTINGS_GLYPH, "${component.name} settings") { + showingSettings = true + } } // The service script's own words about why it could not answer, @@ -2663,27 +2800,35 @@ private fun ComponentCard( } } - // Only worth a row when there is actually a choice, which is - // rare -- the usual case is a single debug build, and an empty - // row here was leaving a band of space at the foot of every - // card for a control almost none of them have. - // - // Beside the build it picks, which is what makes it answerable - // for a project with two clients: the choice is this - // component's, and a picker at the foot of the card could only - // have been the project's. - val variants = component.apk?.variants.orEmpty() - if (variants.size > 1) { - Row( - horizontalArrangement = Arrangement.End, - modifier = Modifier.fillMaxWidth(), - ) { - VariantPicker(variants, chosenVariantPath, onSelectVariant) - } - } + // The variant picker used to sit here. It moved into the + // settings sheet, where the build mode is: on the card the two + // read as the same choice -- both say "debug" and "release" -- + // and they are not. One decides what the build machine + // *builds*; the other decides which of the finished builds + // this phone installs. } } + if (showingSettings) { + ComponentSettingsDialog( + component = component, + chosenVariantPath = chosenVariantPath, + // A mode changes what the build machine runs, and the server + // replaces the state a running build reports through. So the + // picker goes quiet exactly while this component cannot be + // acted on anyway -- the same pair of conditions that disables + // its Update button and withholds its freshness. + modeSettled = !(busy || working || projectState.busy), + onSelectVariant = onSelectVariant, + onEnroll = onEnroll, + onApply = { mode, strip -> + showingSettings = false + onSettings(mode, strip) + }, + onDismiss = { showingSettings = false }, + ) + } + if (showingLog) { ComponentLogDialog( entryKey = entryKey, @@ -2764,6 +2909,194 @@ private fun ComponentCard( } } +/** + * Everything one component can be told, in one sheet: how the build machine builds it, which of the + * finished builds this phone installs, whether it is stripped on the way, and the link a project + * hands over after an install. + * + * The two "debug or release" choices in here are deliberately separated and labelled, because they + * are not the same question and they used to sit on the card looking as though they were. **Build + * mode** is the build machine's: there is one checkout and one set of outputs there, so choosing it + * changes what *every* enrolled phone is offered, which is why the sheet says so rather than + * leaving it to be discovered. **Install** is this device's alone, and picks among builds that + * already exist. + * + * Applied on Save rather than as each control moves, matching the project's own settings sheet: a + * settings write is a round trip that rebuilds the entry on the server, and one per control touched + * while making up your mind is a lot of them. Enrol is the exception and is not a setting -- it is + * an action, it happens on the press, and it is drawn apart from the rest for that reason. + * + * A component with nothing to choose still gets its sections, saying so. Removing them would make + * the sheet's shape the signal, and "this project declares one way of building" and "we could not + * tell" would then look identical -- which is the whole reason the empty cases are written out here + * rather than skipped. + */ +@Composable +private fun ComponentSettingsDialog( + component: ProjectComponent, + chosenVariantPath: String?, + /** False while a build is in flight, when the mode must not be changed underneath it. */ + modeSettled: Boolean, + onSelectVariant: (ApkVariant?) -> Unit, + onEnroll: () -> Unit, + onApply: (mode: String?, strip: Boolean?) -> Unit, + onDismiss: () -> Unit, +) { + // Keyed on what the server last said, so reopening after a save shows + // the saved values rather than stale local ones. + var mode by remember(component.mode) { mutableStateOf(component.mode) } + var strip by remember(component.apk?.strip) { mutableStateOf(component.apk?.strip ?: false) } + // The variant is this device's, so it is applied locally on Save + // rather than sent anywhere; held here so Cancel really cancels. + var variantPath by remember(chosenVariantPath) { mutableStateOf(chosenVariantPath) } + + AlertDialog( + onDismissRequest = onDismiss, + title = { Text(component.name) }, + text = { + Column(Modifier.verticalScroll(rememberScrollState())) { + SettingsHeading("Build mode") + when { + component.modes.isEmpty() -> + SettingsNote( + "This project declares one way of building ${component.name}, so there " + + "is nothing to choose." + ) + else -> { + SettingsNote( + "Chosen on the build machine, so it is what every phone here is " + + "offered -- not just this one." + ) + // A dropdown rather than a row of radios, matching + // the build picker below it: they are the same + // shape of question -- one of a short list -- and + // two different controls for that in one sheet + // makes them look like different kinds of choice. + ModePicker(component.modes, mode, modeSettled) { mode = it } + if (!modeSettled) { + SettingsNote( + "Not while this component is busy: changing it now would leave " + + "the build that is running building the other one." + ) + } + } + } + + // Only an APK has anything below this. A server is not + // installed here, so there is no build for this phone to + // pick and nothing to strip on the way to it. + component.apk?.let { apk -> + Spacer(Modifier.height(16.dp)) + SettingsHeading("Install") + when { + apk.variants.isEmpty() -> + SettingsNote("Nothing is built yet, so there is nothing to install.") + apk.variants.size == 1 -> + SettingsNote( + "One build so far (${apk.variants.first().variant}), so there is " + + "nothing to choose between." + ) + else -> { + SettingsNote( + "Which of the builds on the machine this phone installs. Only " + + "this phone's." + ) + VariantPicker(apk.variants, variantPath) { variantPath = it?.path } + } + } + + Spacer(Modifier.height(16.dp)) + SettingsHeading("Transfer") + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.fillMaxWidth(), + ) { + Text( + "Strip debug symbols", + style = MaterialTheme.typography.bodyLarge, + modifier = Modifier.weight(1f), + ) + Spacer(Modifier.width(12.dp)) + Switch(checked = strip, onCheckedChange = { strip = it }) + } + // Said only when the two disagree. A machine that + // follows the checkout has nothing to report, and a + // line under every switch saying "as declared" is one + // more thing to read on every card. + if (strip != apk.stripDeclared) { + SettingsNote( + if (apk.stripDeclared) { + "The project asks for stripping; this machine is set not to." + } else { + "The project does not ask for stripping; this machine is set to." + } + ) + } + } + + if (component.hasEnrollLink) { + Spacer(Modifier.height(16.dp)) + SettingsHeading("Enrolment") + SettingsNote( + "Asks the build machine for a fresh link and opens it here. Do this after " + + "installing, or after reinstalling over a different signing key -- " + + "either loses whatever enrolment the app had." + ) + // Outside the Save/Cancel bargain the rest of the + // sheet makes, because it is not a setting: pressing + // it does the thing, now, and there is nothing about + // it left to save. + TextButton( + onClick = { + onDismiss() + onEnroll() + } + ) { + Text("Enrol this app") + } + } + } + }, + confirmButton = { + TextButton( + onClick = { + // The variant is this device's, so it is written here + // and not sent; the other two are the build machine's. + if (variantPath != chosenVariantPath) { + onSelectVariant( + component.apk?.variants?.firstOrNull { it.path == variantPath } + ) + } + onApply(mode, strip) + } + ) { + Text("Save") + } + }, + dismissButton = { TextButton(onClick = onDismiss) { Text("Cancel") } }, + ) +} + +@Composable +private fun SettingsHeading(text: String) { + Text(text, style = MaterialTheme.typography.titleSmall) +} + +/** + * A line of explanation under a control, or in place of one that has nothing to offer. + * + * Its own composable so that "there is nothing to choose here" looks the same wherever it is said + * -- four sections each phrasing their empty case differently is how a reader learns to skip them. + */ +@Composable +private fun SettingsNote(text: String) { + Text( + text, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) +} + /** * The three things Uninstall can take away besides the service. * @@ -2903,6 +3236,35 @@ private fun PurgeToggle( } } +@Composable +private fun ModePicker( + modes: List, + chosen: String?, + enabled: Boolean, + onSelect: (String) -> Unit, +) { + var expanded by remember { mutableStateOf(false) } + Box { + TextButton(onClick = { expanded = true }, enabled = enabled) { + // Never "none": unlike the build picker there is always an + // answer here, because a component with modes is always being + // built in one of them -- the first, when nobody has chosen. + Text(chosen ?: modes.firstOrNull().orEmpty()) + } + DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { + modes.forEach { candidate -> + DropdownMenuItem( + text = { Text(if (candidate == chosen) "$candidate ✓" else candidate) }, + onClick = { + expanded = false + onSelect(candidate) + }, + ) + } + } + } +} + @Composable private fun VariantPicker( variants: List, diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 3f0f35f..1ffb7ab 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -416,8 +416,18 @@ impl BuildState { self.git_pull == git_pull && self.git_ipv4 == git_ipv4 && self.components.len() == components.len() - && std::iter::zip(&self.components, components) - .all(|(mine, theirs)| mine.same_declaration(theirs)) + && std::iter::zip(&self.components, components).all(|(mine, theirs)| { + // The declaration, plus the one choice that changes what + // this state would actually run. `same_declaration` + // deliberately ignores the chosen mode -- picking one is + // not the project asking for something new -- but this + // state holds a *snapshot* of the components it builds + // from, so a mode changed underneath it would go on + // running the old mode's command from a card reporting + // the new one. Strip is not here: it is decided at + // download, and nothing this state does depends on it. + mine.same_declaration(theirs) && mine.effective_mode() == theirs.effective_mode() + }) } /// Idempotent: kicks off the build in the background if this app is @@ -546,6 +556,15 @@ impl BuildState { /// uncommitted work and sending it to a phone would also be a /// surprising thing to do with work its author has not committed. pub fn freshness(&self, component: &Component) -> Freshness { + // Behind for a reason no commit can express: what is on disk was + // built some other way than this component is set to build now. + // Reported before the checkout is consulted at all, because it is + // a fact rather than a comparison -- a clean tree at the very + // commit the debug build was made from still does not make that + // build a release one. + if component.built_in_another_mode() { + return Freshness::Behind; + } let built = self .inner .lock() @@ -575,6 +594,16 @@ impl BuildState { if component.build().is_empty() { return false; } + // The same fact `freshness` reports as Behind, and it has to be + // asked here too: for an `Apk`, the "never built at all" check + // below finds *any* variant under the component's directory, so a + // debug build sitting there is enough to make a component + // switched to release look built. Nothing else would notice -- + // the commit has not moved -- so Update would do nothing and the + // phone would install the debug APK from a card saying release. + if component.built_in_another_mode() { + return true; + } // Nothing built at all is as far behind as an output gets, and it // is checked before any rule rather than after: a `staleWhen` // compares two files *inside* a build, so a component that has @@ -960,11 +989,18 @@ impl BuildState { // wants to read afterwards. let log = crate::logs::open_build_log(&self.key, component.name()) .map(|file| Arc::new(Mutex::new(file))); + // The mode, for a build command written once -- which is how a + // project with a script that takes `release` or `debug` says it, + // and why nothing has to be written twice in that case. Nothing + // for a command written per mode: that command already *is* the + // answer, and handing the word over as well would pass a stray + // argument to a build that never asked for one. + let mode = component.build_mode_argument(); self.run_streaming_command( component.name(), component.build(), component.cwd(), - &[], + mode.as_slice(), log, ) } @@ -1323,22 +1359,30 @@ mod tests { vec![ Component::Server { name: "backend".to_string(), - build: crate::config::Command::from_line("true"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line("true")), cwd: Some(PathBuf::from("server")), stale_when: None, also_watch: Vec::new(), service: None, + mode: None, built_from: None, + built_mode: None, }, Component::Apk { name: "app".to_string(), - build: crate::config::Command::from_line("true"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line("true")), cwd: Some(PathBuf::from("app")), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }, ] } @@ -1389,13 +1433,20 @@ mod tests { // pressed, and only the pull closes it. let accepted = vec![Component::Apk { name: "app".to_string(), - build: crate::config::Command::from_line("touch built-marker"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch built-marker", + )), cwd: None, stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, package: None, + mode: None, built_from: None, + built_mode: None, }]; let state = state_for(&clone, accepted.clone()); @@ -1447,22 +1498,34 @@ mod tests { let components = vec![ Component::Server { name: "backend".to_string(), - build: crate::config::Command::from_line("touch backend-built"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch backend-built", + )), cwd: Some(PathBuf::from("server")), stale_when: None, also_watch: Vec::new(), service: None, + mode: None, built_from: None, + built_mode: None, }, Component::Apk { name: "app".to_string(), - build: crate::config::Command::from_line("touch app-built"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch app-built", + )), cwd: Some(PathBuf::from("app")), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }, ]; let state = state_for(root, components); @@ -1527,16 +1590,21 @@ mod tests { let components = ["slow", "quick"] .map(|name| Component::Apk { name: name.to_string(), - build: crate::config::Command::from_line(match name { + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line(match name { "slow" => "./slow.sh", _ => "touch quick-built", - }), + })), cwd: Some(PathBuf::from(name)), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }) .to_vec(); let state = state_for(root, components); @@ -1598,16 +1666,21 @@ mod tests { let components = ["broken", "fine"] .map(|name| Component::Apk { name: name.to_string(), - build: crate::config::Command::from_line(match name { + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line(match name { "broken" => "false", _ => "true", - }), + })), cwd: Some(PathBuf::from(name)), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }) .to_vec(); let state = state_for(root, components); @@ -1671,23 +1744,37 @@ mod tests { let components = vec![ Component::Apk { name: "a".to_string(), - build: crate::config::Command::from_line("touch a-built"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch a-built", + )), cwd: Some(PathBuf::from("a")), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }, Component::Apk { name: "b".to_string(), - build: crate::config::Command::from_line("touch b-built"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line( + "touch b-built", + )), cwd: Some(PathBuf::from("b")), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }, ]; let state = state_for(root, components); @@ -1742,6 +1829,65 @@ mod tests { state.component_is_stale(component, &state.inner.lock().unwrap().built_from.clone()) } + /// The trap a per-component staleness check walks straight into once + /// modes exist. An APK's "never built at all" test finds *any* build + /// under the component's directory, so a debug APK sitting there is + /// enough to make a component switched to release look built -- and + /// switching modes moves no commit, so nothing else has an opinion. + /// Update would then do nothing and the phone would install the debug + /// build from a card saying release. + /// + /// Asserted through `component_is_stale` rather than through + /// `built_in_another_mode` alone, because the rule being right is not + /// the same as the check that decides whether to build asking it. + #[test] + fn a_component_switched_to_another_mode_is_stale_even_with_a_build_on_disk() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + // A build output where an APK actually lands, so the "nothing + // built here" branch is satisfied and cannot be what answers. + let built = root.join("build/outputs/apk/debug"); + std::fs::create_dir_all(&built).expect("mkdir"); + std::fs::write(built.join("app-debug.apk"), b"not really an apk").expect("write"); + + let moded = |mode: Option<&str>| { + vec![Component::Apk { + name: "app".to_string(), + modes: vec!["release".to_string(), "debug".to_string()], + build: crate::config::ByMode::One(crate::config::Command::from_line("true")), + cwd: None, + stale_when: None, + also_watch: Vec::new(), + strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + package: None, + mode: mode.map(str::to_string), + // Built here once, in debug -- without which there is no + // build of ours for the comparison to be about. + built_from: Some("abc123".to_string()), + built_mode: Some("debug".to_string()), + }] + }; + + let matching = state_for(root, moded(Some("debug"))); + assert!( + !stale(&matching, "app"), + "the build on disk is this mode's, so there is nothing to do", + ); + + let switched = state_for(root, moded(Some("release"))); + assert!( + stale(&switched, "app"), + "a debug build must not satisfy a component set to build release", + ); + assert_eq!( + switched.freshness(&switched.components[0]), + Freshness::Behind, + "and the card has to say so rather than reading as current", + ); + } + /// The shape of bug a unit test that only calls `component_is_stale` /// cannot see: `status()` and `trigger_if_needed` hold the lock that /// the staleness check also wants. Taking it twice on one thread is a @@ -1893,13 +2039,18 @@ mod tests { root, vec![Component::Apk { name: "app".to_string(), - build: crate::config::Command::from_line("true"), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line("true")), cwd: None, stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, package: None, + mode: None, built_from: Some("0000000000000000000000000000000000000000".to_string()), + built_mode: None, }], ); assert!(!stale(&state, "app"), "no checkout means no opinion"); diff --git a/server/src/config.rs b/server/src/config.rs index 50a9b79..42adbf6 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -124,6 +124,15 @@ pub struct ProjectConfig { /// "neither set" are states the file can express and nothing means, which /// then need a check to reject rather than being unsayable. /// +/// The command inside either variant is a [`ByMode`], because which +/// binary a service runs is exactly what a build mode changes: a server +/// built with `--release` and started from `target/debug/` is the one +/// mistake this whole feature would otherwise introduce, and it would +/// look like a build that silently did nothing. The map goes *inside* the +/// variant rather than around it -- `service: {"debug": Managed(...)}` +/// cannot be told from a single value, since RON reports both a map and a +/// struct the same way; see [`ByMode`]. +/// /// Whichever it is, what comes out the other side is the same thing -- a /// command run as ` `, answering the contract in /// `crate::service`. `Managed` is not a second mechanism; it is the @@ -136,12 +145,30 @@ pub enum Service { /// something the built-in one does not -- dev-updater's own service is /// the example, since restarting it means restarting the process doing /// the asking. - Script(Command), + Script(ByMode), /// dev-updater supervises this command with its built-in script, so a /// project that just wants its binary kept running does not carry 150 /// lines of init-system detection to keep in step with everyone /// else's copy. - Managed(Command), + Managed(ByMode), +} + +impl Service { + /// The command as written, before a mode is resolved. Public for the + /// declaration check, which asks about the writing rather than the + /// result. + pub fn by_mode(&self) -> &ByMode { + match self { + Self::Script(command) | Self::Managed(command) => command, + } + } + + /// The command for `mode`, keeping the variant -- which decides + /// whether the built-in script wraps it, and is not something a mode + /// changes. + pub fn command(&self, mode: Option<&str>) -> &Command { + self.by_mode().get(mode) + } } /// Where a project's own resources are declared. @@ -231,8 +258,13 @@ pub enum Component { /// An APK to install on the phone. Apk { name: String, - #[serde(default, skip_serializing_if = "Command::is_empty")] - build: Command, + /// The ways this component can be built, in order, the first the + /// default. See [`ByMode`] for what a mode is and how a command + /// learns which one it is being run in. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + modes: Vec, + #[serde(default, skip_serializing_if = "ByMode::is_empty")] + build: ByMode, #[serde(default, skip_serializing_if = "Option::is_none")] cwd: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -249,6 +281,36 @@ pub enum Component { /// a project built without dev-updater has no reason to strip. #[serde(default, skip_serializing_if = "std::ops::Not::not")] strip: bool, + /// A command whose single line of stdout is a URL for the phone + /// to open once this app is installed. + /// + /// What that URL *means* is the project's business and none of + /// this server's: ai-app mints a one-shot enrolment link with it, + /// and anything else that needs the phone to be handed a link + /// after an install fits the same shape. Which is why it is a URL + /// rather than anything named after enrolment -- a route that + /// knew what enrolling was would be a special case of itself. + /// + /// Run **per press**, never cached: a link that carries a + /// credential is minted fresh each time, and one captured at + /// build time would be a stale secret sitting in a config file. + /// + /// Part of the acceptance gate, because it runs on the build + /// machine exactly as `build` does. + #[serde(default, skip_serializing_if = "Command::is_empty")] + enroll: Command, + /// This machine's answer instead, when somebody gave one from the + /// phone. `None` -- the ordinary case -- follows whatever the + /// project declares above, so a project that changes its mind + /// still takes effect on every machine that never overrode it. + /// + /// An override rather than copying the declared value in at + /// acceptance time: copied, a later change to the declaration + /// would be ignored on every machine silently, which is the + /// divergence this file has already been bitten by. Chosen, not + /// measured -- see [`Self::same_declaration`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + strip_here: Option, /// The installed package this replaces, read out of the APK by /// `crate::apkinfo` and cached here so `aapt2` never runs on the /// manifest path. Measured, not declared: absent until there has @@ -269,14 +331,42 @@ pub enum Component { /// has built it once, which reads as *unknown* rather than as /// stale -- a project built by hand before this existed must not /// announce that it needs rebuilding. + /// Which of the modes `build` declares this machine builds in. + /// + /// Chosen here rather than declared, and chosen here rather than + /// on the phone: there is one checkout and one set of build + /// outputs, so two enrolled devices holding different answers + /// would rebuild over each other with nothing on either screen to + /// say why. `None` means the first declared mode, which is why a + /// project puts the one it wants by default first. + /// + /// May name a mode that no longer exists -- a pull rewrites the + /// declaration and nothing rewrites this -- which [`ByMode::get`] + /// resolves by falling back rather than failing. + #[serde(default, skip_serializing_if = "Option::is_none")] + mode: Option, + /// The mode the recorded build was made in, beside the commit it + /// was made from. + /// + /// Measured, like `built_from`, and needed for the same reason it + /// is: switching modes moves no commit, so without this a + /// component built in `debug` and switched to `release` reads as + /// current, offers nothing to press, and serves the debug build + /// for ever. The card would look right the whole time, which is + /// the expensive kind of wrong. + #[serde(default, skip_serializing_if = "Option::is_none")] + built_mode: Option, #[serde(default, skip_serializing_if = "Option::is_none")] built_from: Option, }, /// A server on the build machine, delivered by restarting it. Server { name: String, - #[serde(default, skip_serializing_if = "Command::is_empty")] - build: Command, + /// The ways this component can be built. See [`ByMode`]. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + modes: Vec, + #[serde(default, skip_serializing_if = "ByMode::is_empty")] + build: ByMode, #[serde(default, skip_serializing_if = "Option::is_none")] cwd: Option, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -296,11 +386,44 @@ pub enum Component { /// has built it once, which reads as *unknown* rather than as /// stale -- a project built by hand before this existed must not /// announce that it needs rebuilding. + /// Which of the modes `build` declares this machine builds in. + /// + /// Chosen here rather than declared, and chosen here rather than + /// on the phone: there is one checkout and one set of build + /// outputs, so two enrolled devices holding different answers + /// would rebuild over each other with nothing on either screen to + /// say why. `None` means the first declared mode, which is why a + /// project puts the one it wants by default first. + /// + /// May name a mode that no longer exists -- a pull rewrites the + /// declaration and nothing rewrites this -- which [`ByMode::get`] + /// resolves by falling back rather than failing. + #[serde(default, skip_serializing_if = "Option::is_none")] + mode: Option, + /// The mode the recorded build was made in, beside the commit it + /// was made from. + /// + /// Measured, like `built_from`, and needed for the same reason it + /// is: switching modes moves no commit, so without this a + /// component built in `debug` and switched to `release` reads as + /// current, offers nothing to press, and serves the debug build + /// for ever. The card would look right the whole time, which is + /// the expensive kind of wrong. + #[serde(default, skip_serializing_if = "Option::is_none")] + built_mode: Option, #[serde(default, skip_serializing_if = "Option::is_none")] built_from: Option, }, } +/// What this machine decided about one component, as opposed to what the +/// project asked for. See [`Component::choices`]. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct Choices { + pub mode: Option, + pub strip: Option, +} + impl Component { pub fn name(&self) -> &str { match self { @@ -308,16 +431,118 @@ impl Component { } } - pub fn build(&self) -> &Command { + /// What this component's `build` says as written, before a mode is + /// resolved -- which the declaration check needs, since it is asking + /// about the writing rather than the result. + pub fn build_declaration(&self) -> &ByMode { + self.build_modes() + } + + /// What this component's `build` says, whichever mode it may say it + /// in. + fn build_modes(&self) -> &ByMode { match self { Self::Apk { build, .. } | Self::Server { build, .. } => build, } } - /// Whether to serve a stripped copy of this component's build. Only - /// an `Apk` has anything to strip, so a `Server` is always false - /// rather than this being an option only one variant carries. + /// Every mode this component offers, in declaration order, with the + /// first the default. Empty means one way of building, which is not + /// a choice and gets no picker. + /// + /// One list per component, declared in one place, rather than + /// gathered from whichever fields happened to name modes. That is + /// what makes "this part has a mode the other part has not heard of" + /// unsayable rather than a thing to detect: every per-mode map is + /// checked against *this*, so a missing entry is named as missing + /// instead of silently resolving to some other mode's command. + pub fn modes(&self) -> &[String] { + match self { + Self::Apk { modes, .. } | Self::Server { modes, .. } => modes, + } + } + + /// The mode this machine chose, exactly as stored -- which may name + /// one the checkout no longer declares. [`Self::effective_mode`] is + /// what to ask when the answer has to be a mode that exists. + pub fn chosen_mode(&self) -> Option<&str> { + match self { + Self::Apk { mode, .. } | Self::Server { mode, .. } => mode.as_deref(), + } + } + + /// The mode this component is actually built and run in: the one + /// chosen here if it is still declared, else the first declared, else + /// none at all for a component that offers no choice. + /// + /// One definition, because three things have to agree about it -- + /// which command runs, which service command the unit gets, and what + /// the card says the mode is. Two of them disagreeing is a build in + /// one mode reported as the other. + pub fn effective_mode(&self) -> Option<&str> { + let modes = self.modes(); + match self.chosen_mode() { + // A choice that survived the pull that could have removed it. + Some(chosen) if modes.iter().any(|name| name == chosen) => Some(chosen), + // Nobody chose, or what they chose is gone: the first + // declared, which is what the declaration order is for. + _ => modes.first().map(String::as_str), + } + } + + /// The extra arguments this component's build command needs in order + /// to know which mode it is being run in. + /// + /// The mode name, for a command written once -- which is how a + /// project with a script that takes `release` or `debug` declares it, + /// and it is why nothing has to be written twice in the ordinary + /// case. Nothing at all for a command written per mode: that command + /// *is* the answer, and appending the word as well would hand a + /// stray argument to a binary that never asked for one. + pub fn build_mode_argument(&self) -> Option<&str> { + self.build_modes().mode_argument(self.effective_mode()) + } + + /// The command that builds this component, in the mode it is set to. + /// + /// Every caller of this is unchanged from before modes existed: a + /// component that declares one command still answers with it, and one + /// that declares several answers with the selected one. Which is the + /// point of resolving it here rather than handing a mode down through + /// the build, the staleness check and the service in three separate + /// arguments that could each be passed the wrong thing. + pub fn build(&self) -> &Command { + self.build_modes().get(self.effective_mode()) + } + + /// Whether to serve a stripped copy of this component's build: what + /// this machine chose, or what the project declared when nobody here + /// has said otherwise. Only an `Apk` has anything to strip, so a + /// `Server` is always false rather than this being an option only one + /// variant carries. pub fn strip(&self) -> bool { + match self { + Self::Apk { + strip, strip_here, .. + } => strip_here.unwrap_or(*strip), + Self::Server { .. } => false, + } + } + + /// The command that prints a URL for the phone to open after + /// installing this component, if the project declares one. Only an + /// `Apk` can: it is about the app that was just installed. + pub fn enroll(&self) -> Option<&Command> { + match self { + Self::Apk { enroll, .. } if !enroll.is_empty() => Some(enroll), + Self::Apk { .. } | Self::Server { .. } => None, + } + } + + /// What the *project* asks for, as opposed to what this machine + /// settled on. Shown beside the switch so a machine that differs from + /// the checkout says so rather than quietly disagreeing with it. + pub fn strip_declared(&self) -> bool { matches!(self, Self::Apk { strip: true, .. }) } @@ -382,7 +607,14 @@ impl Component { } /// Whether these two say the same thing about what to *do*, ignoring - /// what this server measured for itself. + /// what this server measured and what this machine chose. + /// + /// Both kinds of exclusion are the same rule from two directions: the + /// gate is about what a *project* asked for, so a package read out of + /// a build and a mode picked on the phone are equally not it. A + /// chosen mode in particular must stay out -- it names one of the + /// declared modes, so including it would make choosing one look like + /// the declaration changing and ask for the acceptance again. /// /// This is the acceptance comparison: a project's file against what was /// accepted from it, so a pull that changes a command shows up as @@ -398,53 +630,76 @@ impl Component { ( Self::Apk { name, + modes, build, cwd, stale_when, also_watch, strip, + enroll, + strip_here: _, + mode: _, package: _, built_from: _, + built_mode: _, }, Self::Apk { name: other_name, + modes: other_modes, build: other_build, cwd: other_cwd, stale_when: other_stale_when, also_watch: other_also_watch, strip: other_strip, + enroll: other_enroll, + strip_here: _, + mode: _, package: _, built_from: _, + built_mode: _, }, ) => { name == other_name + && modes == other_modes && build == other_build && cwd == other_cwd && stale_when == other_stale_when && also_watch == other_also_watch && strip == other_strip + // In the gate because it runs on the build machine, + // which is the whole of what the gate is about. A + // pull must not be able to introduce a command the + // Enroll button then runs unasked. + && enroll == other_enroll } ( Self::Server { name, + modes, build, cwd, stale_when, also_watch, service, + mode: _, built_from: _, + built_mode: _, }, Self::Server { name: other_name, + modes: other_modes, build: other_build, cwd: other_cwd, stale_when: other_stale_when, also_watch: other_also_watch, service: other_service, + mode: _, built_from: _, + built_mode: _, }, ) => { name == other_name + && modes == other_modes && build == other_build && cwd == other_cwd && stale_when == other_stale_when @@ -462,6 +717,13 @@ impl Component { /// something this machine runs by hand, and its card offers no service /// controls. pub fn service(&self) -> Option<&Service> { + self.declared_service() + } + + /// The `service:` as written, without resolving a mode. Separate from + /// [`Self::service`] only because [`Self::modes`] needs it while + /// computing the very mode a resolution would use. + fn declared_service(&self) -> Option<&Service> { match self { Self::Server { service, .. } => service.as_ref(), Self::Apk { .. } => None, @@ -485,6 +747,28 @@ impl Component { } } + /// The mode the recorded build was made in. `None` both for a + /// component never built here and for one built before modes existed + /// -- the same thing to every caller, since neither is evidence that + /// what is on disk is a build of the mode now selected. + pub fn built_mode(&self) -> Option<&str> { + match self { + Self::Apk { built_mode, .. } | Self::Server { built_mode, .. } => built_mode.as_deref(), + } + } + + /// Whether what was last built here was built some other way than + /// this component is set to build now. + /// + /// Only ever true once this server has built it: with no recorded + /// commit there is no build of ours to be about, and announcing that + /// a project built by hand needs rebuilding is the false staleness + /// `built_from` is careful to avoid. The two are read together for + /// that reason. + pub fn built_in_another_mode(&self) -> bool { + self.built_from().is_some() && self.built_mode() != self.effective_mode() + } + /// Records the commit a successful build was made from. Unlike /// [`Self::set_package`] this applies to either kind, because either /// kind can fall behind the checkout. @@ -496,6 +780,60 @@ impl Component { } } + /// Records which mode a successful build was made in, alongside the + /// commit. Written from the component's own resolved mode rather than + /// from anything the caller worked out, so the record cannot describe + /// a different build from the one that ran. + pub fn set_built_mode(&mut self) { + let built = self.effective_mode().map(str::to_string); + match self { + Self::Apk { built_mode, .. } | Self::Server { built_mode, .. } => *built_mode = built, + } + } + + /// Records what somebody chose on this machine: which mode to build + /// in, and whether to strip. `None` for either leaves that one alone. + /// + /// Both at once because they arrive together -- the phone has just + /// shown a person every setting a component has, so what comes back + /// is the whole answer. + pub fn choose(&mut self, chosen_mode: Option, chosen_strip: Option) { + match self { + Self::Apk { + mode, strip_here, .. + } => { + *mode = chosen_mode; + *strip_here = chosen_strip; + } + // A server has nothing to strip, so a `strip` arriving for + // one is dropped rather than stored somewhere nothing reads. + Self::Server { mode, .. } => *mode = chosen_mode, + } + } + + /// Everything this machine decided about this component, for carrying + /// across a rewrite of the components list. + /// + /// Acceptance and the self entry's startup reconciliation both + /// replace `components` wholesale with what the checkout declares, + /// which is right for everything a project asked for and wrong for + /// everything somebody chose here. Returned as one value so a field + /// added to the set is carried by both callers or by neither. + pub fn choices(&self) -> Choices { + Choices { + mode: self.chosen_mode().map(str::to_string), + strip: match self { + Self::Apk { strip_here, .. } => *strip_here, + Self::Server { .. } => None, + }, + } + } + + /// Puts back what [`Self::choices`] took. + pub fn restore(&mut self, choices: Choices) { + self.choose(choices.mode, choices.strip); + } + /// Records what a build was measured to produce, leaving everything a /// project declared alone. The inverse of [`Self::same_declaration`]'s /// exclusion: exactly the fields that comparison ignores are the ones @@ -604,6 +942,12 @@ impl Command { Self(words) } + /// The words, for a caller that has one more to add. Cloned rather + /// than borrowed because every caller is building a new command. + pub fn to_words(&self) -> Vec { + self.0.clone() + } + /// The program and its arguments, or `None` for an empty command -- /// which is a configuration that says "no build step", not an error. pub fn split_first(&self) -> Option<(&String, &[String])> { @@ -649,6 +993,172 @@ impl Serialize for Command { } } +/// A command that may differ per **build mode**. +/// +/// A mode is a way of building the same component -- `debug` and +/// `release` are the ones every toolchain has, but the names are the +/// project's, because what a second way of building means is the +/// project's business and not this server's. Which one to use is chosen +/// on the build machine and stored beside the component; see +/// [`Component::chosen_mode`]. +/// +/// Two forms rather than an optional list, because a component either +/// offers a choice or does not: +/// +/// ```text +/// build: "cargo build" // one way, no name +/// build: { // two, named +/// "release": "cargo build --release", +/// "debug": "cargo build", +/// } +/// ``` +/// +/// The single form is what every project wrote before modes existed and +/// still means exactly what it did, which is the point of accepting both +/// here rather than making everyone grow a mode called something. +/// +/// **Order is the declaration's.** The first mode is the default, so a +/// project puts the one it wants built by default first -- ordinarily +/// `release`. A map preserves it because this stores pairs rather than a +/// `HashMap`; RON hands them over in file order and they are written back +/// in the same. +/// +/// Only [`Command`] goes in here, and that is load-bearing rather than +/// incidental: telling the map form apart from the single form is done +/// with `deserialize_any`, and RON reports a struct (`(path: "a")`) as a +/// *map* -- so a struct-valued field behind this would read its own +/// fields as mode names. `Command` is a string or a sequence, neither of +/// which collides. A field that wants modes and is not a command puts +/// this inside itself instead, which is what [`Service`] does. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ByMode { + /// One command, whatever the mode. + One(Command), + /// One per named mode, in declaration order. + Modes(Vec<(String, Command)>), +} + +/// What [`ByMode::get`] answers for a mode a component does not declare +/// and has no default for -- the same as declaring nothing, which every +/// caller already handles as "no build step". +static NO_COMMAND: Command = Command(Vec::new()); + +impl Default for ByMode { + fn default() -> Self { + Self::One(Command::default()) + } +} + +impl ByMode { + /// The mode names, in declaration order. Empty for a single command: + /// one way of building is not a choice, and a picker offering one + /// entry is a control that cannot do anything. + pub fn modes(&self) -> impl Iterator { + let named: &[(String, Command)] = match self { + Self::One(_) => &[], + Self::Modes(modes) => modes, + }; + named.iter().map(|(name, _)| name.as_str()) + } + + /// The command for `mode`, falling back to the first declared one. + /// + /// Total on purpose. A mode is chosen on this machine and the modes + /// are declared in the checkout, so a pull can rename or remove the + /// one that was chosen -- and this is read on the manifest path, + /// where the alternative to falling back is a card that cannot be + /// drawn. The fallback is the same answer as before anybody chose, + /// which is what a phone's stale variant choice already does. + pub fn get(&self, mode: Option<&str>) -> &Command { + match self { + Self::One(command) => command, + Self::Modes(modes) => mode + .and_then(|mode| modes.iter().find(|(name, _)| name == mode)) + .or_else(|| modes.first()) + .map(|(_, command)| command) + .unwrap_or(&NO_COMMAND), + } + } + + /// The mode name, when this command has to be *told* which mode it + /// is being run in, and nothing when it does not. + /// + /// The rule in one line: a command written per mode already says + /// which mode it is by being the one that was chosen, so it is not + /// told again. Written once, it is told, which is what lets a project + /// with a script that takes `release` or `debug` name that script a + /// single time. + pub fn mode_argument<'a>(&self, mode: Option<&'a str>) -> Option<&'a str> { + match self { + Self::One(_) => mode, + Self::Modes(_) => None, + } + } + + /// Nothing to run in any mode. An empty map counts, since a project + /// that wrote `{}` declared no way of building this either. + pub fn is_empty(&self) -> bool { + match self { + Self::One(command) => command.is_empty(), + Self::Modes(modes) => modes.iter().all(|(_, command)| command.is_empty()), + } + } +} + +impl<'de> Deserialize<'de> for ByMode { + fn deserialize>(deserializer: D) -> Result { + use serde::de; + + struct Either; + impl<'de> de::Visitor<'de> for Either { + type Value = ByMode; + fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + f.write_str(r#"a command, or {"mode": "command"} for one per mode"#) + } + // The two single-command forms, forwarded rather than + // reimplemented so there is still one definition of where a + // command's argument boundaries are. + fn visit_str(self, line: &str) -> Result { + Ok(ByMode::One(Command::from_line(line))) + } + fn visit_seq>(self, seq: A) -> Result { + Command::deserialize(de::value::SeqAccessDeserializer::new(seq)).map(ByMode::One) + } + fn visit_map>(self, mut map: A) -> Result { + let mut modes = Vec::new(); + while let Some((name, command)) = map.next_entry::()? { + if modes.iter().any(|(seen, _)| seen == &name) { + return Err(de::Error::custom(format!( + "the mode {name} is declared twice" + ))); + } + modes.push((name, command)); + } + Ok(ByMode::Modes(modes)) + } + } + deserializer.deserialize_any(Either) + } +} + +impl Serialize for ByMode { + fn serialize(&self, serializer: S) -> Result { + match self { + // Written back as the bare command, so a component that + // declares no modes round-trips to the file it came from. + Self::One(command) => command.serialize(serializer), + Self::Modes(modes) => { + use serde::ser::SerializeMap; + let mut map = serializer.serialize_map(Some(modes.len()))?; + for (name, command) in modes { + map.serialize_entry(name, command)?; + } + map.end() + } + } + } +} + /// "`path` is out of date with respect to `older_than`" -- the whole /// staleness vocabulary, deliberately. /// @@ -773,6 +1283,78 @@ pub struct ProjectFile { pub error: Option, } +/// Why this component's modes do not add up, if they do not. +/// +/// With one declared list per component there is only one thing left to +/// get wrong: a field written *per mode* naming a different set from the +/// list. Checked rather than resolved, because there is no right guess -- +/// [`ByMode::get`] answers an unknown name by falling back to the first, +/// which for a server means building `release` and starting `debug` with +/// nothing anywhere saying so. +/// +/// A per-mode map on a component that declares no modes is the same +/// mistake seen from the other side, and is refused with the same +/// sentence: the list is where modes are declared, and a map is only ever +/// a way of answering for the modes already there. +fn mode_problem(component: &Component) -> Option { + let declared = component.modes(); + let mut fields = vec![("build", component.build_declaration())]; + if let Some(service) = component.service() { + fields.push(("service", service.by_mode())); + } + for (field, by_mode) in fields { + let named: Vec<&str> = by_mode.modes().collect(); + if named.is_empty() { + // Written once, which covers every mode -- including none. + continue; + } + if declared.is_empty() { + return Some(format!( + "the component {}'s {field} is written per mode ({}), but the component declares \ + no modes -- add `modes: [{}]` beside it, which is the one place a mode is \ + declared", + component.name(), + named.join(", "), + named + .iter() + .map(|name| format!("\"{name}\"")) + .collect::>() + .join(", "), + )); + } + let missing: Vec<&str> = declared + .iter() + .map(String::as_str) + .filter(|mode| !named.contains(mode)) + .collect(); + let extra: Vec<&str> = named + .iter() + .copied() + .filter(|mode| !declared.iter().any(|declared| declared == mode)) + .collect(); + if !missing.is_empty() || !extra.is_empty() { + return Some(format!( + "the component {}'s {field} answers for [{}] but the component declares modes \ + [{}]: {} has no {field}, and {} is not a mode", + component.name(), + named.join(", "), + declared.join(", "), + list_or_nothing(&missing), + list_or_nothing(&extra), + )); + } + } + None +} + +fn list_or_nothing(names: &[&str]) -> String { + if names.is_empty() { + "nothing".to_string() + } else { + names.join(", ") + } +} + /// What `project` says about itself, or an empty declaration for a project /// that says nothing -- which is the normal case. /// @@ -790,10 +1372,24 @@ pub fn project_config(project: &Path) -> ProjectFile { let Ok(text) = std::fs::read_to_string(&path) else { return ProjectFile::default(); }; - match wg_app_link::format::parse(&text) { - Ok(declaration) => ProjectFile { - declaration, - error: None, + match wg_app_link::format::parse::(&text) { + // Parsing is not the whole of being readable. A mode set that + // does not add up is discarded exactly as an unknown field is, + // and for the same reason: what would otherwise happen is that + // the project builds in one mode and runs another, from a card + // that looks entirely ordinary. + Ok(declaration) => match declaration.components.iter().find_map(mode_problem) { + None => ProjectFile { + declaration, + error: None, + }, + Some(problem) => { + tracing::warn!("ignoring {}: {problem}", path.display()); + ProjectFile { + declaration: Declaration::default(), + error: Some(format!("{}: {problem}", path.display())), + } + } }, Err(err) => { tracing::warn!("ignoring {}: {err}", path.display()); @@ -945,11 +1541,13 @@ mod tests { for (text, expected) in [ ( r#"service: Script("server/service")"#, - Service::Script(Command::from_line("server/service")), + Service::Script(ByMode::One(Command::from_line("server/service"))), ), ( r#"service: Managed("target/release/ai-server --port 8080")"#, - Service::Managed(Command::from_line("target/release/ai-server --port 8080")), + Service::Managed(ByMode::One(Command::from_line( + "target/release/ai-server --port 8080", + ))), ), ] { let parsed = component(&format!(r#"Server(name: "s", build: "b", {text})"#)) @@ -960,6 +1558,207 @@ mod tests { } } + /// The form every project wrote before modes existed still means + /// what it did, and still comes back out of a render unchanged. This + /// is the whole reason `build:` accepts two shapes rather than every + /// project growing a mode called something. + #[test] + fn one_command_is_still_one_command() { + let parsed = component(r#"Apk(name: "app", build: "./build.sh")"#).expect("parse"); + assert!( + parsed.modes().is_empty(), + "one way of building is no choice" + ); + assert_eq!(parsed.effective_mode(), None); + assert_eq!(parsed.build().to_line(), "./build.sh"); + let written = wg_app_link::format::render(&parsed).expect("render"); + assert!( + written.contains(r#"build: "./build.sh""#), + "the bare command did not survive: {written}" + ); + } + + /// Order is the declaration's, and the first is what gets built when + /// nobody has chosen -- which is why a project puts `release` first. + #[test] + fn the_first_declared_mode_is_the_default() { + let parsed = component( + r#"Apk(name: "app", modes: ["release", "debug"], build: { + "release": "./build.sh release", + "debug": "./build.sh debug", + })"#, + ) + .expect("parse"); + assert_eq!(parsed.modes(), vec!["release", "debug"]); + assert_eq!(parsed.effective_mode(), Some("release")); + assert_eq!(parsed.build().to_line(), "./build.sh release"); + } + + /// Choosing one picks its command, and choosing one that a pull has + /// since removed falls back to the first rather than failing -- this + /// is read on the manifest path, where the alternative to a fallback + /// is a card that cannot be drawn at all. + #[test] + fn a_chosen_mode_picks_its_command_and_a_stale_one_falls_back() { + let mut parsed = + component(r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "r", "debug": "d"})"#).expect("parse"); + parsed.choose(Some("debug".to_string()), None); + assert_eq!(parsed.effective_mode(), Some("debug")); + assert_eq!(parsed.build().to_line(), "d"); + + parsed.choose(Some("profile".to_string()), None); + assert_eq!(parsed.effective_mode(), Some("release")); + assert_eq!(parsed.build().to_line(), "r"); + } + + /// A mode map round-trips as a map, so a config written back by this + /// server is still the file it was read from. The two halves only + /// round-trip together, which is what this is here to hold. + #[test] + fn a_mode_map_is_written_back_as_a_map() { + let text = r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "r", "debug": "d"})"#; + let parsed = component(text).expect("parse"); + let written = wg_app_link::format::render(&parsed).expect("render"); + let again = component(&written).expect("re-parse what was written"); + assert_eq!(parsed, again, "did not survive a round trip: {written}"); + } + + /// Picking a mode is not the project asking for something new, so it + /// must not re-open the acceptance gate -- a card that asked to be + /// accepted again every time somebody changed a setting would teach + /// people to press it without reading. + #[test] + fn choosing_a_mode_or_strip_is_not_a_changed_declaration() { + let declared = + component(r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "r", "debug": "d"}, strip: true)"#) + .expect("parse"); + let mut chosen = declared.clone(); + chosen.choose(Some("debug".to_string()), Some(false)); + assert!(declared.same_declaration(&chosen)); + assert!(chosen.same_declaration(&declared)); + // ...while a changed command still is one. + let other = + component(r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "x", "debug": "d"})"#).expect("parse"); + assert!(!declared.same_declaration(&other)); + } + + /// This machine's answer wins where it has one, and the project's + /// stands where it does not -- so a project that changes its mind + /// still reaches every machine that never overrode it. + #[test] + fn strip_follows_the_declaration_until_this_machine_says_otherwise() { + let mut parsed = component(r#"Apk(name: "app", build: "b", strip: true)"#).expect("parse"); + assert!(parsed.strip()); + assert!(parsed.strip_declared()); + parsed.choose(None, Some(false)); + assert!(!parsed.strip()); + assert!(parsed.strip_declared(), "the project still asks for it"); + } + + /// A server names its modes on `service:` alone when both modes build + /// the same way, and the union is what the component offers. + #[test] + fn a_service_may_be_where_the_modes_are_named() { + let parsed = component( + r#"Server(name: "s", modes: ["release", "debug"], build: "cargo build --all", service: Managed({ + "release": "target/release/s", + "debug": "target/debug/s", + }))"#, + ) + .expect("parse"); + assert_eq!(parsed.modes(), vec!["release", "debug"]); + assert_eq!(parsed.build().to_line(), "cargo build --all"); + } + + /// With one declared list per component there is one thing left to + /// get wrong: a field written per mode that answers for a different + /// set. Refused with both sets named, because resolving it would + /// build one mode and run another with nothing on the card saying so. + #[test] + fn a_per_mode_field_must_answer_for_the_modes_that_were_declared() { + let missing = component( + r#"Server(name: "s", modes: ["release", "debug"], build: "b", service: Managed({ + "release": "target/release/s", + }))"#, + ) + .expect("parse"); + let problem = mode_problem(&missing).expect("this should not add up"); + assert!(problem.contains("debug has no service"), "{problem}"); + + let unknown = component( + r#"Server(name: "s", modes: ["release"], build: {"release": "r", "quick": "q"})"#, + ) + .expect("parse"); + let problem = mode_problem(&unknown).expect("quick is not a declared mode"); + assert!(problem.contains("quick is not a mode"), "{problem}"); + + // The list is the one place modes are declared, so a map without + // one is the same mistake from the other side -- and the refusal + // says what to write rather than only what is wrong. + let undeclared = + component(r#"Apk(name: "app", build: {"release": "r", "debug": "d"})"#).expect("parse"); + let problem = mode_problem(&undeclared).expect("no modes are declared"); + assert!( + problem.contains(r#"modes: ["release", "debug"]"#), + "{problem}" + ); + + // Answering for exactly the declared set says nothing, and so + // does a command written once -- which covers every mode. + let fine = component( + r#"Server(name: "s", modes: ["release", "debug"], build: "./build.sh", service: Managed({ + "debug": "target/debug/s", "release": "target/release/s", + }))"#, + ) + .expect("parse"); + assert_eq!(mode_problem(&fine), None); + } + + /// A command written once is handed the mode; a command written per + /// mode is not handed it again. Getting this the other way round + /// passes a stray word to a binary that never asked for one, which + /// for a service is a process that will not start. + #[test] + fn only_a_command_written_once_is_told_which_mode_it_is() { + let script = + component(r#"Apk(name: "app", modes: ["release", "debug"], build: "./build.sh")"#) + .expect("parse"); + assert_eq!(script.build().to_line(), "./build.sh"); + assert_eq!(script.build_mode_argument(), Some("release")); + + let per_mode = component( + r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "r", "debug": "d"})"#, + ) + .expect("parse"); + assert_eq!(per_mode.build().to_line(), "r"); + assert_eq!(per_mode.build_mode_argument(), None); + + // And a component with no modes at all is told nothing, which is + // every project that existed before modes did. + let plain = component(r#"Apk(name: "app", build: "./build.sh")"#).expect("parse"); + assert_eq!(plain.build_mode_argument(), None); + } + + /// Switching modes moves no commit, so nothing about the checkout can + /// say that what is on disk is a build of something else. This is the + /// only thing that can, and it stays quiet for a component this + /// server has never built -- a project built by hand must not be told + /// it is out of date. + #[test] + fn a_build_from_another_mode_is_not_this_mode_s_build() { + let mut parsed = + component(r#"Apk(name: "app", modes: ["release", "debug"], build: {"release": "r", "debug": "d"})"#).expect("parse"); + assert!( + !parsed.built_in_another_mode(), + "never built here, so there is no build of ours to be about" + ); + parsed.set_built_from("abc123".to_string()); + parsed.set_built_mode(); + assert!(!parsed.built_in_another_mode()); + parsed.choose(Some("debug".to_string()), None); + assert!(parsed.built_in_another_mode()); + } + /// A server that declares no service at all is a real state -- it /// builds something a person runs by hand -- and must not become an /// error or a service with an empty command. diff --git a/server/src/main.rs b/server/src/main.rs index 58081a3..e8e8b48 100644 --- a/server/src/main.rs +++ b/server/src/main.rs @@ -41,6 +41,7 @@ mod registry; mod resources; mod restart; mod routes; +mod script; mod sdk; mod service; mod shipped; diff --git a/server/src/registry.rs b/server/src/registry.rs index b77fa8f..7cf197f 100644 --- a/server/src/registry.rs +++ b/server/src/registry.rs @@ -304,6 +304,39 @@ fn component_id(component: &Component) -> ComponentId { ) } +/// What this machine chose about each component, for carrying across a +/// rewrite of the components list. +/// +/// The mirror of [`measured_packages`], and here for the same reason: +/// both acceptance and the self entry's startup reconciliation replace +/// `components` wholesale with what the checkout declares, which is right +/// for everything the project asked for and wrong for everything somebody +/// chose here. Without it, choosing to build this server in `debug` +/// would last exactly until the next restart -- and restarting is how +/// this server is updated, so it would never last at all. +/// +/// Keyed the same way for the same reason: once a component's directory +/// decides which builds are its own, a reused name is a different +/// component, and handing it the old one's mode would build something +/// nobody asked for. +fn chosen_settings(components: &[Component]) -> HashMap { + components + .iter() + .map(|component| (component_id(component), component.choices())) + .collect() +} + +fn restore_chosen_settings( + components: &mut [Component], + chosen: &HashMap, +) { + for component in components { + if let Some(choices) = chosen.get(&component_id(component)) { + component.restore(choices.clone()); + } + } +} + fn measured_packages(components: &[Component]) -> HashMap { components .iter() @@ -492,6 +525,11 @@ impl AppState { .find(|candidate| candidate.name() == component) { built.set_built_from(sha); + // Beside the commit and in the same write: the mode is + // half of "what is on disk", and a record of one without + // the other is a component that looks current in a mode + // it was never built in. + built.set_built_mode(); } Ok(()) }); @@ -604,13 +642,18 @@ impl AppState { // way to start running its commands. components: vec![Component::Apk { name: "app".to_string(), - build: crate::config::Command::default(), + modes: Vec::new(), + build: crate::config::ByMode::default(), cwd: None, stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package, built_from: None, + built_mode: None, }], }); Ok(key) @@ -661,6 +704,46 @@ impl AppState { }) } + /// Records what somebody chose for one component from the phone: + /// which declared mode to build it in, and whether to strip it. + /// + /// A whole-component write rather than a field at a time, because the + /// phone has just shown a person every setting the component has and + /// what comes back is the complete answer. + /// + /// The mode is checked against what the component actually declares. + /// An unknown one is refused rather than stored: `ByMode::get` would + /// fall back to the first mode, so storing it would leave the card + /// naming a mode nothing builds in -- which reads exactly like a + /// setting that took effect. + pub fn set_component_choices( + &self, + key: &str, + component: &str, + mode: Option, + strip: Option, + ) -> Result<()> { + self.update(|config| { + let project = config + .projects + .iter_mut() + .find(|project| project.key == key) + .with_context(|| format!("no app named {key}"))?; + let target = project + .components + .iter_mut() + .find(|candidate| candidate.name() == component) + .with_context(|| format!("{key} has no component named {component}"))?; + if let Some(mode) = &mode + && !target.modes().iter().any(|name| name == mode) + { + bail!("{component} has no build mode called {mode}"); + } + target.choose(mode, strip); + Ok(()) + }) + } + /// Accepts what this project asks to have run, after a person has read /// it on the phone. From here on those components are ordinary /// configuration, indistinguishable from hand-written ones. @@ -695,6 +778,7 @@ impl AppState { .find(|project| project.key == key) .with_context(|| format!("{key} is built in and has nothing to accept"))?; let measured = measured_packages(&project.components); + let chosen = chosen_settings(&project.components); project.git_pull = declared.git_pull; // Everything `matches_accepted` compares has to be written // here, or accepting cannot clear the gate. `resources` joined @@ -710,6 +794,12 @@ impl AppState { component.set_package(package.clone()); } } + // Accepting is about what the project asks to have run. It is + // not somebody withdrawing the mode they picked or the strip + // they turned off, so those survive it -- exactly as the + // measured package does, and for the same reason: neither is + // part of what was being agreed to. + restore_chosen_settings(&mut project.components, &chosen); Ok(()) }) } @@ -775,13 +865,18 @@ fn reconcile_self(config: &mut Config, self_project: &Path) -> bool { None => { components.push(Component::Apk { name: "app".to_string(), - build: crate::config::Command::default(), + modes: Vec::new(), + build: crate::config::ByMode::default(), cwd: None, stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, + mode: None, package: None, built_from: None, + built_mode: None, }); "app".to_string() } @@ -791,6 +886,14 @@ fn reconcile_self(config: &mut Config, self_project: &Path) -> bool { .projects .iter_mut() .find(|project| project.key == SELF_KEY); + // Carried across the row being rebuilt, like `gitIpv4` below: what is + // *derived* here is what the checkout declares, and a mode chosen for + // this server's own component is not that. Restarting is how this + // server takes an update, so a choice that did not survive a restart + // would not survive being acted on. + if let Some(existing) = existing.as_ref() { + restore_chosen_settings(&mut components, &chosen_settings(&existing.components)); + } let derived = ProjectConfig { key: SELF_KEY.to_string(), label: declared.label.unwrap_or_else(|| SELF_LABEL.to_string()), @@ -946,13 +1049,18 @@ mod tests { fn asking_for(command: &str) -> Vec { vec![Component::Apk { name: "app".to_string(), - build: crate::config::Command::from_line(command), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::from_line(command)), cwd: None, stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, package: None, + mode: None, built_from: None, + built_mode: None, }] } @@ -964,6 +1072,16 @@ mod tests { .expect("write"); } + /// A component's declaration in the project file, with modes. + fn write_moded_request(project: &Path) { + std::fs::write( + project.join(crate::config::PROJECT_CONFIG_FILE), + "components: [Apk(name: \"app\", modes: [\"release\", \"debug\"], \ + build: {\"release\": \"r\", \"debug\": \"d\"})],\n", + ) + .expect("write"); + } + /// A live `AppState` holding one configured app pointed at `project`, /// with nothing accepted yet. /// @@ -1027,13 +1145,18 @@ mod tests { fn apk_named(name: &str, cwd: Option<&str>) -> Component { Component::Apk { name: name.to_string(), - build: crate::config::Command::default(), + modes: Vec::new(), + build: crate::config::ByMode::One(crate::config::Command::default()), cwd: cwd.map(PathBuf::from), stale_when: None, also_watch: Vec::new(), strip: false, + enroll: crate::config::Command::default(), + strip_here: None, package: None, + mode: None, built_from: None, + built_mode: None, } } @@ -1178,6 +1301,59 @@ mod tests { ); } + /// Accepting is about what the project asks to have *run*. It is not + /// somebody withdrawing the mode they picked, so the choice survives + /// -- exactly as the measured package does, and it has to be written + /// here rather than asserted from a hand-built config, because what + /// "accepted" means is whatever `approve_declaration` writes. + #[test] + fn a_chosen_mode_survives_the_declaration_being_accepted_again() { + let dir = tempfile::tempdir().expect("tempdir"); + write_moded_request(dir.path()); + let (_home, state) = state_with(dir.path()); + state.approve_declaration("demo").expect("approve"); + state + .set_component_choices("demo", "app", Some("debug".to_string()), None) + .expect("choose the debug mode"); + assert_eq!( + state.entry("demo").expect("entry").components[0].effective_mode(), + Some("debug"), + ); + + state.approve_declaration("demo").expect("approve again"); + let component = &state.entry("demo").expect("entry").components[0]; + assert_eq!( + component.effective_mode(), + Some("debug"), + "accepting must not silently put the build back to the default mode", + ); + assert_eq!(component.build().to_line(), "d"); + } + + /// A mode this component does not declare is refused rather than + /// stored: `ByMode::get` falls back to the first, so storing it would + /// leave the card naming a mode nothing is built in -- which reads + /// exactly like a setting that took effect. + #[test] + fn a_mode_the_component_does_not_declare_is_refused() { + let dir = tempfile::tempdir().expect("tempdir"); + write_moded_request(dir.path()); + let (_home, state) = state_with(dir.path()); + state.approve_declaration("demo").expect("approve"); + let refused = state + .set_component_choices("demo", "app", Some("profile".to_string()), None) + .expect_err("there is no such mode"); + assert!( + refused.to_string().contains("profile"), + "the refusal should name what was asked for: {refused}", + ); + assert_eq!( + state.entry("demo").expect("entry").components[0].effective_mode(), + Some("release"), + "and it must leave the component where it was", + ); + } + /// The case the gate exists for, and the one a cached answer got /// wrong: the project file changes on a *pull*, which writes no config /// and so rebuilds no entries. An entry built while the request diff --git a/server/src/resources.rs b/server/src/resources.rs index d2c6527..b782af8 100644 --- a/server/src/resources.rs +++ b/server/src/resources.rs @@ -58,7 +58,7 @@ pub fn read(project: &Path, declaration: &Resources) -> Result { - let text = run(project, command)?; + let text = crate::script::capture(project, None, command, SCRIPT_TIMEOUT)?; parse(&text).map_err(|err| format!("in the output of {}: {err}", command.to_line())) } } @@ -74,55 +74,6 @@ fn parse(text: &str) -> Result { wg_app_link::format::parse(text).map_err(|err| err.to_string()) } -fn run(project: &Path, command: &crate::config::Command) -> Result { - use std::io::Read; - - let mut child = command - .to_process(project, None, &[])? - // Closed, because a script that asks a question would otherwise - // wait for an answer nobody is there to give -- the same rule the - // service scripts run under. - .stdin(std::process::Stdio::null()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .map_err(|err| format!("starting {}: {err}", command.to_line()))?; - - let deadline = std::time::Instant::now() + SCRIPT_TIMEOUT; - loop { - match child.try_wait() { - Err(err) => return Err(format!("waiting on {}: {err}", command.to_line())), - Ok(Some(status)) => { - let mut out = String::new(); - if let Some(mut pipe) = child.stdout.take() { - let _ = pipe.read_to_string(&mut out); - } - if status.success() { - return Ok(out); - } - let mut err = String::new(); - if let Some(mut pipe) = child.stderr.take() { - let _ = pipe.read_to_string(&mut err); - } - return Err(format!( - "{} failed ({status}): {}", - command.to_line(), - err.lines().next().unwrap_or("no output").trim() - )); - } - Ok(None) if std::time::Instant::now() >= deadline => { - let _ = child.kill(); - return Err(format!( - "{} did not answer within {}s", - command.to_line(), - SCRIPT_TIMEOUT.as_secs() - )); - } - Ok(None) => std::thread::sleep(std::time::Duration::from_millis(20)), - } - } -} - /// One project to read, and where from. pub struct Target { pub key: String, diff --git a/server/src/routes.rs b/server/src/routes.rs index 8bcf695..971d52e 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -22,6 +22,12 @@ //! services again //! PUT /apps/{key}/settings {gitIpv4} //! this machine's preferences for it +//! PUT /apps/{key}/components/{name}/settings {mode, strip} +//! this machine's preferences for one +//! component +//! POST /apps/{key}/components/{name}/enroll-link +//! run that component's `enroll:` and +//! answer the URL it printed //! GET /apps/{key}/components/{name}/logs[?lines=&generation=] //! what that component wrote //! POST /apps/{key}/components/{name}/{action} @@ -98,6 +104,21 @@ pub fn tls_router(state: Arc) -> Router { // action -- axum matches a literal segment ahead of a capture, // but declaring it first says so to a reader too. .route("/apps/{key}/components/{name}/logs", get(component_logs)) + // Beside `logs` and ahead of `{action}` for the same reason, even + // though the methods already keep them apart: a reader should not + // have to check that `settings` is not an action. + .route( + "/apps/{key}/components/{name}/settings", + put(set_component_settings), + ) + // Its own route rather than one more `{action}`: those drive a + // server's service and answer with what the service is doing, + // where this belongs to an APK and answers with a URL. Folding it + // in would have made the action list mean two things. + .route( + "/apps/{key}/components/{name}/enroll-link", + post(enrollment_link), + ) .route( "/apps/{key}/components/{name}/{action}", post(service_action), @@ -236,6 +257,14 @@ struct ManifestApk { /// would mean running the strip pipeline here; see /// `strip::serveable_now`. size: u64, + /// Whether a debug-symbol-stripped copy is what gets served -- what + /// this machine settled on, which is what the download will actually + /// do. + strip: bool, + /// What the *project* asks for, which is only different when somebody + /// overrode it here. Sent so the sheet can say the two differ rather + /// than showing a switch that silently disagrees with the checkout. + strip_declared: bool, variants: Vec, } @@ -332,6 +361,21 @@ struct ManifestComponent { /// project that keeps nothing. #[serde(skip_serializing_if = "Option::is_none")] resources_error: Option, + /// This component can hand the phone a link to open after it is + /// installed, because the project declares a command that prints one. + /// What the link does is the project's business; the button just + /// opens it. + has_enroll_link: bool, + /// Every way this component can be built, in declaration order, with + /// the first the default. Empty for a component that declares one + /// way, which is not a choice -- the settings sheet says so rather + /// than drawing a picker with one entry in it. + modes: Vec, + /// Which of them it is set to build in. Absent exactly when `modes` + /// is empty; when it is not, this is always one of them, including + /// the case where nobody has chosen and it is the first. + #[serde(skip_serializing_if = "Option::is_none")] + mode: Option, /// What there is to install, for a component that produces an APK. #[serde(skip_serializing_if = "Option::is_none")] apk: Option, @@ -415,6 +459,12 @@ impl ManifestComponent { resources_error: is_server .then(|| state.resource_checks.error(key)) .flatten(), + // Gated the same way `has_build` is: while a declaration is + // waiting to be accepted the command will not run, so + // offering the button would be offering one that refuses. + has_enroll_link: may_build && component.enroll().is_some(), + modes: component.modes().to_vec(), + mode: component.effective_mode().map(str::to_string), apk: match is_server { true => None, false => Some(ManifestApk::read(state, key, entry, component).await?), @@ -455,6 +505,8 @@ impl ManifestApk { .map(|apk| epoch_secs(apk.modified)) .unwrap_or(0.0), size, + strip: component.strip(), + strip_declared: component.strip_declared(), variants: entry .variants(component) .into_iter() @@ -1220,6 +1272,113 @@ async fn set_settings( Ok(StatusCode::NO_CONTENT) } +#[derive(Serialize)] +#[serde(rename_all = "camelCase")] +struct EnrollmentLink { + url: String, +} + +/// How long a project's `enroll:` command is given before it is given up +/// on. Somebody is holding a phone waiting for it, so shorter than a +/// service action's minute -- but long enough for a command that has to +/// touch a keystore or a config file on the way. +const ENROLL_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(20); + +/// Runs one component's `enroll:` and answers the URL it printed, for the +/// phone to open. +/// +/// Run on every press rather than cached anywhere. The link a project +/// mints is ordinarily one-shot and carries a credential, so a stored one +/// would be both stale and a secret sitting in a file -- and a project +/// that mints a fresh token each time is exactly the shape this is for. +/// +/// One line, and the first: the contract is a single URL on stdout, which +/// makes a project's diagnostics stderr's business. Trimmed rather than +/// parsed -- what a URL means is the phone's business and not this +/// server's, so nothing here inspects the scheme. +async fn enrollment_link( + State(state): State>, + UrlPath((key, name)): UrlPath<(String, String)>, +) -> Result, ApiError> { + let entry = state.entry(&key).ok_or(ApiError::UnknownApp(key.clone()))?; + // The same gate the build runs behind, asked here rather than + // inherited: this is a command the project declared, so a pull must + // not be able to introduce one that a press then runs. + if entry.pending_declaration().is_some() { + return Err(ApiError::BadRequest(format!( + "{} is asking to have a command accepted before anything of its own runs", + entry.label + ))); + } + let component = entry + .components + .iter() + .find(|component| component.name() == name) + .ok_or_else(|| ApiError::UnknownComponent(key.clone(), name.clone()))?; + let command = component + .enroll() + .ok_or_else(|| { + ApiError::BadRequest(format!("{name} does not say how to get an enrolment link")) + })? + .clone(); + let project = entry.project_path.clone(); + let cwd = component.cwd().map(std::path::Path::to_path_buf); + let printed = tokio::task::spawn_blocking(move || { + crate::script::capture(&project, cwd.as_deref(), &command, ENROLL_TIMEOUT) + }) + .await + .context("the enrolment command panicked")? + .map_err(|err| ApiError::Internal(anyhow::anyhow!(err)))?; + let url = printed + .lines() + .next() + .unwrap_or_default() + .trim() + .to_string(); + if url.is_empty() { + return Err(ApiError::Internal(anyhow::anyhow!( + "{name}'s enrolment command printed nothing on stdout" + ))); + } + Ok(Json(EnrollmentLink { url })) +} + +/// What somebody chose for one component on the settings sheet. +/// +/// `mode` absent means "no choice, take the first declared one", which is +/// a real answer rather than a missing field -- it is what a component +/// reverts to and what every component starts as. `strip` absent means +/// the same about the project's own declaration. +#[derive(Deserialize)] +#[serde(rename_all = "camelCase")] +struct ComponentSettingsBody { + #[serde(default)] + mode: Option, + #[serde(default)] + strip: Option, +} + +/// This machine's preferences for one component: which declared mode to +/// build it in, and whether to strip what it serves. +/// +/// On the build machine rather than on the device, unlike the variant a +/// download names: a mode decides what gets *built*, and there is one +/// checkout and one set of outputs, so two enrolled phones holding +/// different answers would rebuild over each other with nothing on either +/// screen to say why. Which of the finished builds a phone installs stays +/// that phone's business. +async fn set_component_settings( + State(state): State>, + UrlPath((key, name)): UrlPath<(String, String)>, + Json(body): Json, +) -> Result { + mutate(state, move |state| { + state.set_component_choices(&key, &name, body.mode, body.strip) + }) + .await?; + Ok(StatusCode::NO_CONTENT) +} + async fn set_roots( State(state): State>, Json(body): Json, diff --git a/server/src/script.rs b/server/src/script.rs new file mode 100644 index 0000000..75b588c --- /dev/null +++ b/server/src/script.rs @@ -0,0 +1,76 @@ +//! Running a command a project declared and keeping what it printed. +//! +//! Two things here ask a project to answer a question by running +//! something: `resources`, which reads where a project keeps its state, +//! and the enrolment link on an `Apk` component. Both want the same +//! bargain -- stdin closed, a deadline, stdout kept, and a failure that +//! names the command and the first line of its stderr -- so the bargain +//! is written once rather than twice with the second copy quietly +//! drifting. +//! +//! Deliberately *not* what a build runs through. A build's output is +//! streamed as it arrives, because it is long and somebody is watching a +//! progress bar; these are short and their whole value is the string at +//! the end. + +use std::path::Path; +use std::time::{Duration, Instant}; + +use crate::config::Command; + +/// Runs `command` in the project and answers its stdout. +/// +/// Stdin is closed rather than inherited. A script that prompts for a +/// password gets end-of-file and fails, instead of hanging until the +/// deadline with a card stuck on whatever it was doing -- the same +/// bargain the service scripts make. +pub fn capture( + project: &Path, + cwd: Option<&Path>, + command: &Command, + timeout: Duration, +) -> Result { + use std::io::Read; + + let mut child = command + .to_process(project, cwd, &[])? + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn() + .map_err(|err| format!("starting {}: {err}", command.to_line()))?; + + let deadline = Instant::now() + timeout; + loop { + match child.try_wait() { + Err(err) => return Err(format!("waiting on {}: {err}", command.to_line())), + Ok(Some(status)) => { + let mut out = String::new(); + if let Some(mut pipe) = child.stdout.take() { + let _ = pipe.read_to_string(&mut out); + } + if status.success() { + return Ok(out); + } + let mut err = String::new(); + if let Some(mut pipe) = child.stderr.take() { + let _ = pipe.read_to_string(&mut err); + } + return Err(format!( + "{} failed ({status}): {}", + command.to_line(), + err.lines().next().unwrap_or("no output").trim() + )); + } + Ok(None) if Instant::now() >= deadline => { + let _ = child.kill(); + return Err(format!( + "{} did not answer within {}s", + command.to_line(), + timeout.as_secs() + )); + } + Ok(None) => std::thread::sleep(Duration::from_millis(20)), + } + } +} diff --git a/server/src/service.rs b/server/src/service.rs index 20540c3..e873832 100644 --- a/server/src/service.rs +++ b/server/src/service.rs @@ -80,19 +80,42 @@ impl ServiceState { /// declares no service, which are the same thing to every caller: there /// is nothing to drive. pub fn driver(key: &str, component: &Component) -> Option { - match component.service()? { - Service::Script(script) if !script.is_empty() => Some(script.clone()), - Service::Managed(run) if !run.is_empty() => Some(Command::from_words(vec![ + let service = component.service()?; + // Resolved here, once, from the component's own mode. Which binary a + // managed service runs is exactly what a build mode changes, so a + // second place deciding the mode is a service started from the other + // mode's output -- which looks like a build that did nothing. + let mode = component.effective_mode(); + let command = service.command(mode); + if command.is_empty() { + // Declared empty, which says the same as not declaring it. + return None; + } + // The same rule the build follows: a command written once is told + // which mode it is running in, a command written per mode is not told + // twice. It lands *before* the subcommand the caller appends, which + // is what keeps ` ` the contract every service + // script is written against. + let mode = service.by_mode().mode_argument(mode); + let with_mode = |command: &Command| match mode { + None => command.clone(), + Some(mode) => { + let mut words: Vec = command.to_words(); + words.push(mode.to_string()); + Command::from_words(words) + } + }; + match service { + Service::Script(_) => Some(with_mode(command)), + Service::Managed(_) => Some(Command::from_words(vec![ crate::shipped::service_default() .to_string_lossy() .into_owned(), "--name".to_string(), unit_name(key, component.name()), "--exec".to_string(), - run.to_line(), + with_mode(command).to_line(), ])), - // Declared empty, which says the same as not declaring it. - Service::Script(_) | Service::Managed(_) => None, } } @@ -391,19 +414,64 @@ mod tests { use std::os::unix::fs::PermissionsExt; use super::*; + use crate::config::ByMode; fn server(service: Option) -> Component { + server_with_modes(Vec::new(), service) + } + + fn server_with_modes(modes: Vec, service: Option) -> Component { Component::Server { name: "backend".to_string(), - build: Command::default(), + modes, + build: ByMode::One(Command::default()), cwd: None, stale_when: None, also_watch: Vec::new(), service, + mode: None, built_from: None, + built_mode: None, } } + /// Which binary a managed service runs is exactly what a mode + /// changes, and this is the single place that gets decided -- a + /// second one would build `release` and start `debug`, from a card + /// reporting the mode it was asked for. + #[test] + fn the_mode_decides_which_binary_the_service_runs() { + let mut component = server_with_modes( + vec!["release".to_string(), "debug".to_string()], + Some(Service::Managed(ByMode::Modes(vec![ + ( + "release".to_string(), + Command::from_line("target/release/backend"), + ), + ( + "debug".to_string(), + Command::from_line("target/debug/backend"), + ), + ]))), + ); + + let exec_of = |component: &Component| { + let words = driver("app", component).expect("a command to drive it"); + let (_, args) = words.split_first().expect("the script and its arguments"); + let at = args + .iter() + .position(|arg| arg == "--exec") + .expect("--exec is how the built-in script is told what to run"); + args[at + 1].clone() + }; + + // Nobody has chosen, so it is the first declared -- which is what + // makes declaration order worth getting right. + assert_eq!(exec_of(&component), "target/release/backend"); + component.choose(Some("debug".to_string()), None); + assert_eq!(exec_of(&component), "target/debug/backend"); + } + /// The one place the two variants become the same thing, so this is /// where it is worth pinning down what each turns into. #[test] @@ -412,7 +480,10 @@ mod tests { // already the thing the contract describes. let own = Command::from_line("server/service"); assert_eq!( - driver("app", &server(Some(Service::Script(own.clone())))), + driver( + "app", + &server(Some(Service::Script(ByMode::One(own.clone())))) + ), Some(own) ); @@ -421,9 +492,9 @@ mod tests { // appends still lands last. let managed = driver( "app", - &server(Some(Service::Managed(Command::from_line( + &server(Some(Service::Managed(ByMode::One(Command::from_line( "target/release/ai-server --port 8080", - )))), + ))))), ) .expect("a managed component has a driver"); let (program, arguments) = managed.split_first().expect("a program"); @@ -452,11 +523,17 @@ mod tests { fn nothing_to_drive_is_none_however_it_was_said() { assert_eq!(driver("app", &server(None)), None); assert_eq!( - driver("app", &server(Some(Service::Script(Command::default())))), + driver( + "app", + &server(Some(Service::Script(ByMode::One(Command::default())))) + ), None ); assert_eq!( - driver("app", &server(Some(Service::Managed(Command::default())))), + driver( + "app", + &server(Some(Service::Managed(ByMode::One(Command::default())))) + ), None ); } diff --git a/test-projects/service-and-app/.dev-updater.ron b/test-projects/service-and-app/.dev-updater.ron index c58b3ab..cca5e64 100644 --- a/test-projects/service-and-app/.dev-updater.ron +++ b/test-projects/service-and-app/.dev-updater.ron @@ -20,6 +20,13 @@ resources: Ron("resources.ron"), components: [ Server( name: "daemon", + // Two modes, declared once. Both commands are written *once* + // and handed the mode as their last argument, which is the + // ordinary way -- Dev Updater's own declaration covers the other + // way, where a command cannot take the word and is written per + // mode instead. Between the two fixtures both paths are + // exercised. `release` first, since the first is the default. + modes: ["release", "debug"], build: "./build-daemon.sh", // Managed, so Dev Updater's own service script drives it and this // project needs no systemd or OpenRC knowledge of its own. The diff --git a/test-projects/service-and-app/build-daemon.sh b/test-projects/service-and-app/build-daemon.sh index d60a3e3..c797d27 100755 --- a/test-projects/service-and-app/build-daemon.sh +++ b/test-projects/service-and-app/build-daemon.sh @@ -6,16 +6,31 @@ # seconds. A card with one instant component and one slow one is how you see # whether the per-component timings and the concurrent build really are per # component. +# +# It takes the build mode as its one argument, so that this fixture +# exercises a component with more than one way of being built. `release` +# is deliberately the slow one, which is both what a real optimised build +# is and what makes a mode switch visible on the card: a bar that takes +# eight seconds and one that takes two. set -eu -STEPS=8 +MODE="${1:-release}" +case "$MODE" in +release) STEPS=8 ;; +debug) STEPS=2 ;; +*) + echo "unknown build mode: $MODE (expected release or debug)" >&2 + exit 2 + ;; +esac + i=0 while [ "$i" -lt "$STEPS" ]; do i=$((i + 1)) echo "@@progress $i/$STEPS" - echo "==> Pretending to compile part $i of $STEPS" + echo "==> Pretending to compile part $i of $STEPS ($MODE)" sleep 1 done chmod +x ./serve.sh -echo "==> Daemon ready" +echo "==> Daemon ready ($MODE)" diff --git a/test-projects/service-and-app/serve.sh b/test-projects/service-and-app/serve.sh index 1b70a4d..9c02c64 100755 --- a/test-projects/service-and-app/serve.sh +++ b/test-projects/service-and-app/serve.sh @@ -7,12 +7,27 @@ # that the directory exists to be shown in the dialog. Its *log* is not # written here: a managed service's output is captured by Dev Updater's # service script, which is what the runtime log tab reads. +# +# Takes the build mode as its one argument, ahead of whatever subcommand +# the service contract appends -- which is how a command written once +# learns which mode it is running in. It ticks at a different rate in +# each, so that a mode switch is checkable from a phone: the runtime log +# is the only place the difference shows. set -eu +MODE="${1:-release}" +case "$MODE" in +release) INTERVAL=10 ;; +debug) INTERVAL=2 ;; +*) + echo "unknown mode: $MODE (expected release or debug)" >&2 + exit 2 + ;; +esac DATA="${XDG_DATA_HOME:-$HOME/.local/share}/dutest-service" mkdir -p "$DATA" -echo "started at $(date -Is), writing to $DATA" +echo "started at $(date -Is), writing to $DATA, ticking every ${INTERVAL}s in $MODE" count=0 while true; do count=$((count + 1)) @@ -20,5 +35,5 @@ while true; do # Colour, so the runtime log tab has escapes to render too -- the same # reason breakable/build.sh writes them. printf 'tick \033[1;32m%s\033[0m -- still here\n' "$count" - sleep 10 + sleep "$INTERVAL" done