diff --git a/AGENTS.md b/AGENTS.md index 66eb613..0995e37 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -467,6 +467,42 @@ mutable at runtime from the phone. builds a component has, a reused name is not the same APK, and handing it the old one's package would be wrong until that component happened to be downloaded. +- **`/prepare` and `/build` take `?component=` to build one component + instead of every one with a command.** Without it, pressing Update on + one client of a multi-client project ran every component's build to + get the one that was actually asked for -- fine when a project had one + APK, expensive the moment it had two and one of them was slow. + `named_component` in `routes.rs` is the one place a name from the phone + is checked against the project's own components, so `/prepare` and + `/build` cannot disagree about what an unknown name means, and + `BuildState::{trigger_if_needed,build_now,run_build,is_stale}` all take + the same `Option<&str>` -- `None` still means the whole project, which + is what `Pull & Build`, the project-row `Rebuild`, and every project + with a single component keep doing. There is deliberately no + component-scoped Rebuild: forcing one component's build without + touching the rest happens by pressing Update on it, which runs + `/prepare` scoped to that component. + **Where `component.dir()` belongs is now one definition** + (`Component::dir` in `config.rs`), because a second one very nearly + shipped a real bug: `component_is_stale`'s "never built at all" check + still asked `find_apks` of the *project root* after per-component + discovery had already moved everywhere else to `component.dir()`. A + project with two `Apk` components has one's output sitting under the + root-anchored patterns too -- `*/build/outputs/apk/*/*.apk` matches any + one-level subdirectory, regardless of which component put it there -- + so the moment *either* component had ever been built, the whole + project read as "something is built here," and the *other* component, + never built, silently stopped being offered its own first build: + `prepare` saw a component with a command and no output and declared it + current. Caught by testing the actual behaviour of a two-APK project + rather than trusting that scoping the build implied scoping the + staleness check that decides whether to run it -- they are two + different reads of "which directory is this component's," and only one + of them had been moved. + The same check is deliberately *not* asked of a `Server`: a service + never has an APK to find under its own directory by definition, so + asking would report every server "never built" forever. Guarded on + `matches!(component, Component::Apk { .. })` for that reason. - **The self entry's project is the working directory itself**, so this server has to be started from the root of its own checkout -- which is where everything else here is driven from, and what the service unit diff --git a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt index 36c00b6..825fabe 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/BuildStatus.kt @@ -1,5 +1,6 @@ package com.example.devupdater +import java.net.URLEncoder import org.json.JSONObject // The three routes that act on the *build machine* rather than this @@ -112,14 +113,32 @@ private fun requestBuildStatus(path: String, method: String): BuildStatus = fun pullAndBuild(key: String, force: Boolean = false): BuildStatus = requestBuildStatus("/apps/$key/pull" + if (force) "?force=true" else "", "POST") -fun prepareBuild(key: String): BuildStatus = requestBuildStatus("/apps/$key/prepare", "POST") +/** + * Builds one component if its staleness rule says so, before it is downloaded. + * + * Scoped to that one component rather than the whole project: a project with more than one -- two + * independent Android clients sharing a checkout, say -- would otherwise pay for every component's + * build to get the one that was actually pressed, which for a slow one is the whole complaint. The + * server still only runs a component's command when *that* component is behind, same as before; + * naming it just keeps the others out of the run entirely. + */ +fun prepareBuild(key: String, component: String): BuildStatus = + requestBuildStatus( + "/apps/$key/prepare?component=${URLEncoder.encode(component, "UTF-8")}", + "POST", + ) /** - * Builds because the person asked, not because anything looked stale. + * Builds every component, because the person asked, not because anything looked stale. * * The staleness rules keep a download from rebuilding the world; they have no business overruling a * button. This is also the only way a project already current with its checkout ever records what * it was built from, which is what the "out of date" signal is compared against. + * + * Whole-project on purpose, unlike [prepareBuild]: this is the project row's Rebuild, which sits + * beside Pull & Build and means the same "the whole checkout" that one does. Forcing just one + * component's build without touching the rest happens by pressing Update on that component, which + * runs [prepareBuild] scoped to it -- there is no second, component-scoped Rebuild. */ fun buildNow(key: String): BuildStatus = requestBuildStatus("/apps/$key/build", "POST") 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 0cf933b..df2c9f2 100644 --- a/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/devupdater/UpdaterScreen.kt @@ -720,7 +720,7 @@ private fun AppListScreen( if (entry.needsBuild) { cardStates = cardStates + (entry.key to CardState.Preparing(null)) try { - var status = withContext(Dispatchers.IO) { prepareBuild(entry.key) } + var status = withContext(Dispatchers.IO) { prepareBuild(entry.key, component) } while (status.building) { cardStates = cardStates + (entry.key to CardState.Preparing(status)) delay(BUILD_POLL_INTERVAL_MS) diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 9243693..6a5464c 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -371,10 +371,16 @@ impl BuildState { /// Idempotent: kicks off the build in the background if this app is /// stale and no build is already running, a no-op otherwise. Safe for /// the phone to call on every download, which is exactly what it does. - pub fn trigger_if_needed(self: &Arc, record: RecordBuilt) { + /// + /// `component` narrows both the staleness check and the build itself + /// to one component -- what the phone asks before downloading it, so + /// that pressing Update on one client of a multi-client project + /// doesn't also pay for the other's build. `None` is every other + /// caller, which still means "the whole project". + pub fn trigger_if_needed(self: &Arc, component: Option<&str>, record: RecordBuilt) { // Before the lock, not inside it -- see `is_stale`. - if self.is_stale() { - self.build_now(record); + if self.is_stale(component) { + self.build_now(component, record); } } @@ -387,8 +393,17 @@ impl BuildState { /// that runs here, so a project already current with its checkout /// could never record one and would report unknown for ever. /// - /// Still idempotent -- while one run is going, another does nothing. - pub fn build_now(self: &Arc, record: RecordBuilt) { + /// Still idempotent -- while one run is going, another does nothing, + /// whichever component either names: there is one build slot per + /// project, not one per component, so a second request while the + /// first is still running is a no-op rather than a second concurrent + /// build. The phone notices by polling `/status` and re-reads once it + /// clears. + /// + /// `component` restricts the run to one named component, or every one + /// with a command for `None` -- see [`Self::trigger_if_needed`] for + /// why a caller would want the former. + pub fn build_now(self: &Arc, component: Option<&str>, record: RecordBuilt) { { let mut inner = self.inner.lock().unwrap(); if inner.building { @@ -403,16 +418,17 @@ impl BuildState { } let this = Arc::clone(self); + let component = component.map(str::to_string); tokio::task::spawn_blocking(move || { - this.run_build(&record); + this.run_build(component.as_deref(), &record); }); } - /// Whether any component is behind. Per project, because a run is per - /// project: one component being stale is reason enough to walk the - /// whole list, and each component's own command is skipped if it has - /// nothing to do. - fn is_stale(&self) -> bool { + /// Whether any component `name` selects is behind. Whole project for + /// `None`, which is what a run with nothing named means: one + /// component being stale is reason enough to walk the whole list, and + /// each component's own command is skipped if it has nothing to do. + fn is_stale(&self, name: Option<&str>) -> bool { // Read once, here, and passed down. `component_is_stale` must not // take this lock itself: it is reached from callers that already // hold it, and a `std::sync::Mutex` is not reentrant, so doing so @@ -420,6 +436,7 @@ impl BuildState { let built_from = self.inner.lock().unwrap().built_from.clone(); self.components .iter() + .filter(|component| name.is_none_or(|name| component.name() == name)) .any(|component| self.component_is_stale(component, &built_from)) } @@ -464,11 +481,27 @@ impl BuildState { } // 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 on a project that has - // never been built it reports not-stale and would leave the first - // build impossible to trigger -- which a project can be added - // before having done. - if crate::discover::find_apks(&self.project_path).is_empty() { + // compares two files *inside* a build, so a component that has + // never been built reports not-stale under it and would leave the + // first build impossible to trigger -- which a project can be + // added before having done. + // + // Scoped to *this component's own* directory, not the project + // root: a project producing two APKs has one component's output + // sitting under the root-anchored patterns too (an inner + // `*/build/outputs/apk/*/*.apk` matches a one-level subdirectory + // regardless of which component it belongs to), which made the + // whole project read as "something is built here" the moment + // either component had ever been built -- masking that the + // *other* component, never built, had nothing to trigger it. + // + // Only asked of an `Apk`: a `Server` never has one to find under + // its own directory by definition, so the same question asked of + // it would report every server "never built" for ever, which is + // exactly the false staleness this check exists to rule out. + if matches!(component, Component::Apk { .. }) + && crate::discover::find_apks(&component.dir(&self.project_path)).is_empty() + { return true; } // Behind the checkout: this component was built from a commit @@ -559,10 +592,10 @@ impl BuildState { // `may_build()` is called here, with the pulled // declaration on disk, for the reason in the doc // comment above. - if !may_build() || !this.has_command() || !(pulled || this.is_stale()) { + if !may_build() || !this.has_command() || !(pulled || this.is_stale(None)) { this.finish(None, None); } else { - this.run_build(&record); + this.run_build(None, &record); } } } @@ -591,31 +624,31 @@ impl BuildState { Ok(true) } - /// Walks the components in declared order, stopping at the first - /// failure. + /// Builds every component `name` selects, at once. /// - /// Stopping is the point of the order: a project whose server must be - /// current before its APK is served declares them that way round, and a - /// failed server build must not be followed by an APK build that would - /// hand the phone half of a matched pair. What already succeeded stays - /// -- there is nothing to roll back a build to. + /// Every one of them together rather than in turn -- they are + /// independent, a Rust build and a Gradle build share nothing but the + /// machine, and measured on this one, running them together takes + /// about three quarters of the time running them in turn does. The + /// saving only appears when more than one has work to do, which is + /// the case a pull produces; `name` is how a caller that wants only + /// one opts out of paying for the others (see + /// [`Self::trigger_if_needed`]) -- `None` still means all of them. + /// + /// What running them together costs is that a failure no longer stops + /// the others: they are already running by the time it happens, so + /// stopping them would mean killing work that is probably fine, and + /// the first failure in declaration order is the one reported, which + /// is what a walk in that order would have said. /// /// Each component's name is the phase name, so the card says which one /// is being worked on without needing anything new to carry it. - fn run_build(self: &Arc, record: &RecordBuilt) { - // Every component at once. They are independent -- a Rust build and - // a Gradle build share nothing but the machine -- and measured on - // this one, running them together takes about three quarters of the - // time running them in turn does. The saving only appears when more - // than one has work to do, which is the case a pull produces. - // - // What it costs is that a failure no longer stops the others. They - // are already running by the time it happens, so stopping them - // would mean killing work that is probably fine; the first failure - // in declaration order is the one reported, which is what a walk in - // that order would have said. + fn run_build(self: &Arc, name: Option<&str>, record: &RecordBuilt) { let mut running = Vec::new(); for (index, component) in self.components.iter().enumerate() { + if name.is_some_and(|name| component.name() != name) { + continue; + } if component.build().is_empty() { continue; } @@ -1065,7 +1098,7 @@ impl BuildState { pub fn status(&self) -> BuildStatus { // Before the lock, not inside it -- see `is_stale`. - let stale = self.is_stale(); + let stale = self.is_stale(None); let inner = self.inner.lock().unwrap(); BuildStatus { stale, @@ -1279,6 +1312,138 @@ mod tests { ); } + /// Naming one component builds only that one. + /// + /// The case a project with more than one component exists to avoid + /// paying for: pressing Update on one client of a two-client project + /// must not also run the other's build, which for a slow one (an ARM + /// cross-compile, say) is the entire complaint a phone would have. + #[tokio::test] + async fn naming_a_component_builds_only_that_one() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("server")).expect("mkdir"); + std::fs::create_dir_all(root.join("app")).expect("mkdir"); + let components = vec![ + Component::Server { + name: "backend".to_string(), + build: crate::config::Command::from_line("touch backend-built"), + cwd: Some(PathBuf::from("server")), + stale_when: None, + service: None, + built_from: None, + }, + Component::Apk { + name: "app".to_string(), + build: crate::config::Command::from_line("touch app-built"), + cwd: Some(PathBuf::from("app")), + stale_when: None, + strip: false, + package: None, + built_from: None, + }, + ]; + let state = state_for(root, components); + + state.build_now(Some("app"), Arc::new(|_: &str, _: String| {})); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while state.status().building { + assert!( + std::time::Instant::now() < deadline, + "build did not finish in time" + ); + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + + assert!( + root.join("app/app-built").exists(), + "the named component ran" + ); + assert!( + !root.join("server/backend-built").exists(), + "naming one component must not build the other", + ); + + let status = state.status(); + assert_eq!( + status + .components + .iter() + .map(|c| c.name.as_str()) + .collect::>(), + vec!["app"], + "the status only reports the component that actually ran", + ); + } + + /// A sibling that has already been built must not mask that this one + /// never has. + /// + /// The bug the previous test's fix uncovered: `APK_PATTERNS` includes + /// a one-level pattern (`*/build/outputs/apk/*/*.apk`), so a second + /// component's output sitting one directory below the project root + /// satisfies a scan of the *root* even when it belongs to a component + /// nobody has ever built here. `component_is_stale`'s "never built" + /// check used to run against the project root rather than the + /// component's own directory, so the moment either of two components + /// had been built once, the other quietly stopped being offered its + /// own first build -- `prepare` (what Update runs before a download) + /// saw a component with a command and no output and declared it + /// current anyway. + #[tokio::test] + async fn a_sibling_already_built_does_not_hide_that_this_one_never_has() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + std::fs::create_dir_all(root.join("a")).expect("mkdir"); + std::fs::create_dir_all(root.join("b")).expect("mkdir"); + let components = vec![ + Component::Apk { + name: "a".to_string(), + build: crate::config::Command::from_line("touch a-built"), + cwd: Some(PathBuf::from("a")), + stale_when: None, + strip: false, + package: None, + built_from: None, + }, + Component::Apk { + name: "b".to_string(), + build: crate::config::Command::from_line("touch b-built"), + cwd: Some(PathBuf::from("b")), + stale_when: None, + strip: false, + package: None, + built_from: None, + }, + ]; + let state = state_for(root, components); + + // `a` has an APK on disk already -- built by something other than + // this state, which is the ordinary case for "added after the + // fact" -- while `b` has never been built at all. + std::fs::create_dir_all(root.join("a/build/outputs/apk/debug")).expect("mkdir"); + std::fs::write(root.join("a/build/outputs/apk/debug/a.apk"), b"").expect("write"); + + assert!( + stale(&state, "b"), + "b has never been built, regardless of what a has on disk", + ); + + state.trigger_if_needed(Some("b"), Arc::new(|_: &str, _: String| {})); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while state.status().building { + assert!( + std::time::Instant::now() < deadline, + "build did not finish in time" + ); + tokio::time::sleep(std::time::Duration::from_millis(10)).await; + } + assert!( + root.join("b/b-built").exists(), + "prepare must have run b's build" + ); + } + fn state_for(root: &Path, components: Vec) -> Arc { BuildState::new( "test".to_string(), diff --git a/server/src/config.rs b/server/src/config.rs index 3a4d371..f7744be 100644 --- a/server/src/config.rs +++ b/server/src/config.rs @@ -323,6 +323,22 @@ impl Component { } } + /// Where this component's own files are, resolved against the + /// project root: `project.join(cwd)`, or `project` itself for a + /// component that names none. + /// + /// The one definition of what `cwd` resolves to, so a component's + /// build command, its staleness check, its discovered builds and its + /// recorded commit can never resolve it four different ways. `join` + /// on an absolute `cwd` yields that path, so a config may give either + /// form. + pub fn dir(&self, project: &Path) -> PathBuf { + match self.cwd() { + Some(cwd) => project.join(cwd), + None => project.to_path_buf(), + } + } + pub fn stale_when(&self) -> Option<&StaleRule> { match self { Self::Apk { stale_when, .. } | Self::Server { stale_when, .. } => stale_when.as_ref(), diff --git a/server/src/registry.rs b/server/src/registry.rs index 6d37bc7..d8f2e98 100644 --- a/server/src/registry.rs +++ b/server/src/registry.rs @@ -109,12 +109,7 @@ impl AppEntry { /// client's build is reachable and each component's builds are its /// own. pub fn component_dir(&self, component: &Component) -> PathBuf { - match component.cwd() { - // `join` on an absolute path yields that path, as everywhere - // else a cwd is resolved. - Some(cwd) => self.project_path.join(cwd), - None => self.project_path.clone(), - } + component.dir(&self.project_path) } /// The APK to serve: `requested` if it is still one of this project's diff --git a/server/src/routes.rs b/server/src/routes.rs index fce6fc1..38f993b 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -14,7 +14,9 @@ //! POST /apps/{key}/pull fetch, fast-forward, and build //! ?force=true resets onto the upstream //! POST /apps/{key}/prepare run the on-demand build step, if any +//! ?component= restricts it to one //! POST /apps/{key}/build run it whether or not it looks stale +//! ?component= restricts it to one //! POST /apps/{key}/approve accept the build step it asks for //! POST /apps/{key}/recheck ask that one project's remote and //! services again @@ -1198,6 +1200,46 @@ fn lookup(state: &AppState, key: Option>) -> Result, +} + +/// Resolves a `?component=` against the project's own components, or says +/// why it could not -- the one place both build routes turn a name from +/// the phone into something [`crate::build_state::BuildState`] accepts, so +/// they cannot disagree about what an unknown name means. +fn named_component<'a>( + entry: &'a AppEntry, + named: Option<&'a str>, +) -> Result, ApiError> { + match named { + Some(name) + if entry + .components + .iter() + .any(|component| component.name() == name) => + { + Ok(Some(name)) + } + Some(name) => Err(ApiError::UnknownComponent( + entry.label.clone(), + name.to_string(), + )), + None => Ok(None), + } +} + /// The callback a build reports each finished component through. /// /// Made here because this is where both halves are in scope: the app list @@ -1213,6 +1255,7 @@ fn records_builds(state: &Arc, key: &str) -> crate::build_state::Recor async fn build_prepare( State(state): State>, key: UrlPath, + Query(query): Query, ) -> Result, ApiError> { let entry = lookup(&state, Some(key))?; // Checked here rather than trusted from the entry's build state: the @@ -1225,11 +1268,12 @@ async fn build_prepare( entry.label, ))); } + let component = named_component(&entry, query.component.as_deref())?; let build = entry .build .as_ref() .ok_or_else(|| ApiError::NoBuildStep(entry.label.clone()))?; - build.trigger_if_needed(records_builds(&state, &entry.key)); + build.trigger_if_needed(component, records_builds(&state, &entry.key)); Ok(Json(build.status())) } @@ -1244,6 +1288,7 @@ async fn build_prepare( async fn build_now( State(state): State>, key: UrlPath, + Query(query): Query, ) -> Result, ApiError> { let entry = lookup(&state, Some(key))?; if entry.pending_declaration().is_some() { @@ -1253,11 +1298,12 @@ async fn build_now( entry.label, ))); } + let component = named_component(&entry, query.component.as_deref())?; let build = entry .build .as_ref() .ok_or_else(|| ApiError::NoBuildStep(entry.label.clone()))?; - build.build_now(records_builds(&state, &entry.key)); + build.build_now(component, records_builds(&state, &entry.key)); Ok(Json(build.status())) }