From 7eaf79370e616a1edbcc12f4d77b3722b2a8d4aa Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Tue, 1 Sep 2026 00:28:36 -0400 Subject: [PATCH] Build one component instead of every one with a command Pressing Update on one client of a multi-client project ran every component's build to get the one that was actually asked for -- cheap while a project had one APK, expensive the moment it had two and one of them was slow (an ARM cross-compile, say). /prepare and /build now take ?component= to restrict a run to one named component; absent still means the whole project, which is what Pull & Build, the project-row Rebuild, and every single-component project 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 now runs /prepare scoped to that component. Verified against a live server driving a scratch two-Apk project: building one component leaves the other's marker untouched, an unknown ?component= answers 404 naming the project and the component, and naming none still builds both. That verification surfaced a second, sharper bug the scoping change had not caused but did make newly visible: component_is_stale's "never built at all" check still asked find_apks of the whole project root, left behind when per-component discovery (c4e19c2) moved everywhere else to each component's own directory. A project with two Apk components has one's output sitting under the root-anchored patterns too, so the moment either component had ever been built, the whole project read as "something is built here" -- and the other, never built, silently stopped being offered its own first build. /prepare saw a component with a command and no output and declared it current. Fixed by scoping the same check to the component's own directory, guarded to Apk components only: a Server never has an APK to find under its directory by definition, and asking would have reported every server "never built" forever, which broke two existing tests before the guard was added. Component::dir is now the one definition of what a component's directory is, used by the build command, the staleness check, and discovery alike. Co-Authored-By: Claude Sonnet 5 --- AGENTS.md | 36 +++ .../com/example/devupdater/BuildStatus.kt | 23 +- .../com/example/devupdater/UpdaterScreen.kt | 2 +- server/src/build_state.rs | 241 +++++++++++++++--- server/src/config.rs | 16 ++ server/src/registry.rs | 7 +- server/src/routes.rs | 50 +++- 7 files changed, 326 insertions(+), 49 deletions(-) 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())) }