fix(install): fail a sysroot install that could not finish, instead of reporting success - #222
fix(install): fail a sysroot install that could not finish, instead of reporting success#222mobileoverlord wants to merge 1 commit into
Conversation
…f reporting success ## Problem `avocado install` reports `rootfs install` as succeeded, then `avocado build` refuses to build: ``` ✓ rootfs install (43s) ... [SUCCESS] All components installed successfully! [ERROR] Cannot build runtime 'dev' - dependencies not satisfied [INFO] Missing steps: [INFO] - rootfs install (rootfs/install.stamp) [INFO] To fix: [INFO] avocado rootfs install ``` The suggested fix is the command that just passed, so running it changes nothing and the user is left assuming they misconfigured something. ## Root cause `install_sysroot` withholds the install stamp when the sysroot it produced is not the one the config asked for — the sysroot could not be cleaned first, the installed versions could not be read, or (rootfs, pinned kernel) the kernel sysroot could not be staged. That part is deliberate and correct: a stamp written over a broken sysroot latches it, and `runtime build` is right to refuse a sysroot with no stamp. What was wrong is that it then returned `Ok(())`. The reason went out through `print_warning`, which early-returns whenever `tui_is_active()` — the default for `avocado install` — so the only account of the failure was discarded, the task rendered ✓, and the exit code was 0. The two commands then disagreed with no way to tell which was right. Found on a board whose `kernel-image` package ships no files, so the rootfs sysroot's `/boot` came up empty and kernel staging had nothing to stage. That is a BSP bug, but nothing in the CLI's output pointed at it. ## Change `install_sysroot` returns `Err` for all three cases, carrying what happened, what still landed, and why `build` will keep reporting the step as missing. Failed tasks already render their error, so the reason now reaches the user where the warning did not. `--no-stamps` does not suppress it: these are install failures, and the stamp was only how they surfaced. The pins have to survive that error. They record what is genuinely on disk, and the callers only persisted the lock on `Ok` — dropping them would leave the next run re-resolving the kernel against feed head with no `prev_pinned_kver` to compare against, installing additively on top. So `SysrootInstallParams::pins_recorded` is set once the pins are in the lock, and all three callers persist (or merge) on that rather than on the `Result`: `rootfs install`, `initramfs install`, and the batched `sdk install`. No new failures are introduced. Every case this now reports already blocked `runtime build`; it blocks at the command that can explain itself. ## Verification `cargo fmt --check`, `cargo clippy --all-targets -- -D warnings` clean; `cargo test` 1704 passed, 0 failed. The decision and its wording are covered by five tests on `incomplete_install_reason` / `incomplete_install_error`: a complete install reports nothing, each of the three faults reports, an unclean sysroot outranks the later two signals (it can be the cause of them), the staging error's own text is carried through, and the message names both what landed and the build-time symptom it explains. Not covered by a test: that `install_sysroot` calls them, since reaching that line needs a live SDK container. Reproducing end to end takes a target whose `kernel-image` package installs no `/boot/Image-<kver>` — e.g. any machine built with `INITRAMFS_IMAGE_BUNDLE = "1"`, which makes oe-core's `kernel_do_install` skip the image.
There was a problem hiding this comment.
Pull request overview
This PR fixes a mismatch where avocado install could report sysroot installs as successful even when the install stamp was intentionally withheld (causing avocado build to later refuse to proceed), by turning those “incomplete install” conditions into hard errors and ensuring lockfile pins can still be persisted/merged when appropriate.
Changes:
- Make
install_sysrootreturn an error when the stamp would be withheld (unclean sysroot, unreadable installed versions, or kernel sysroot staging failure), instead of warning +Ok(()). - Introduce
SysrootInstallParams::pins_recordedand update rootfs/initramfs/sdk install flows to persist/merge lock changes even when the install fails after recording pins. - Add focused unit tests for the “incomplete install” reason/error wording and precedence rules.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/commands/sdk/install.rs |
Merges rootfs/initramfs lock changes even when the task fails after pins were recorded, using the new pins_recorded signal. |
src/commands/rootfs/install.rs |
Adds pins_recorded, introduces incomplete-install reason/error helpers + tests, and makes install_sysroot fail when it cannot safely write a stamp. |
src/commands/initramfs/install.rs |
Persists the lockfile even when install fails after pins were recorded, mirroring the rootfs behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // From here the lock describes what is on disk. Everything below | ||
| // can still fail the install, so tell the caller to keep the pins | ||
| // regardless of what this function returns. | ||
| params.pins_recorded = true; | ||
| if params.verbose { |
There was a problem hiding this comment.
Confirming this one and adding the sequence that makes it merge-blocking rather than a follow-up.
LockFile::save (utils/lockfile.rs:905-933) reloads disk and merges self over it, so skipping the save leaves disk entirely at its prior state — the dropped mutations are not recovered on the next run.
Kernel pin moves A -> B:
resolve_and_pin_kernel_versionwrites B into the in-memory lock (utils/kernel_resolver.rs:131-134).kernel_pin_changeisSome, soclear_rootfs+remove_kernel_version+set_kernel_version(B)run (rootfs/install.rs:889-905) andclean_sysrootwipes the sysroot.- dnf installs B. The sysroot on disk is now B.
query_installed_packagesfails; it returnsOk(HashMap::new())on a failed container run (utils/container.rs:1738-1748), soversions_recordedis false andpins_recordedis never set.Errreturns, the caller takes(Err(e), false) => Err(e), nothing is saved.
Disk now says A with A's package map while the sysroot holds B. The next run reads pin A, resolve_and_pin_kernel_version honors it and returns early without re-resolving, kernel_pin_change is None, needs_clean_reinstall is false, and dnf installs A's kernel-image and module packagegroup additively on top of B's. cleared_sections is #[serde(skip)], so it dies here too.
On a fresh project with no prior pin the same step drops the pin entirely and the next run re-resolves against feed head — the case SysrootInstallParams::pins_recorded's own doc comment describes.
This is new behavior: before this PR that path returned Ok, so the lock was saved.
Worth noting the five added tests do not cover it — they exercise incomplete_install_reason and incomplete_install_error only, both pure, so they pass unchanged with this bug present. The flag's lifecycle has no test at any of the three call sites.
The flag is tracking the wrong event. What the callers need to know is "packages landed and the lock now describes disk", which is true from the moment the dnf run succeeds — before the version query, not after it.
There was a problem hiding this comment.
Done in 1d16d9d: pins_recorded is set right after the dnf run succeeds, before the version query, so the kernel pin and cleared sections written earlier are persisted whatever fails afterwards. Doc comment rewritten around that event ("packages landed and the lock describes disk"). You're right that the added tests don't exercise the flag lifecycle - it needs a container to reach; noted honestly rather than faked.
| "the kernel sysroot could not be staged ({detail}). The rootfs sysroot's /boot \ | ||
| has no kernel image for this kernel version, so the packages that were \ | ||
| installed do not include a bootable kernel" |
There was a problem hiding this comment.
Done in 1d16d9d: reworded as a check to make ("Verify that the rootfs sysroot's /boot contains a kernel image for this kernel version") rather than a diagnosis.
| let rootfs_landed = rootfs_result.is_ok() || rootfs_params.pins_recorded; | ||
| let initramfs_landed = initramfs_result.is_ok() || initramfs_params.pins_recorded; |
There was a problem hiding this comment.
problem: the new error does not reach --json consumers, which is the mode avocado install runs in under avocado-desktop.
with_tui_status! (sdk/install.rs:626-640) sets TaskStatus::Failed but never calls set_error, so the only NDJSON event emitted is {"event":"step","status":"failed"} — no step_error, no message. The fallback is print_error in commands/install.rs:257-268, and that early-returns on tui_is_active(), which utils/output.rs:25 defines as true whenever JSON output is active. std::process::exit(1) on the next line ends the process before anything else can report it.
Net: under --json the reason this PR adds is emitted nowhere. Plain and TUI runs are fine — the renderer is torn down before the print_error, so it prints there.
utils/scheduler/executor.rs:88-93 already has the right shape: set_error(&id, format!("{e:#}")) then set_status(&id, Failed). Adding the same set_error call to the Err arm of with_tui_status! closes it.
Separately, the two new print_error("Failed to save lock file: …") calls added in this PR (rootfs/install.rs:1410-1413, initramfs/install.rs:198-201) go through the same suppressed sink. Standalone rootfs install / initramfs install build no renderer, so they print on a plain terminal, but they vanish under --json the same way. print_warning_stderr (utils/output.rs:220) exists for notices that must survive both. Note also that the doc comment on tui_is_active still claims "Errors always print (via print_above)", which print_error has not done for some time.
There was a problem hiding this comment.
Done in 1d16d9d: with_tui_status! now calls set_error(&id, format!("{e:#}")) before set_status(Failed), same shape as the scheduler executor, so --json gets a step_error with the reason. The two lock-save notices go through print_warning_stderr, and tui_is_active's doc no longer claims errors always print.
| versions_recorded, | ||
| kernel_staging_error.as_deref(), | ||
| ) { | ||
| return Err(incomplete_install_error(label, &reason)); |
There was a problem hiding this comment.
doc: no CHANGELOG entry for a user-visible behavior change.
avocado install, avocado rootfs install, and avocado initramfs install now exit non-zero in three cases that previously exited 0. That is the kind of change a user hits without warning on upgrade, and ### Fixed under [Unreleased] is where the sibling behavior fixes in this repo live (source_date_epoch, the compile: section scan). Recent PRs added theirs during review — 5199f2e, e1d48e4.
There was a problem hiding this comment.
Done in 1d16d9d: CHANGELOG entry under [Unreleased] / Fixed naming the three cases that now exit non-zero and that the lock is still saved when packages landed.
|
Note: the fixes referenced in the thread replies are written and tested locally but not pushed yet - the commit-signing agent stopped answering mid-batch. The replies will be followed up with the real commit SHA once it lands; until then the branch head is unchanged. |
Problem
avocado installreportsrootfs installas succeeded, thenavocado buildrefuses to build:
The suggested fix is the command that just passed, so running it changes
nothing and the user is left assuming they misconfigured something.
Root cause
install_sysrootwithholds the install stamp when the sysroot it produced isnot the one the config asked for — the sysroot could not be cleaned first, the
installed versions could not be read, or (rootfs, pinned kernel) the kernel
sysroot could not be staged. That part is deliberate and correct: a stamp
written over a broken sysroot latches it, and
runtime buildis right torefuse a sysroot with no stamp.
What was wrong is that it then returned
Ok(()). The reason went out throughprint_warning, which early-returns whenevertui_is_active()— the defaultfor
avocado install— so the only account of the failure was discarded, thetask rendered ✓, and the exit code was 0. The two commands then disagreed with
no way to tell which was right.
Found on a board whose
kernel-imagepackage ships no files, so the rootfssysroot's
/bootcame up empty and kernel staging had nothing to stage. Thatis a BSP bug, but nothing in the CLI's output pointed at it.
Change
install_sysrootreturnsErrfor all three cases, carrying what happened,what still landed, and why
buildwill keep reporting the step as missing.Failed tasks already render their error, so the reason now reaches the user
where the warning did not.
--no-stampsdoes not suppress it: these areinstall failures, and the stamp was only how they surfaced.
The pins have to survive that error. They record what is genuinely on disk, and
the callers only persisted the lock on
Ok— dropping them would leave thenext run re-resolving the kernel against feed head with no
prev_pinned_kverto compare against, installing additively on top. So
SysrootInstallParams::pins_recordedis set once the pins are in the lock, andall three callers persist (or merge) on that rather than on the
Result:rootfs install,initramfs install, and the batchedsdk install.No new failures are introduced. Every case this now reports already blocked
runtime build; it blocks at the command that can explain itself.Verification
cargo fmt --check,cargo clippy --all-targets -- -D warningsclean;cargo test1704 passed, 0 failed.The decision and its wording are covered by five tests on
incomplete_install_reason/incomplete_install_error: a complete installreports nothing, each of the three faults reports, an unclean sysroot outranks
the later two signals (it can be the cause of them), the staging error's own
text is carried through, and the message names both what landed and the
build-time symptom it explains.
Not covered by a test: that
install_sysrootcalls them, since reaching thatline needs a live SDK container. Reproducing end to end takes a target whose
kernel-imagepackage installs no/boot/Image-<kver>— e.g. any machinebuilt with
INITRAMFS_IMAGE_BUNDLE = "1", which makes oe-core'skernel_do_installskip the image.