diff --git a/AGENTS.md b/AGENTS.md index 7cff35a..56291d0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -587,6 +587,34 @@ mutable at runtime from the phone. differ in nothing but the label and the old one said "Pulling and building" for all three. +- **"This server has never built it" is something to build, not + something to sit on** -- and getting that backwards made it the one + state nothing could clear. `component_is_stale` treated a missing + `built_from` as no evidence of staleness, so Update built nothing, so + no build ever wrote the record: a card reading "not built here" with + Pull, Update and the project's own button all doing nothing about it, + for ever. Worst for a `Server`, which the output check beside it + deliberately skips (it has no APK to look for), so nothing else could + rescue it. Measured on a two-component fixture rather than argued: + `/prepare` answered `components: []` and the freshness was unchanged + afterwards; with the fix the same call reports `step: "building"` and + the card reads `current`. + So `nothing_built` now asks the record as well as the output, and the + cost is one rebuild per component on a machine whose records were lost + -- which was the objection when it was written the other way, and is + the right price now that the state is *visible*. It is self-clearing: + the first build writes the record and the question is never asked + again. + Which leaves `NotBuiltHere` meaning exactly one thing: a component with + **no build command**, where nothing here was ever going to measure + anything and no button would change it. That is the whole difference + between it and `NeverBuilt` -- one is actionable and flagged, the other + is quiet, and they were one word covering both. + `nothing_built` takes the record map rather than reading it: a `lock()` + temporary inside a call's argument list stays held for the whole call, + and a test helper doing exactly that deadlocked the moment anything + under it wanted the same lock. + - **Freshness says *which* answer it is, and only three of them read as out of date.** `Freshness` was `current | behind | unknown`, and the card drew unknown as nothing at all -- so a component this server had diff --git a/server/src/build_state.rs b/server/src/build_state.rs index a2efdf1..6ea04fa 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -67,9 +67,14 @@ pub enum Freshness { /// the comparison unreliable. Somebody is editing there and building /// it themselves. Uncommitted, - /// Cannot tell: this server has never built it, so there is no commit - /// to compare the build on disk against. An ordinary state for a - /// project somebody builds by hand. + /// Cannot tell, and nothing here can change that: this component has + /// no build command, so nothing on this machine was ever going to + /// produce a commit to compare against. The ordinary state for a + /// project somebody builds by hand, and deliberately *not* actionable + /// -- there is no button on the row that would clear it, which is the + /// whole difference between it and `NeverBuilt`. A component that + /// does have a command and no record is `NeverBuilt` instead, because + /// building it is exactly what fixes it. NotBuiltHere, /// Cannot tell: no checkout under this project, or git would not say. NoCheckout, @@ -627,17 +632,20 @@ impl BuildState { // one function, because the two answering it differently is what // produced that: the staleness rules knew there was no build and // the card did not. - if self.nothing_built(component) { + // One read of the record, handed to both questions below. Taken + // as a whole statement rather than inline, because a temporary + // guard in a larger expression stays locked for that expression + // -- which is how a helper that passed `inner.lock()...clone()` + // straight into `component_is_stale` deadlocked the moment + // anything under it wanted the same lock. + let built_from = self.inner.lock().unwrap().built_from.clone(); + if self.nothing_built(component, &built_from) { return Freshness::NeverBuilt; } - let built = self - .inner - .lock() - .unwrap() - .built_from - .get(component.name()) - .cloned(); - let Some(built) = built else { + // Only reachable for a component with no build command: + // `nothing_built` above has already answered for every component + // that has one, and a missing record is `NeverBuilt` there. + let Some(built) = built_from.get(component.name()) else { return Freshness::NotBuiltHere; }; match crate::git::subtree_dirty(&self.project_path, &component.watched_paths()) { @@ -646,7 +654,7 @@ impl BuildState { None => return Freshness::NoCheckout, } match crate::git::subtree_head(&self.project_path, &component.watched_paths()) { - Some(current) if current == built => Freshness::Current, + Some(current) if ¤t == built => Freshness::Current, Some(_) if parked => Freshness::Parked, Some(_) => Freshness::Behind, None => Freshness::NoCheckout, @@ -679,9 +687,32 @@ impl BuildState { /// this server builds: with no command there is nothing to press, so /// saying it out loud would be a nag about a project somebody builds /// by hand. - fn nothing_built(&self, component: &Component) -> bool { - !component.build().is_empty() - && matches!(component, Component::Apk { .. }) + fn nothing_built(&self, component: &Component, built_from: &HashMap) -> bool { + if component.build().is_empty() { + return false; + } + // No commit recorded means no build of this component has ever + // finished here, whatever is or is not sitting on disk. It is the + // only way to ask a `Server`, which has no output to look for -- + // and it used to be asked of nothing at all, which made it the + // one state Update could not clear: with no record the component + // was not stale, so no build ran, so no record was written. The + // card said "not built here" and every button that should have + // fixed it did nothing, for ever. Measured on a two-component + // fixture: `/prepare` answered with an empty component list and + // the freshness was unchanged afterwards. + // + // Self-clearing, which is what makes it safe to act on: the first + // build writes the record and the question is never asked again. + // The cost is one rebuild per component on a machine whose + // records were lost, which is the price of every later reading + // being something this server measured rather than assumed. + if !built_from.contains_key(component.name()) { + return true; + } + // And for an APK, the output itself: a build recorded a commit and + // somebody deleted what it produced. + matches!(component, Component::Apk { .. }) && crate::discover::find_apks(&component.dir(&self.project_path)).is_empty() } @@ -709,7 +740,7 @@ impl BuildState { // 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. - if self.nothing_built(component) { + if self.nothing_built(component, built_from) { return true; } // Behind the checkout: this component was built from a commit @@ -1991,7 +2022,11 @@ mod tests { .iter() .find(|component| component.name() == name) .expect("component"); - state.component_is_stale(component, &state.inner.lock().unwrap().built_from.clone()) + // Bound first: a `lock()` temporary inside the call's argument + // list stays held for the whole call, which deadlocks anything + // under it that wants the same lock. + let built_from = state.inner.lock().unwrap().built_from.clone(); + state.component_is_stale(component, &built_from) } /// The trap a per-component staleness check walks straight into once @@ -2129,8 +2164,10 @@ mod tests { assert_eq!( state.freshness(app, false), - Freshness::NotBuiltHere, - "no build of ours to compare, which is an ordinary state for a project built by hand", + Freshness::NeverBuilt, + "this server has built nothing for it, which is a first build to make rather than a \ + comparison to draw -- `NotBuiltHere` is now only for a component with no build \ + command at all, where there is nothing to press", ); state @@ -2147,6 +2184,26 @@ mod tests { Freshness::Uncommitted, "somebody is editing in there, so the commit comparison says nothing", ); + + // The other kind of not knowing, and the one that stays quiet: + // nothing here builds this component, so there is no button that + // would ever change the answer and nothing to flag. + let by_hand = Component::Apk { + name: "by-hand".to_string(), + modes: Vec::new(), + build: crate::config::ByMode::default(), + 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, + }; + assert_eq!(state.freshness(&by_hand, false), Freshness::NotBuiltHere); } /// Moving the checkout runs git and nothing else. @@ -2427,11 +2484,25 @@ mod tests { ); } - /// Nothing recorded is "we have never built this", not "this is out of - /// date" -- otherwise every project that existed before this feature - /// would demand a rebuild the moment it arrived. + /// Nothing recorded means this server has never built the component, + /// and that is something to build rather than something to sit on. + /// + /// It was the other way round, on the grounds that every project + /// predating the record would demand one rebuild when the feature + /// arrived. The cost of that reading only showed up once freshness + /// started saying which kind of unknown it was: with no record a + /// component is not stale, so Update builds nothing, so no record is + /// ever written -- a card reading "not built here" with every button + /// that should fix it doing nothing, for ever. A `Server` cannot even + /// be rescued by the output check beside this one, having no output + /// to look for. + /// + /// So the one-time rebuild is the price, and it buys the thing that + /// makes every later reading worth anything: a commit this server + /// measured rather than assumed. It is self-clearing -- the first + /// build writes the record and the question is never asked again. #[test] - fn a_component_this_server_has_never_built_is_not_called_stale() { + fn a_component_this_server_has_never_built_is_built_once() { let dir = tempfile::tempdir().expect("tempdir"); let root = dir.path(); let components = two_component_checkout(root); @@ -2441,8 +2512,20 @@ mod tests { state.inner.lock().unwrap().built_from.is_empty(), "nothing recorded" ); - assert!(!stale(&state, "backend")); - assert!(!stale(&state, "app")); + assert!(stale(&state, "backend"), "a server has no output to check"); + assert!(stale(&state, "app")); + + let head = crate::git::subtree_head(root, &[PathBuf::from("server")]).expect("a commit"); + state + .inner + .lock() + .unwrap() + .built_from + .insert("backend".to_string(), head); + assert!( + !stale(&state, "backend"), + "and one build settles it, or the rebuild would repeat for ever", + ); } /// The same must hold where there is no checkout to compare against: