A component with nothing built is not current with the checkout
`freshness` compared two commits and never asked whether there was an output at all, so a component built once and then cleaned reported the commit it was last built from and read as `current`: the card said nothing, filed it under "up to date", and offered Rebuild for a build that did not exist. Found on test-projects/two-clients, whose tablet component had a recorded builtFrom and no APK under it. The staleness rules already knew -- `component_is_stale` has asked `find_apks` under the component's own directory since per-component discovery landed -- so the two readers of "is this build current" disagreed, which is the shape worth removing rather than the one wrong answer. Both now go through `nothing_built`. Only for an Apk, since a Server has none to find, and only for a component this server builds, since with no command there is nothing to press and saying it would be a nag about a project built by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
25c069909b
commit
9efe08e4ce
1 file changed
+87
-17
+87
-17
@@ -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
|
||||
|
||||
Reference in new issue
Block a user