diff --git a/CHANGELOG.md b/CHANGELOG.md index a200c2bb..39ac9e22 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -197,6 +197,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 that collapse is what let an unparseable version fall through as a pass. ### Fixed +- **`avocado install` / `rootfs install` / `initramfs install` exit non-zero + when a sysroot install could not finish.** Three cases previously reported + success and wrote a current install stamp: the sysroot could not be cleaned + before a kernel change, the installed package versions could not be read + back, or the kernel sysroot could not be staged. Each now fails with the + reason (also emitted under `--json`) and leaves no stamp, so the next run + repairs instead of reporting "up to date". The lock is still saved whenever + packages landed, so a failed run cannot leave the kernel pin out of step + with the disk. - **`source_date_epoch` is honored outside extension images.** The key was read only by `ext image`, which exports it inside its own script. The rootfs build script has always passed `-T "${SOURCE_DATE_EPOCH:-0}"` to diff --git a/src/commands/initramfs/install.rs b/src/commands/initramfs/install.rs index 1aee3533..1f7c6b5c 100644 --- a/src/commands/initramfs/install.rs +++ b/src/commands/initramfs/install.rs @@ -7,7 +7,7 @@ use crate::utils::{ config::{ComposedConfig, Config}, container::{RunConfig, SdkContainer}, lockfile::{LockFile, SysrootType}, - output::{print_error, OutputLevel}, + output::{print_error, print_warning_stderr, OutputLevel}, runs_on::RunsOnContext, target::validate_and_log_target, }; @@ -149,10 +149,10 @@ impl InitramfsInstallCommand { // `runs_on` NFS server and remote mount, and an early return here would // skip it — the next `--runs-on` run picks a fresh nfs_port and stacks // another one on top. Carried into `result` and returned past teardown. - let result = match prefetched_stamp { - Err(e) => Err(e), + let (result, pins_recorded) = match prefetched_stamp { + Err(e) => (Err(e), false), Ok(prefetched_stamp) => { - install_sysroot(&mut SysrootInstallParams { + let mut params = SysrootInstallParams { sysroot_type: SysrootType::Initramfs, config, lock_file: &mut lock_file, @@ -173,17 +173,36 @@ impl InitramfsInstallCommand { parsed: Some(&composed.merged_value), prefetched_stamp, tui_context: None, - }) - .await + pins_recorded: false, + }; + let outcome = install_sysroot(&mut params).await; + // Copied out before `params` drops: it holds the `&mut` on + // `lock_file`, which the save below needs back. + let pins_recorded = params.pins_recorded; + (outcome, pins_recorded) } }; // Persist the lockfile the install updated — `install_sysroot` leaves // saving to its caller. Folded into `result` rather than `?` so a save // failure still reaches the teardown below. - let result = match result { - Ok(()) => lock_file.save(src_dir), - Err(e) => Err(e), + // + // Also saved when the install failed *after* recording pins: they + // describe the packages that actually landed, and dropping them makes + // the next run re-resolve the kernel against feed head with no + // `prev_pinned_kver` to compare against. The install error is the one + // returned — it is what the user has to act on. + let result = match (result, pins_recorded) { + (Ok(()), _) => lock_file.save(src_dir), + (Err(e), true) => { + if let Err(save_err) = lock_file.save(src_dir) { + // stderr, not print_error: under --json print_error is + // suppressed and this notice must survive. + print_warning_stderr(&format!("Failed to save lock file: {save_err}")); + } + Err(e) + } + (Err(e), false) => Err(e), }; if let Some(ref mut context) = runs_on_context { diff --git a/src/commands/rootfs/install.rs b/src/commands/rootfs/install.rs index 52defa9a..8bfc9060 100644 --- a/src/commands/rootfs/install.rs +++ b/src/commands/rootfs/install.rs @@ -53,7 +53,9 @@ use crate::utils::{ kernel_resolver::{off_kernel_dnf_excludes, resolve_and_pin_kernel_version, ResolveParams}, kernel_version::substitute_kernel_version, lockfile::{build_package_spec_with_lock, LockFile, SysrootType}, - output::{print_error, print_info, print_success, print_warning, OutputLevel}, + output::{ + print_error, print_info, print_success, print_warning, print_warning_stderr, OutputLevel, + }, prerequisites::read_stamps_batch, runs_on::RunsOnContext, stamps::{ @@ -96,6 +98,16 @@ pub struct SysrootInstallParams<'a> { pub prefetched_stamp: Option, /// TUI context for output capture (if TUI is active). pub tui_context: Option, + /// Set by [`install_sysroot`] the moment the package install has landed on + /// disk. From then on `lock_file` describes that disk - the kernel pin + /// chosen for it, the sections cleared for a clean reinstall, and (when + /// they could be read) the installed package versions - so it must be + /// persisted whatever happens next. It stays set when the install then + /// fails: a caller that drops the lock leaves disk holding kernel B while + /// the lock still says A, and the next run honors A's pin and installs + /// additively on top of B. Callers must persist (or merge) the lock + /// whenever this is true, whatever the returned `Result` says. + pub pins_recorded: bool, } impl SysrootInstallParams<'_> { @@ -268,6 +280,53 @@ fi Ok(()) } +/// Why an install that placed its packages still must not report success. +/// +/// `None` means every claim the install stamp makes holds: the packages are on +/// disk, the lock records their versions, and (for rootfs with a pinned +/// kernel) the kernel sysroot is staged. `Some(reason)` means one of them does +/// not, so no stamp is written and `runtime build` will refuse to build. +/// +/// Split out of [`install_sysroot`] to keep the wording under test: it is the +/// only place a user learns why an install that looked fine left the build +/// unable to proceed. +fn incomplete_install_reason( + install_is_clean: bool, + versions_recorded: bool, + kernel_staging_error: Option<&str>, +) -> Option { + if !install_is_clean { + // Checked first: a sysroot that could not be cleaned may be carrying + // packages from a previous config, which makes the other two signals + // describe the wrong tree. + Some("the sysroot could not be cleaned first, so stale contents may remain".to_string()) + } else if !versions_recorded { + Some("the installed package versions could not be read".to_string()) + } else { + kernel_staging_error.map(|detail| { + format!( + "the kernel sysroot could not be staged ({detail}). Verify that the rootfs \ + sysroot's /boot contains a kernel image for this kernel version; without one \ + the packages that were installed do not include a bootable kernel" + ) + }) + } +} + +/// The error an install that placed its packages but could not finish reports. +/// +/// Kept next to [`incomplete_install_reason`] and under test because this text +/// is the whole remedy path: without it the user sees a passing `install` and a +/// `build` that names ` install` as missing, whose printed fix is the +/// command that just passed. +fn incomplete_install_error(label: &str, reason: &str) -> anyhow::Error { + anyhow::anyhow!( + "Installed the {label} sysroot's packages, but the install did not complete: {reason}. \ + No install stamp was recorded, so `avocado build` will keep reporting `{label} install` \ + as missing until this succeeds." + ) +} + /// Detect package removals by comparing the **effective** package set for /// this sysroot against what the lockfile recorded. A non-empty result means /// the sysroot must be cleaned and reinstalled from scratch, because dnf @@ -1025,6 +1084,10 @@ $DNF_SDK_HOST $DNF_SDK_TARGET_REPO_CONF \ if success { print_success(&format!("Installed {label} sysroot."), OutputLevel::Normal); + // Packages are on disk: the lock's kernel pin and cleared sections now + // describe it, whether or not the version query below succeeds. Tell + // the caller before anything else can fail. + params.pins_recorded = true; // Query installed versions for ALL config packages and update lock file let installed_versions = params @@ -1050,7 +1113,7 @@ $DNF_SDK_HOST $DNF_SDK_TARGET_REPO_CONF \ // stamp anyway latches the broken state: every later run reports "up to // date" and the user is wedged until they guess --no-stamps. let versions_recorded = !installed_versions.is_empty(); - let mut kernel_staging_ok = true; + let mut kernel_staging_error: Option = None; let install_is_clean = clean_ok; if versions_recorded { @@ -1107,14 +1170,7 @@ $DNF_SDK_HOST $DNF_SDK_TARGET_REPO_CONF \ ) .await { - print_error( - &format!( - "Kernel sysroot staging failed: {e}. \ - provision may fall back to reading the Image from the rootfs sysroot." - ), - OutputLevel::Normal, - ); - kernel_staging_ok = false; + kernel_staging_error = Some(e.to_string()); } } } @@ -1125,32 +1181,38 @@ $DNF_SDK_HOST $DNF_SDK_TARGET_REPO_CONF \ // derived: the install just re-pinned this sysroot's packages, and the // stamp has to record the lock state the *next* run will compare // against. - let stamp_is_trustworthy = versions_recorded && kernel_staging_ok && install_is_clean; - - if !params.no_stamps && !stamp_is_trustworthy { - print_warning( - &format!( - "Not recording an install stamp for {label}: {}. \ - The next run will reinstall rather than report it up to date.", - if !install_is_clean { - "the sysroot could not be cleaned first, so stale contents may remain" - } else if !versions_recorded { - "the installed package versions could not be read" - } else { - "kernel sysroot staging failed" - } - ), - OutputLevel::Normal, - ); + // Fail rather than return a success the stamp contradicts. + // + // Each of these means the sysroot on disk is not the one the config + // asked for, so no stamp is written -- and `runtime build` refuses to + // build without one. Reporting the step as succeeded left the two + // commands disagreeing: a green `install` followed by `build` naming + // `rootfs install` as missing, with `avocado rootfs install` as the + // suggested remedy -- the command that had just "succeeded". Nothing in + // that loop names the actual fault, so it reads as user error, and the + // reason (a `print_warning`) is invisible whenever the TUI is active, + // which is the default for `avocado install`. + // + // Failing here surfaces the reason through the failed-task rendering + // and stops the loop at the command that can still explain itself. + // `--no-stamps` does not suppress it: these are install failures, and + // the stamp is only how they became visible. + if let Some(reason) = incomplete_install_reason( + install_is_clean, + versions_recorded, + kernel_staging_error.as_deref(), + ) { + return Err(incomplete_install_error(label, &reason)); } - // Deliberately not `?` anywhere below. The pins recorded above are only - // persisted by the caller when this function returns Ok, so propagating a - // stamp-write failure would discard the pins for an install that already - // landed — the next run would re-resolve the kernel against feed head with - // no prev_pinned_kver to compare against and install additively on top. - // A missing stamp is the benign outcome: the next run reinstalls. - if !params.no_stamps && stamp_is_trustworthy { + // Deliberately not `?`. Unlike the checks above -- which report a + // sysroot that is not what the config asked for -- a stamp-write + // failure leaves a correct sysroot that simply is not recorded, and the + // next run reinstalls. Failing here would add nothing and cost the + // teardown the callers run on the way out. + // + // Reaching this line means `incomplete_install_reason` returned None. + if !params.no_stamps { if let Err(e) = write_install_stamp(params, &packages, label).await { print_warning( &format!( @@ -1302,10 +1364,10 @@ impl RootfsInstallCommand { // `runs_on` NFS server and remote mount, and an early return here would // skip it — the next `--runs-on` run picks a fresh nfs_port and stacks // another one on top. Carried into `result` and returned past teardown. - let result = match prefetched_stamp { - Err(e) => Err(e), + let (result, pins_recorded) = match prefetched_stamp { + Err(e) => (Err(e), false), Ok(prefetched_stamp) => { - install_sysroot(&mut SysrootInstallParams { + let mut params = SysrootInstallParams { sysroot_type: SysrootType::Rootfs, config, lock_file: &mut lock_file, @@ -1326,8 +1388,13 @@ impl RootfsInstallCommand { parsed: Some(&composed.merged_value), prefetched_stamp, tui_context: None, - }) - .await + pins_recorded: false, + }; + let outcome = install_sysroot(&mut params).await; + // Copied out before `params` drops: it holds the `&mut` on + // `lock_file`, which the save below needs back. + let pins_recorded = params.pins_recorded; + (outcome, pins_recorded) } }; @@ -1335,9 +1402,23 @@ impl RootfsInstallCommand { // longer saves for itself — under `avocado sdk install` it runs on a // clone that the caller merges and saves once. Folded into `result` // rather than `?` so a save failure still reaches the teardown below. - let result = match result { - Ok(()) => lock_file.save(src_dir), - Err(e) => Err(e), + // + // Also saved when the install failed *after* recording pins: they + // describe the packages that actually landed, and dropping them makes + // the next run re-resolve the kernel against feed head with no + // `prev_pinned_kver` to compare against. The install error is the one + // returned — it is what the user has to act on. + let result = match (result, pins_recorded) { + (Ok(()), _) => lock_file.save(src_dir), + (Err(e), true) => { + if let Err(save_err) = lock_file.save(src_dir) { + // stderr, not print_error: under --json print_error is + // suppressed and this notice must survive. + print_warning_stderr(&format!("Failed to save lock file: {save_err}")); + } + Err(e) + } + (Err(e), false) => Err(e), }; // Always teardown runs_on context @@ -1356,10 +1437,71 @@ impl RootfsInstallCommand { #[cfg(test)] mod tests { - use super::{build_overlay_script, detect_sysroot_package_removals, parse_probe_output}; + use super::{ + build_overlay_script, detect_sysroot_package_removals, incomplete_install_error, + incomplete_install_reason, parse_probe_output, + }; use crate::utils::lockfile::{LockFile, SysrootType}; use std::collections::{HashMap, HashSet}; + // A sysroot install that places its packages but cannot finish the job must + // not report success: no stamp is written for it, and `runtime build` + // refuses to build without one. Returning Ok here is what produced a green + // `avocado install` followed by `avocado build` reporting `rootfs install` + // as missing and suggesting the very command that had just passed. + #[test] + fn a_complete_install_has_no_reason_to_fail() { + assert_eq!(incomplete_install_reason(true, true, None), None); + } + + #[test] + fn kernel_staging_failure_is_reported_with_its_cause() { + let reason = incomplete_install_reason( + true, + true, + Some("Failed to stage kernel sysroot for kernel-version '6.18.37'"), + ) + .expect("staging failure must not be silent"); + // The underlying error is carried, not swallowed — it is the only part + // that names which kernel version had no image. + assert!(reason.contains("6.18.37"), "{reason}"); + assert!(reason.contains("/boot"), "{reason}"); + } + + #[test] + fn an_unreadable_package_list_and_an_unclean_sysroot_each_fail() { + assert!(incomplete_install_reason(true, false, None) + .expect("unread versions must not be silent") + .contains("package versions")); + assert!(incomplete_install_reason(false, true, None) + .expect("an unclean sysroot must not be silent") + .contains("cleaned")); + } + + #[test] + fn an_unclean_sysroot_outranks_the_later_signals() { + // Order matters: a sysroot that could not be cleaned may hold packages + // from a previous config, which makes the other two signals describe + // the wrong tree. Report the cause, not a symptom of it. + let reason = incomplete_install_reason(false, false, Some("staging blew up")) + .expect("must not be silent"); + assert!(reason.contains("cleaned"), "{reason}"); + assert!(!reason.contains("staging blew up"), "{reason}"); + } + + #[test] + fn the_failure_names_the_sysroot_and_the_remedy_loop_it_breaks() { + let msg = incomplete_install_error("rootfs", "the kernel sysroot could not be staged") + .to_string(); + assert!(msg.contains("rootfs"), "{msg}"); + // Says the packages did land — otherwise it reads as "nothing worked". + assert!(msg.contains("Installed the rootfs sysroot"), "{msg}"); + // And ties the install-time fault to the build-time symptom, so the + // user does not go looking for a mistake of their own. + assert!(msg.contains("No install stamp"), "{msg}"); + assert!(msg.contains("avocado build"), "{msg}"); + } + const KVER: &str = "6.8.12-l4t-r39.2.0-1021.21"; const TARGET: &str = "jetson-agx-thor"; diff --git a/src/commands/sdk/install.rs b/src/commands/sdk/install.rs index d9993590..3f2a7983 100644 --- a/src/commands/sdk/install.rs +++ b/src/commands/sdk/install.rs @@ -550,6 +550,7 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ parsed: Some(&composed.merged_value), prefetched_stamp: rootfs_stamp, tui_context: rootfs_tui, + pins_recorded: false, }; let mut initramfs_params = SysrootInstallParams { sysroot_type: SysrootType::Initramfs, @@ -572,6 +573,7 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ parsed: Some(&composed.merged_value), prefetched_stamp: initramfs_stamp, tui_context: initramfs_tui, + pins_recorded: false, }; // Build the target-dev future (or a no-op if not needed) @@ -626,10 +628,17 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ async { let result = $fut.await; if let Some(r) = crate::utils::tui::get_active_renderer() { - if result.is_ok() { - r.set_status(&$task_id, TaskStatus::Success); - } else { - r.set_status(&$task_id, TaskStatus::Failed); + match &result { + Ok(_) => r.set_status(&$task_id, TaskStatus::Success), + Err(e) => { + // Same shape as the scheduler executor: the + // message must be set for --json to emit a + // step_error event; a bare Failed status + // carries no reason and print_error is + // suppressed in that mode. + r.set_error(&$task_id, format!("{e:#}")); + r.set_status(&$task_id, TaskStatus::Failed); + } } } result @@ -666,6 +675,12 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ // Merge lock file changes back into a single lock file for saving let mut final_lock = lock_file.clone(); + // A sysroot install that failed *after* recording its pins still has + // pins worth merging — they describe the packages that landed. See + // `SysrootInstallParams::pins_recorded`. + let rootfs_landed = rootfs_result.is_ok() || rootfs_params.pins_recorded; + let initramfs_landed = initramfs_result.is_ok() || initramfs_params.pins_recorded; + // Merge SDK packages lock if sdk_pkg_result.is_ok() { if let Some(target_locks) = sdk_pkg_lock.targets.get(target) { @@ -678,7 +693,7 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ // `kernels` entries staged from the rootfs install (Phase 2c) — they // live on the clone too and would otherwise be dropped on the merge // floor. - if rootfs_result.is_ok() { + if rootfs_landed { if let Some(target_locks) = rootfs_lock.targets.get(target) { let entry = final_lock.targets.entry(target.to_string()).or_default(); entry.rootfs = target_locks.rootfs.clone(); @@ -706,7 +721,7 @@ $DNF_SDK_HOST $DNF_NO_SCRIPTS $DNF_SDK_TARGET_REPO_CONF \ } } // Merge initramfs lock. - if initramfs_result.is_ok() { + if initramfs_landed { if let Some(target_locks) = initramfs_lock.targets.get(target) { let entry = final_lock.targets.entry(target.to_string()).or_default(); entry.initramfs = target_locks.initramfs.clone(); diff --git a/src/utils/output.rs b/src/utils/output.rs index 2c69c58e..4cc62553 100644 --- a/src/utils/output.rs +++ b/src/utils/output.rs @@ -17,7 +17,9 @@ pub enum OutputLevel { /// When a TUI renderer is active, info/success/warning/plain messages are /// suppressed — the TUI task status lines are the progress indicator. -/// Errors always print (via `print_above`) so they're visible immediately. +/// `print_error` is suppressed too (the error is reported through task state +/// at shutdown); a notice that must survive both modes goes through +/// `print_warning_stderr` / `print_info_stderr`. /// /// Also returns true when JSON output mode is active: prose would interleave /// with the NDJSON event stream and confuse consumers, so we treat JSON