Claiming the whole project means what's stale, not everything built here
after_moving (Pull and Update's shared machinery) gated a post-pull build on `pulled || is_stale(None)` and then, once open, `claim(&mut inner, None)` -- which marks every component with a build command regardless of whether it was the one that moved. `trigger_if_needed(None, ...)` had the same shape one level up: check is_stale, then hand off to build_now, which is unconditional by design. So any pull that fetched anything rebuilt every sibling beside the one that actually changed -- "it tries to update the compose apk even if nothing was done." `claim` now takes `only_stale`: false for build_now (a person pressed a specific button, and staleness has no business overruling that), true for trigger_if_needed and for the build after_moving runs after a pull. component_is_stale was already scoped correctly per component; the bug was building past that answer once any one component tripped it. MismatchedPairNote also had its own word for the same "behind" freshness freshnessNote already calls "out of date" -- "older than this build" was a second name for one measurement, which is what "pulling says something weird about the compose apk" turned out to be. Reused the existing word. Regression test: a_pull_touching_one_component_does_not_rebuild_its_sibling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
663626345a
commit
c41c36f25d
3 files changed
+194
-19
No files matched your search
+161
-17
@@ -480,10 +480,23 @@ impl BuildState {
|
||||
/// 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<Self>, component: Option<&str>, record: RecordBuilt) {
|
||||
// Before the lock, not inside it -- see `is_stale`.
|
||||
if self.is_stale(component) {
|
||||
self.build_now(component, record);
|
||||
}
|
||||
let claimed = {
|
||||
let mut inner = self.inner.lock().unwrap();
|
||||
// The one thing a component still waits behind. See
|
||||
// `Inner::pulling`.
|
||||
if inner.pulling {
|
||||
return;
|
||||
}
|
||||
// `true` is the whole of this method: claim only what is
|
||||
// individually stale, so naming no component still means "the
|
||||
// whole project" while leaving every component already
|
||||
// current alone. Passing `component` through unfiltered here
|
||||
// used to build every component with a command the moment any
|
||||
// one of them was stale -- pressing Update to bring in a
|
||||
// one-line server fix rebuilt the APK beside it every time.
|
||||
self.claim(&mut inner, component, true)
|
||||
};
|
||||
self.start(claimed, record);
|
||||
}
|
||||
|
||||
/// Builds whether or not anything looks stale, for somebody who asked.
|
||||
@@ -514,7 +527,7 @@ impl BuildState {
|
||||
if inner.pulling {
|
||||
return;
|
||||
}
|
||||
self.claim(&mut inner, component)
|
||||
self.claim(&mut inner, component, false)
|
||||
};
|
||||
self.start(claimed, record);
|
||||
}
|
||||
@@ -522,6 +535,15 @@ impl BuildState {
|
||||
/// Marks each component `name` selects that has something to build
|
||||
/// and is not already building, and answers where they are.
|
||||
///
|
||||
/// `only_stale` is the whole difference between a person's button and
|
||||
/// an automatic trigger: `false` for `build_now`, where somebody
|
||||
/// pressed a specific button and every component `name` selects
|
||||
/// starts regardless of what it looks like; `true` for
|
||||
/// `trigger_if_needed`, where naming no component means "the whole
|
||||
/// project" and that must not mean "every component with a command,
|
||||
/// stale or not" -- a pull that only touched one component's
|
||||
/// directory has no business rebuilding the others beside it.
|
||||
///
|
||||
/// Takes the guard rather than the lock so that a caller can do this
|
||||
/// and something else in one step -- which the pull needs: releasing
|
||||
/// `pulling` and claiming what it decided to build have to happen
|
||||
@@ -533,7 +555,7 @@ impl BuildState {
|
||||
/// returns, so a status read in that window would otherwise find the
|
||||
/// component idle -- and idle is exactly what the phone is waiting
|
||||
/// for.
|
||||
fn claim(&self, inner: &mut Inner, name: Option<&str>) -> Vec<usize> {
|
||||
fn claim(&self, inner: &mut Inner, name: Option<&str>, only_stale: bool) -> Vec<usize> {
|
||||
let mut claimed = Vec::new();
|
||||
for (index, component) in self.components.iter().enumerate() {
|
||||
if name.is_some_and(|name| component.name() != name) {
|
||||
@@ -545,6 +567,9 @@ impl BuildState {
|
||||
if inner.component_running(component.name()) {
|
||||
continue;
|
||||
}
|
||||
if only_stale && !self.component_is_stale(component, &inner.built_from) {
|
||||
continue;
|
||||
}
|
||||
// Whatever the last run left against this component goes now,
|
||||
// so nothing from it can be read as this run's outcome -- an
|
||||
// old error in particular, which the phone treats as a reason
|
||||
@@ -899,26 +924,32 @@ impl BuildState {
|
||||
tracing::error!("moving the checkout failed: {}", error.message);
|
||||
this.fail_pull(error);
|
||||
}
|
||||
Ok(pulled) => {
|
||||
// Nothing configured to build, nothing allowed to, or
|
||||
// a move that builds nothing by definition: the move
|
||||
// was the whole job.
|
||||
Ok(_) => {
|
||||
// Nothing configured to build, or nothing allowed to:
|
||||
// the move was the whole job. Which components (if
|
||||
// any) actually build is `claim`'s question, not this
|
||||
// one -- a pull that fetched something is not itself a
|
||||
// reason to rebuild a component whose own directory
|
||||
// the fetch never touched. That used to be folded in
|
||||
// here as `pulled || this.is_stale(None)`, which
|
||||
// meant *any* pull rebuilt *every* component with a
|
||||
// command: pressing Update to bring in a one-line
|
||||
// server fix rebuilt the APK beside it every time,
|
||||
// whether or not `app/` had moved.
|
||||
// `may_build()` is called here, with the pulled
|
||||
// declaration on disk, for the reason in the doc
|
||||
// comment above. Both it and `is_stale` take the lock
|
||||
// themselves, so they are asked before it is held.
|
||||
let build = then_build
|
||||
// comment above.
|
||||
let may_build = then_build
|
||||
.as_ref()
|
||||
.is_some_and(|(may_build, _)| may_build())
|
||||
&& this.has_command()
|
||||
&& (pulled || this.is_stale(None));
|
||||
&& this.has_command();
|
||||
// Handing the run over in one step, so nothing can
|
||||
// observe the moment between the pull ending and the
|
||||
// builds it decided on starting -- see `claim`.
|
||||
let claimed = {
|
||||
let mut inner = this.inner.lock().unwrap();
|
||||
let claimed = match build {
|
||||
true => this.claim(&mut inner, None),
|
||||
let claimed = match may_build {
|
||||
true => this.claim(&mut inner, None, true),
|
||||
false => Vec::new(),
|
||||
};
|
||||
inner.pulling = false;
|
||||
@@ -1679,6 +1710,119 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// Update pulls the whole project, but a commit under only one
|
||||
/// component's directory must not rebuild the other.
|
||||
///
|
||||
/// The bug this guards: `after_moving` used to gate the whole build on
|
||||
/// `pulled || is_stale(None)` and then claim *every* component with a
|
||||
/// command once that gate opened, rather than only the ones
|
||||
/// `component_is_stale` actually flagged. So any pull that fetched
|
||||
/// anything -- a one-line fix to `server/`, say -- rebuilt every
|
||||
/// sibling beside it too, which on a real project meant pressing
|
||||
/// Update to bring in a backend fix also rebuilt the APK for no
|
||||
/// reason. Reported as "it tries to update the compose apk even if
|
||||
/// nothing was done."
|
||||
#[tokio::test]
|
||||
async fn a_pull_touching_one_component_does_not_rebuild_its_sibling() {
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
let origin = dir.path().join("origin");
|
||||
std::fs::create_dir_all(origin.join("server")).expect("mkdir");
|
||||
// Committed rather than left untracked: `pull` refuses to run over
|
||||
// uncommitted changes, and `nothing_built` asks the filesystem for
|
||||
// an Apk's own output regardless of git, so without something
|
||||
// here the "app" component reads as never-built on every round --
|
||||
// stale for a reason that has nothing to do with what this test
|
||||
// is checking.
|
||||
std::fs::create_dir_all(origin.join("app/build/outputs/apk/debug")).expect("mkdir");
|
||||
std::fs::write(origin.join("app/build/outputs/apk/debug/a.apk"), b"").expect("write");
|
||||
std::fs::write(origin.join("server/main.rs"), "one").expect("write");
|
||||
std::fs::write(origin.join("app/main.kt"), "one").expect("write");
|
||||
for args in [
|
||||
&["git", "init", "-q", "-b", "main"][..],
|
||||
&["git", "config", "user.email", "t@example.com"],
|
||||
&["git", "config", "user.name", "Test"],
|
||||
&["git", "add", "-A"],
|
||||
&["git", "commit", "-qm", "one"],
|
||||
] {
|
||||
run(&origin, args);
|
||||
}
|
||||
let clone = dir.path().join("clone");
|
||||
run(
|
||||
dir.path(),
|
||||
&[
|
||||
"git",
|
||||
"clone",
|
||||
"-q",
|
||||
origin.to_str().unwrap(),
|
||||
clone.to_str().unwrap(),
|
||||
],
|
||||
);
|
||||
|
||||
let components = vec![
|
||||
Component::Server {
|
||||
name: "backend".to_string(),
|
||||
modes: Vec::new(),
|
||||
build: crate::config::ByMode::One(crate::config::Command::from_line(
|
||||
"touch backend-built",
|
||||
)),
|
||||
cwd: Some(PathBuf::from("server")),
|
||||
stale_when: None,
|
||||
also_watch: Vec::new(),
|
||||
service: None,
|
||||
mode: None,
|
||||
built_from: None,
|
||||
built_mode: None,
|
||||
},
|
||||
Component::Apk {
|
||||
name: "app".to_string(),
|
||||
modes: Vec::new(),
|
||||
build: crate::config::ByMode::One(crate::config::Command::from_line(
|
||||
"touch app-built",
|
||||
)),
|
||||
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,
|
||||
},
|
||||
];
|
||||
let state = state_for(&clone, components);
|
||||
|
||||
// Settle both components once, so each has a recorded commit to be
|
||||
// "current" against -- otherwise both would build regardless, as
|
||||
// "never built" rather than as a pull bringing something in.
|
||||
state.build_now(None, Arc::new(|_: &str, _: String| {}));
|
||||
while state.status().building {
|
||||
tokio::time::sleep(std::time::Duration::from_millis(10)).await;
|
||||
}
|
||||
assert!(clone.join("server/backend-built").exists());
|
||||
assert!(clone.join("app/app-built").exists());
|
||||
std::fs::remove_file(clone.join("server/backend-built")).expect("rm");
|
||||
std::fs::remove_file(clone.join("app/app-built")).expect("rm");
|
||||
|
||||
// A commit under the remote's `server/` alone, then Update.
|
||||
std::fs::write(origin.join("server/main.rs"), "two").expect("write");
|
||||
run(&origin, &["git", "commit", "-qam", "server fix"]);
|
||||
state.pull_and_build(false, || true, Arc::new(|_: &str, _: String| {}));
|
||||
while state.status().building {
|
||||
tokio::time::sleep(std::time::Duration::from_millis(10)).await;
|
||||
}
|
||||
|
||||
assert!(
|
||||
clone.join("server/backend-built").exists(),
|
||||
"backend moved, so it is the one to rebuild",
|
||||
);
|
||||
assert!(
|
||||
!clone.join("app/app-built").exists(),
|
||||
"nothing under app/ changed -- it must be left alone",
|
||||
);
|
||||
}
|
||||
|
||||
/// Naming one component builds only that one.
|
||||
///
|
||||
/// The case a project with more than one component exists to avoid
|
||||
|
||||
Reference in new issue
Block a user