diff --git a/crates/didbot-setup/src/bin/didbot-setup.rs b/crates/didbot-setup/src/bin/didbot-setup.rs index 6035f510..4e029e09 100644 --- a/crates/didbot-setup/src/bin/didbot-setup.rs +++ b/crates/didbot-setup/src/bin/didbot-setup.rs @@ -176,10 +176,14 @@ fn do_apply(options: &Options, stack: &Stack) -> ExitCode { // Still offered: settings written and no binary on PATH is a machine // wired to something that does not exist, and that is exactly the // state a second run is trying to get out of. - install_hook_binary(options); + let installed = install_hook_binary(options); println!("\n hooks load at session start, so a session open now is running whatever it started with."); println!(" `didbot-setup verify` proves a new one works."); - return ExitCode::SUCCESS; + return if installed { + ExitCode::SUCCESS + } else { + ExitCode::FAILURE + }; } println!( @@ -217,39 +221,52 @@ fn do_apply(options: &Options, stack: &Stack) -> ExitCode { return ExitCode::FAILURE; } }; + // Either of these leaves a machine that is not in the state `apply` + // reports: settings written that nothing can take back, or settings + // written that point at a binary the install failed to produce. The + // changes are real, so they are still printed — but the status is what an + // automated deployment reads, and it has to say something went wrong. + let mut whole = true; if let Err(err) = ledger.save(&ledger_path) { - // The changes are made; only the ability to undo them automatically is - // lost, and saying so is better than failing after the fact. eprintln!("didbot-setup: made the changes but cannot record them for undo: {err}"); + whole = false; } println!("\n made {made} change(s)."); - install_hook_binary(options); + whole &= install_hook_binary(options); println!("\n BLOCKED restart the harness"); println!(" Hook configuration is read once, at session start. This session is"); println!(" still running what it started with; the next one picks these up."); println!(" Then: didbot-setup verify"); - ExitCode::SUCCESS + if whole { + ExitCode::SUCCESS + } else { + ExitCode::FAILURE + } } /// Puts the hook binary on `PATH`, if it is not already this checkout's. /// +/// False when an install was attempted and failed. Declining one, or having +/// nothing to install from, is true: neither leaves the machine in a state +/// `apply` did not describe. +/// /// The one step here that is neither a file edit nor impossible: the harness /// invokes the hook by name, so a machine with the settings written and no /// binary is wired to something that does not exist. It is separated from the /// edits and asked for separately because it takes minutes rather than /// milliseconds and it writes outside the repository. -fn install_hook_binary(options: &Options) { +fn install_hook_binary(options: &Options) -> bool { let state = check::hook_binary_state(&options.dir); if !state.wants_installing() { - return; + return true; } let Some(root) = workspace_root(&options.dir) else { println!("\n didbot-hook is not this checkout's, and this is not a checkout of the"); println!(" workspace, so there is nothing here to install from."); - return; + return true; }; match &state { @@ -282,8 +299,10 @@ fn install_hook_binary(options: &Options) { ask(" install it now?") }; if !permitted { + // Declining is an answer, not a failure: nothing was attempted, and + // the caller asked for exactly that by leaving --install off. println!(" left alone."); - return; + return true; } let status = std::process::Command::new("cargo") @@ -297,9 +316,18 @@ fn install_hook_binary(options: &Options) { ]) .status(); match status { - Ok(status) if status.success() => println!(" installed."), - Ok(status) => println!(" cargo install exited {status}; the settings are still written."), - Err(err) => println!(" could not run cargo: {err}"), + Ok(status) if status.success() => { + println!(" installed."); + true + } + Ok(status) => { + println!(" cargo install exited {status}; the settings are still written."); + false + } + Err(err) => { + println!(" could not run cargo: {err}"); + false + } } } @@ -437,20 +465,24 @@ fn do_disk(options: &Options, clean: bool) -> ExitCode { return ExitCode::SUCCESS; } - let mut freed = 0; - for target in targets - .iter() - .filter(|target| target.owner.is_reclaimable()) - { - match std::fs::remove_dir_all(&target.path) { - Ok(()) => { - println!(" removed {}", target.path.display()); - freed += target.bytes; - } - Err(err) => println!(" kept {}: {err}", target.path.display()), - } + let cleaned = disk::clean(&targets); + for path in &cleaned.removed { + println!(" removed {}", path.display()); + } + for (path, err) in &cleaned.failed { + println!(" kept {}: {err}", path.display()); + } + println!("\n freed {}", disk::human(cleaned.freed)); + if !cleaned.failed.is_empty() { + // A script clears space before a build. One that cannot tell a clean + // from a no-op runs the build anyway and fails at the linker. + eprintln!( + "didbot-setup: {} of {} orphan(s) could not be removed", + cleaned.failed.len(), + cleaned.failed.len() + cleaned.removed.len() + ); + return ExitCode::FAILURE; } - println!("\n freed {}", disk::human(freed)); ExitCode::SUCCESS } diff --git a/crates/didbot-setup/src/disk.rs b/crates/didbot-setup/src/disk.rs index ad3462c1..8c1e537a 100644 --- a/crates/didbot-setup/src/disk.rs +++ b/crates/didbot-setup/src/disk.rs @@ -182,6 +182,44 @@ pub fn size_of(path: &Path) -> u64 { total } +/// What a clean actually managed to remove. +/// +/// Separated from the printing because the caller has to set an exit status +/// from it: a removal that failed and a removal that never happened look the +/// same to a script otherwise, and this is the one command here that a script +/// runs to make room before a build. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct Cleaned { + /// Bytes the removed directories held. + pub freed: u64, + /// What went. + pub removed: Vec, + /// What did not, and why. + pub failed: Vec<(PathBuf, String)>, +} + +/// Removes the reclaimable targets, reporting each outcome. +/// +/// Only [`Owner::is_reclaimable`] ones, and each independently: one directory +/// whose permissions have gone strange must not stop the other four from being +/// reclaimed on a machine that is out of disk. +pub fn clean(targets: &[Target]) -> Cleaned { + let mut cleaned = Cleaned::default(); + for target in targets + .iter() + .filter(|target| target.owner.is_reclaimable()) + { + match std::fs::remove_dir_all(&target.path) { + Ok(()) => { + cleaned.freed += target.bytes; + cleaned.removed.push(target.path.clone()); + } + Err(err) => cleaned.failed.push((target.path.clone(), err.to_string())), + } + } + cleaned +} + /// A size a person reads, rather than a number of bytes. pub fn human(bytes: u64) -> String { const UNITS: [(&str, u64); 4] = [ @@ -240,6 +278,83 @@ pub fn worktrees(root: &Path) -> Vec { mod tests { use super::*; + /// A scratch directory of this test's own, removed by whatever uses it. + fn scratch(name: &str) -> PathBuf { + let dir = + std::env::temp_dir().join(format!("didbot-setup-clean-{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).expect("scratch directory"); + dir + } + + #[test] + fn a_removal_that_failed_is_reported_and_the_rest_still_go() { + let root = scratch("partial"); + let gone = root.join("already-removed"); + let real = root.join("orphan"); + std::fs::create_dir_all(real.join("debug")).expect("orphan directory"); + + let cleaned = clean(&[ + Target { + path: gone.clone(), + owner: Owner::Orphan, + bytes: 4096, + }, + Target { + path: real.clone(), + owner: Owner::Orphan, + bytes: 1024, + }, + ]); + + // Load-bearing: without this the caller cannot tell a no-op from a + // clean, and exits zero either way. + assert_eq!( + cleaned.failed.len(), + 1, + "a directory that could not be removed was not reported" + ); + assert_eq!(cleaned.failed[0].0, gone); + assert_eq!(cleaned.removed, vec![real.clone()]); + assert_eq!( + cleaned.freed, 1024, + "the failed target's bytes were counted" + ); + assert!(!real.exists()); + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn clean_leaves_everything_that_belongs_to_somebody() { + let root = scratch("owned"); + let mine = root.join("here"); + let idle = root.join("idle"); + for dir in [&mine, &idle] { + std::fs::create_dir_all(dir).expect("owned directory"); + } + + let cleaned = clean(&[ + Target { + path: mine.clone(), + owner: Owner::Here, + bytes: 1, + }, + Target { + path: idle.clone(), + owner: Owner::IdleElsewhere, + bytes: 1, + }, + ]); + + assert!(mine.exists(), "this worktree's build directory was removed"); + assert!( + idle.exists(), + "another worktree's build directory was removed" + ); + assert_eq!(cleaned, Cleaned::default()); + let _ = std::fs::remove_dir_all(&root); + } + #[test] fn only_an_orphan_is_this_commands_business() { assert!(Owner::Orphan.is_reclaimable());