diff --git a/CHANGELOG.md b/CHANGELOG.md index 171bf8ba..5525836f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **Installs no longer prompt, and `--force` no longer means "don't prompt".** + `avocado install`, `ext install`, `runtime install` and `sdk install` apply + the package set `avocado.yaml` and `avocado.lock` already declare, so dnf's + confirmation offers no decision: approving it changes nothing and declining + it leaves a half-configured sysroot. All four now pass `-y` unconditionally. + + Previously `-y` was passed only under `--force`, which conflated two + unrelated things and made the documented invocation the expensive one. + `--force` *also* clears every extension's sysroot and drops its build and + image stamps, so anyone passing `-f` merely to skip the prompts — which is + what our own material tells people to do — discarded all built extension + content and paid a full rebuild on the next `avocado build`, every + iteration. Measured on a four-extension project: `install -f` then `build` + skipped 2 of 10 steps and took 10.2s; without `-f` it skips all 10 and takes + 6.6s. + + `--force` now means only what its name says: reinstall from scratch. Existing + scripts and docs using `-f` keep working, but should drop the flag — its help + text now says so. `sdk dnf`, `ext dnf` and `runtime dnf` are unchanged: they + pass their arguments to dnf verbatim and still prompt, which is the right + behaviour when a person is driving dnf directly. + + No opt-out flag was added. An interactive confirmation cannot be answered + usefully in CI or under the TUI, and the dnf pass-through commands already + cover reviewing a transaction by hand. + +- **`--output json` no longer implies `--force`.** It did, because the TUI + renderer was gated on `--force` — dnf could prompt without it, and the + renderer made the prompt invisible — and because `docker run -it` fails + where no terminal is attached. Both reasons are gone: installs now always + pass `-y`, and `utils::interactivity` decides the container's stdio flags + from what the environment can actually support. With `--force` meaning + reinstall from scratch, keeping the coercion would have turned every request + for machine-readable output into a full rebuild. The renderer is no longer + gated on `--force` either, so an interactive `avocado install` gets the live + checklist without asking for a rebuild to see it. + ### Fixed - `avocado signing-keys create` no longer generates a key before discovering the name is taken. A duplicate name is rejected up front, so a repeated diff --git a/src/commands/container/dev.rs b/src/commands/container/dev.rs index 79dbf8a6..1301948e 100644 --- a/src/commands/container/dev.rs +++ b/src/commands/container/dev.rs @@ -1505,6 +1505,16 @@ mod tests { ); drop(held); + // `flock` locks belong to the open file description, and `fork` shares + // it: any test on another thread that is between `fork` and `exec` at + // this instant (`sh -n`, `python3`, ...) still holds a reference to the + // lock's fd until CLOEXEC closes it. So release is observable only once + // that window passes. Bound the wait with the same budget `acquire` uses + // to tell a transient hold from a real one. + let deadline = Instant::now() + LOCK_ACQUIRE_WAIT; + while session_is_live(&lock).unwrap() && Instant::now() < deadline { + std::thread::sleep(LOCK_ACQUIRE_POLL); + } assert!( !session_is_live(&lock).unwrap(), "releasing the lock must make the session read as dead again" diff --git a/src/commands/ext/install.rs b/src/commands/ext/install.rs index a6addbbc..5899bafd 100644 --- a/src/commands/ext/install.rs +++ b/src/commands/ext/install.rs @@ -1083,7 +1083,10 @@ impl ExtInstallCommand { if !packages.is_empty() { // Build DNF install command - let yes = if self.force { "-y" } else { "" }; + // dnf never prompts here: this applies the package set avocado.yaml and + // avocado.lock already declare, so there is no decision left to make. + // `sdk dnf` / `ext dnf` / `runtime dnf` are the interactive path. + let yes = "-y"; let installroot = format!("$AVOCADO_EXT_SYSROOTS/{extension}"); let dnf_args_str = if let Some(args) = &self.dnf_args { format!(" {} ", args.join(" ")) diff --git a/src/commands/install.rs b/src/commands/install.rs index 8577c7c8..eb3d9661 100644 --- a/src/commands/install.rs +++ b/src/commands/install.rs @@ -200,28 +200,34 @@ impl InstallCommand { // Register SDK + sysroot tasks upfront (we know these from config). // Ext/runtime tasks are added after config reload. // Create a renderer when either: - // • TUI mode is on (interactive terminal + --force so dnf gets - // --assumeyes), so the user sees a live checklist; or + // • TUI mode is on (an interactive terminal), so the user sees a + // live checklist; or // • JSON output mode is on, so the renderer's state-mutators // emit NDJSON `step` events for the desktop app's step list. // // In JSON mode the renderer is in Passthrough mode and doesn't // paint anything to stderr — its hooks only fire the JSON sink. - let renderer = - if crate::utils::output::should_create_renderer() && !self.verbose && self.force { - let r = Arc::new(TaskRenderer::new(false)); - r.register_task(TaskId::SdkInstall, "sdk bootstrap".to_string()); - r.register_task(TaskId::SdkPackages, "sdk packages".to_string()); - r.register_task(TaskId::RootfsInstall, "rootfs install".to_string()); - r.register_task(TaskId::InitramfsInstall, "initramfs install".to_string()); - // target-dev install is registered dynamically by sdk/install.rs - // after fetching extensions and discovering compile sections - crate::utils::tui::set_active_renderer(&r); - r.start(); - Some(r) - } else { - None - }; + // + // No longer gated on `--force`. That gate existed because dnf could + // prompt without it and the TUI made the prompt invisible; installs + // now always pass `-y`, so the reason is gone. Keeping the gate is + // what forced `--output json` to imply `--force`, and with `--force` + // meaning reinstall-from-scratch that turned a request for machine + // output into a full rebuild. + let renderer = if crate::utils::output::should_create_renderer() && !self.verbose { + let r = Arc::new(TaskRenderer::new(false)); + r.register_task(TaskId::SdkInstall, "sdk bootstrap".to_string()); + r.register_task(TaskId::SdkPackages, "sdk packages".to_string()); + r.register_task(TaskId::RootfsInstall, "rootfs install".to_string()); + r.register_task(TaskId::InitramfsInstall, "initramfs install".to_string()); + // target-dev install is registered dynamically by sdk/install.rs + // after fetching extensions and discovering compile sections + crate::utils::tui::set_active_renderer(&r); + r.start(); + Some(r) + } else { + None + }; // 1. Install SDK dependencies if let Some(ref r) = renderer { @@ -429,8 +435,10 @@ impl InstallCommand { let sched_renderer = renderer .clone() .unwrap_or_else(|| Arc::new(TaskRenderer::new(true))); - // Without TUI (no --force), run tasks sequentially so each - // interactive prompt gets exclusive stdin access. + // Without a renderer, run tasks sequentially: parallel tasks write + // to the same terminal, and interleaved dnf output is unreadable. + // (It used to be about giving each prompt exclusive stdin; installs + // no longer prompt.) let effective_parallel = if renderer.is_some() { max_parallel } else { 1 }; let mut scheduler = TaskScheduler::new(graph, sched_renderer, effective_parallel); diff --git a/src/commands/rootfs/install.rs b/src/commands/rootfs/install.rs index d1d63e3d..5ee43080 100644 --- a/src/commands/rootfs/install.rs +++ b/src/commands/rootfs/install.rs @@ -1014,7 +1014,10 @@ pub async fn install_sysroot(params: &mut SysrootInstallParams<'_>) -> Result<() } let pkg = pkg_specs.join(" "); - let yes = if params.force { "-y" } else { "" }; + // dnf never prompts here: this applies the package set avocado.yaml and + // avocado.lock already declare, so there is no decision left to make. + // `sdk dnf` / `ext dnf` / `runtime dnf` are the interactive path. + let yes = "-y"; let dnf_args_str = if let Some(args) = ¶ms.dnf_args { format!(" {} ", args.join(" ")) } else { @@ -1598,6 +1601,41 @@ mod tests { names.iter().map(|n| n.to_string()).collect() } + /// Installs must never wait on a dnf prompt. + /// + /// This used to depend on `--force`, which also clears every extension + /// sysroot and drops its stamps — so the only way to avoid the prompt was + /// to pay a full rebuild, and the documented invocation (`install -f`) did + /// exactly that on every iteration. A prompt here also hangs CI and the + /// TUI, which is why the renderer was gated on `--force` too. The `yes` + /// argument is now a constant at all five install call sites; this pins the + /// step that consumes it. + #[test] + fn dnf_sync_step_passes_assume_yes() { + use crate::commands::rootfs::install::dnf_sync_step; + let step = dnf_sync_step(true, "rootfs", "", "-y", ""); + assert!(step.contains("-y"), "expected -y in: {step}"); + // And the sources agree, across every install site. One exact string in + // one file was too narrow: the coupling can come back in any of the five + // and can be spelled several ways, so match the shape instead — the + // assume-yes literal and a flag on the same line. + for (name, src) in [ + ("rootfs/install.rs", include_str!("install.rs")), + ("install.rs", include_str!("../install.rs")), + ("sdk/install.rs", include_str!("../sdk/install.rs")), + ("runtime/install.rs", include_str!("../runtime/install.rs")), + ("ext/install.rs", include_str!("../ext/install.rs")), + ] { + for (n, line) in src.lines().enumerate() { + assert!( + !(line.contains("\"-y\"") && line.contains("force")), + "{name}:{} derives the assume-yes flag from a flag: {line}", + n + 1 + ); + } + } + } + #[test] fn fresh_resolve_adds_a_distro_sync_after_install_and_pins_do_not() { use super::dnf_sync_step; diff --git a/src/commands/runtime/install.rs b/src/commands/runtime/install.rs index f4b2dbab..b3256c37 100644 --- a/src/commands/runtime/install.rs +++ b/src/commands/runtime/install.rs @@ -687,7 +687,10 @@ impl RuntimeInstallCommand { OutputLevel::Normal, ); - let yes = if self.force { "-y" } else { "" }; + // dnf never prompts here: this applies the package set avocado.yaml and + // avocado.lock already declare, so there is no decision left to make. + // `sdk dnf` / `ext dnf` / `runtime dnf` are the interactive path. + let yes = "-y"; let dnf_args_str = if let Some(args) = &self.dnf_args { format!(" {} ", args.join(" ")) } else { diff --git a/src/commands/sdk/install.rs b/src/commands/sdk/install.rs index 231dd56d..4b3c983e 100644 --- a/src/commands/sdk/install.rs +++ b/src/commands/sdk/install.rs @@ -385,7 +385,10 @@ impl SdkInstallCommand { all_compile_package_names.sort(); all_compile_package_names.dedup(); - let yes = if self.force { "-y" } else { "" }; + // dnf never prompts here: this applies the package set avocado.yaml and + // avocado.lock already declare, so there is no decision left to make. + // `sdk dnf` / `ext dnf` / `runtime dnf` are the interactive path. + let yes = "-y"; let dnf_args_str = if let Some(args) = &self.dnf_args { format!(" {} ", args.join(" ")) } else { @@ -1868,7 +1871,10 @@ fi let mut all_sdk_package_names: Vec = bootstrap_package_names.to_vec(); if !sdk_packages.is_empty() { - let yes = if self.force { "-y" } else { "" }; + // dnf never prompts here: this applies the package set avocado.yaml and + // avocado.lock already declare, so there is no decision left to make. + // `sdk dnf` / `ext dnf` / `runtime dnf` are the interactive path. + let yes = "-y"; let dnf_args_str = if let Some(args) = &self.dnf_args { format!(" {} ", args.join(" ")) } else { diff --git a/src/main.rs b/src/main.rs index de38a414..7e630f72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -279,7 +279,12 @@ enum Commands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Reinstall extensions from scratch: clear every extension's sysroot + /// and re-seed it. + /// + /// Not needed to skip dnf's prompts — installs never prompt. Forcing + /// discards every extension's built content, so the next build has to + /// redo all of it. #[arg(short, long)] force: bool, /// Runtime name to install packages into (or sync when no packages given) @@ -1652,7 +1657,11 @@ enum SdkCommands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Install the SDK, rootfs, initramfs and target-dev sysroots in + /// parallel rather than one at a time. + /// + /// Clears nothing. Not needed to skip dnf's prompts — installs never + /// prompt. #[arg(short, long)] force: bool, /// Target architecture @@ -1726,7 +1735,11 @@ enum RuntimeCommands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Run non-interactively: no live checklist, and the container is + /// started without a TTY. + /// + /// Clears nothing. Not needed to skip dnf's prompts — installs never + /// prompt. #[arg(short, long)] force: bool, /// Runtime name (deprecated, use positional argument) @@ -2250,12 +2263,12 @@ async fn main() -> Result<()> { } => { let _json_guard = run_with_json_lifecycle(output, "install", target.as_deref(), runtime.as_deref()); - // JSON output implies no human at the keyboard — auto-enable - // --force so dnf gets -y and container starts without -it. - // Without this, `docker run -it` fails ("cannot attach stdin - // to a TTY-enabled container") and dnf hangs waiting for - // confirmation. - let force = force || output.is_json(); + // No JSON-implies-force coercion. Both reasons for it are gone: + // installs always pass `-y`, and `utils::interactivity` decides the + // container's stdio flags from what the environment can actually + // support, so `docker run -it` no longer fails without a tty. With + // `--force` meaning reinstall-from-scratch, keeping the coercion + // turned `--output json` into a full rebuild. if packages.is_empty() { // No packages specified: sync all from config (original behavior) validate_runtime_if_provided(&config, runtime.as_ref())?; @@ -2479,9 +2492,8 @@ async fn main() -> Result<()> { target.as_deref(), name.as_deref().or(runtime.as_deref()), ); - // JSON output implies no human at the keyboard — auto-enable - // --force (see `Install` arm above for rationale). - let force = force || output.is_json(); + // No JSON-implies-force coercion here either (see the `Install` + // arm above for why both of its reasons are gone). let runtime = resolve_runtime_at_path(&config, name.as_deref().or(runtime.as_deref()))?; let provision_cmd = @@ -4441,7 +4453,12 @@ enum ExtCommands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Reinstall from scratch: clear this extension's sysroot and re-seed + /// it. + /// + /// Not needed to skip dnf's prompts — installs never prompt. Forcing + /// discards the extension's built content, so the next build has to + /// redo it. #[arg(short, long)] force: bool, /// Extension name (deprecated, use positional argument) @@ -4697,7 +4714,10 @@ enum RootfsCommands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Run non-interactively: the container is started without a TTY. + /// + /// Clears nothing. Not needed to skip dnf's prompts — installs never + /// prompt. #[arg(short, long)] force: bool, /// Target architecture @@ -4764,7 +4784,10 @@ enum InitramfsCommands { /// Enable verbose output #[arg(short, long)] verbose: bool, - /// Force the operation to proceed, bypassing warnings or confirmation prompts + /// Run non-interactively: the container is started without a TTY. + /// + /// Clears nothing. Not needed to skip dnf's prompts — installs never + /// prompt. #[arg(short, long)] force: bool, /// Target architecture @@ -5192,6 +5215,31 @@ mod tests { /// NOT route — routing can auto-start the VM, which is wrong for e.g. /// `connect auth`/`deploy`. Pins the Docker-socket fix against regressions /// (dropping the upload arm, or over-broadly routing all of `Connect`). + /// `--output json` must not imply `--force`. + /// + /// It did, for two reasons that are both gone: the TUI renderer was gated on + /// `--force` because dnf could prompt without it, and `docker run -it` + /// failed with no tty. Installs now always pass `-y`, and + /// `utils::interactivity` decides the container's stdio from what the + /// environment can actually support. With `--force` meaning + /// reinstall-from-scratch, the coercion turned a request for machine-readable + /// output into a full rebuild on every invocation. + /// + /// The needle is assembled at runtime so this assertion does not match its + /// own source. + #[test] + fn json_output_does_not_imply_force() { + let src = include_str!("main.rs"); + let needle = format!("force {}", "|| output.is_json()"); + for (n, line) in src.lines().enumerate() { + assert!( + !line.contains(&needle), + "main.rs:{} makes --output json imply --force: {line}", + n + 1 + ); + } + } + #[test] fn needs_vm_routing_gates_connect_upload_only() { let cmd = |args: &[&str]| {