diff --git a/server/src/build_state.rs b/server/src/build_state.rs index 1e48f41..e6a6d51 100644 --- a/server/src/build_state.rs +++ b/server/src/build_state.rs @@ -592,6 +592,18 @@ impl BuildState { if component.built_in_another_mode() { return Freshness::Behind; } + // Nothing built at all, which no commit can express either: the + // comparison below is between two commits, so a component whose + // output was removed -- or never produced on this machine -- + // reports the commit it was last built from and reads as current, + // with the card saying nothing and its button offering to build it + // *again*. Asked here as well as in `component_is_stale` through + // 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) { + return Freshness::Behind; + } let built = self .inner .lock() @@ -614,6 +626,38 @@ impl BuildState { } } + /// Whether this component has no build output at all. + /// + /// The state a commit cannot express, which is why both readers of + /// "is this build current" have to ask it separately from comparing + /// commits: `component_is_stale`, so a first build can be triggered, + /// and `freshness`, so the card does not call a component with nothing + /// on disk up to date. They asked it in one place only for a while, + /// and a component whose APK had been removed after a build read as + /// `current` on the card while the staleness rules knew better -- + /// nothing looked broken, which is what makes that shape expensive. + /// + /// 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. And only of a component + /// 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 { .. }) + && crate::discover::find_apks(&component.dir(&self.project_path)).is_empty() + } + fn component_is_stale( &self, component: &Component, @@ -638,23 +682,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. - // - // 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() - { + if self.nothing_built(component) { return true; } // Behind the checkout: this component was built from a commit @@ -2081,6 +2109,48 @@ mod tests { } } + /// A component with nothing built is not "current with the checkout", + /// however still the commits are. + /// + /// Found on a two-APK test project whose second component had been + /// built once and had its output removed since: the card said nothing, + /// filed it under "up to date", and offered *Rebuild* for a component + /// with no build to re-do. Every commit involved was equal, which is + /// all `freshness` used to look at. + #[test] + fn a_component_with_no_output_is_not_reported_as_current() { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + let components = two_component_checkout(root); + let head = crate::git::subtree_head(root, &[PathBuf::from("app")]).expect("a commit"); + let state = state_for(root, components); + // Built here, at the commit that is still checked out: as current + // as a comparison of commits can make it. + state + .inner + .lock() + .unwrap() + .built_from + .insert("app".to_string(), head); + let app = state + .components + .iter() + .find(|component| component.name() == "app") + .expect("the app component"); + assert_eq!(state.freshness(app, false), Freshness::Current); + + std::fs::remove_file(root.join("app/build/outputs/apk/debug/a.apk")).expect("rm"); + assert_eq!( + state.freshness(app, false), + Freshness::Behind, + "the commits still match, and there is nothing built to match them", + ); + assert!( + stale(&state, "app"), + "and the two readers of this agree, which is the point of asking it once", + ); + } + /// 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