diff --git a/README.md b/README.md index 4e4f232..53fa05e 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,7 @@ alt="patch-hub-demo-v0.1.0"> - **Apply patch(set)** to your local kernel tree. - **Bookmark** important patches - **Reply with `Reviewed-by` tags** to the series. + - **KwOps** (`w`) β€” build and remotely deploy the applied tree with `kw`. - **Bookmarking System** β€” Bookmark patchsets for easy reference later. @@ -98,12 +99,25 @@ alt="patch-hub-demo-v0.1.0"> visualization or use the built-in vanilla renderer (`default`) for a dependency-free experience. -### More Features Coming! +### KwOps (build and deploy with kw) -Future updates will introduce deeper integration with kw, including: +From a patchset's Details screen, press `w` to open **Kw operations** and +build or remotely deploy the applied tree with `kw`. -- Seamlessly compile and deploy patchset versions of the kernel to target - machines. +- **Build** (`b`), **Deploy** (`d`), and **Build then deploy** (`D`). +- Live logs while a job runs β€” you can leave the screen without freezing the + TUI. +- **Cancel** (`c`) a running job, or **Restore previous branch** (`r`) after + a job switched `HEAD`. Press `?` for in-screen help. + +KwOps uses your existing `kw` setup: [`kw`](https://github.com/kworkflow/kworkflow/) +0.10 or newer on `PATH`, a `.kw/` directory in the kernel tree (`kw init` / +`kw env`), and for deploy a remote plus `kw deploy --setup`. Local deploy is +not available from the UI. Extra `kw` arguments can be typed per run; they +are not saved. + +Related settings in Edit Config / `config.json`: `stay_on_applied_branch`, +`kw_reboot_after_deploy`, and `kw_deploy_force`. ## :package: How To Install diff --git a/src/app/actions/apply.rs b/src/app/actions/apply.rs index bca3849..85c2d32 100644 --- a/src/app/actions/apply.rs +++ b/src/app/actions/apply.rs @@ -58,7 +58,13 @@ pub(crate) fn apply_patchset( } Err(e) => { switch_to_branch(shell, kernel_tree, &original_branch)?; - Err(format!(" `git am` failed\n{}{}", &original_branch, e)) + let cleanup_note = match delete_branch(shell, kernel_tree, &target_branch) { + Ok(()) => String::new(), + Err(error) => format!("\n\n(could not delete '{target_branch}': {error})"), + }; + Err(format!( + " `git am` failed (back on branch '{original_branch}')\n{e}{cleanup_note}" + )) } } } @@ -218,6 +224,30 @@ fn create_target_branch( Ok(target_branch_name) } +/// Deletes a branch `create_target_branch` made for an apply that failed. +/// `-D` because `git am --abort` leaves it at the base commit, which is not +/// necessarily merged into the branch HEAD switched back to. +fn delete_branch( + shell: &dyn ShellTrait, + kernel_tree: &KernelTree, + branch: &str, +) -> Result<(), String> { + let out = shell + .execute( + &ShellCommand::new("git") + .arg("-C") + .arg(kernel_tree.path()) + .args(["branch", "-D", branch]), + ) + .map_err(|e| e.to_string())?; + + if !out.success { + return Err(String::from_utf8_lossy(&out.stderr).trim().to_string()); + } + + Ok(()) +} + fn run_git_am( request: &ApplyPatchsetRequest, shell: &dyn ShellTrait, @@ -515,14 +545,15 @@ mod tests { output("", "apply failed", false), output("", "", true), output("", "", true), + output("", "", true), ]); let result = apply_patchset(&request(), &fs, &shell, &config).unwrap_err(); - assert!(result.contains("`git am` failed")); - assert!(result.contains("feature")); - assert!(result.contains("apply failed")); + assert!(result.starts_with(" `git am` failed (back on branch 'feature')\napply failed")); + assert!(!result.contains("could not delete")); let calls = calls.lock().unwrap(); + assert_eq!(9, calls.len()); assert_eq!( &calls[6], &command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]) @@ -531,6 +562,32 @@ mod tests { &calls[7], &command(&["git", "-C", KERNEL_TREE_PATH, "switch", "feature"]) ); + assert_eq!( + &calls[8], + &command(&["git", "-C", KERNEL_TREE_PATH, "branch", "-D", &calls[4][5]]) + ); } } + + #[test] + fn failed_branch_cleanup_is_noted_without_hiding_the_git_am_error() { + let fs = clean_fs(); + let (shell, _calls) = shell_with_outputs(vec![ + output("", "", true), + output("", "", true), + output("feature\n", "", true), + output("", "", true), + output("", "", true), + output("", "apply failed", false), + output("", "", true), + output("", "", true), + output("", "error: branch is locked\n", false), + ]); + + let result = apply_patchset(&request(), &fs, &shell, &config()).unwrap_err(); + + assert!(result.starts_with(" `git am` failed (back on branch 'feature')\napply failed")); + assert!(result.contains("could not delete 'patchset-")); + assert!(result.contains("error: branch is locked)")); + } } diff --git a/src/app/flows/edit_config.rs b/src/app/flows/edit_config.rs index 73a04b7..2c491d0 100644 --- a/src/app/flows/edit_config.rs +++ b/src/app/flows/edit_config.rs @@ -36,6 +36,12 @@ pub async fn handle_edit_config(app: &mut App, input: InputEvent) -> Result<()> edit_config_state.clear_edit(); edit_config_state.toggle_editing(); } + InputEvent::NavigateLeft => { + edit_config_state.cycle_edit(false); + } + InputEvent::NavigateRight => { + edit_config_state.cycle_edit(true); + } _ => {} } } @@ -75,12 +81,224 @@ pub async fn handle_edit_config(app: &mut App, input: InputEvent) -> Result<()> pub fn generate_help_popup() -> AppPopup { AppPopup::help() .title("Edit Config") - .description("This screen allows you to edit the configuration options for patch-hub.\nMore configurations may be available in the configuration file.") - .keybind("ESC", "Exit") - .keybind("ENTER", "Save changes") + .description("This screen allows you to edit the configuration options for patch-hub.\nKernel trees are added by editing the config file; this screen selects among existing keys.") + .keybind("ESC / q", "Save and exit") + .keybind("ENTER", "Edit the highlighted option") .keybind("?", "Show this help screen") .keybind("j/πŸ‘‡", "Down") .keybind("k/πŸ‘…", "Up") - .keybind("e", "Toggle editing for a configuration option") + .keybind("←/β†’", "Cycle the target kernel tree while editing that row") .build() } + +#[cfg(test)] +mod tests { + use std::{ + collections::HashMap, + env::VarError, + fs, + path::PathBuf, + sync::{ + atomic::{AtomicU64, Ordering}, + Arc, + }, + }; + + use tokio::sync::mpsc; + + use crate::{ + app::{ + screens::{ + bookmarked::BookmarkedPatchsetsState, mail_list::MailingListSelectionState, + CurrentScreen, + }, + state::{AppState, ConfigUiState, LoreUiState, NavigationState, UserLoreState}, + App, AppServices, + }, + config::{bootstrap_parts, ConfigActor, ConfigHandle, DEFAULT_CONFIG_PATH_SUFFIX}, + infrastructure::{ + env::MockEnvTrait, file_system::MockFileSystemTrait, file_system::OsFileSystem, + shell::MockShellTrait, + }, + kw::history::MockKwHistoryStore, + lore::{application::handle::LoreApiHandle, domain::mailing_list::MailingList}, + render::handle::RenderHandle, + }; + + use super::*; + + static TEST_SEQ: AtomicU64 = AtomicU64::new(0); + + fn unique_test_dir(prefix: &str) -> PathBuf { + let n = TEST_SEQ.fetch_add(1, Ordering::SeqCst); + let p = std::env::temp_dir().join(format!( + "patch-hub-edit-config-{prefix}-{}-{n}", + std::process::id() + )); + fs::create_dir_all(&p).unwrap(); + p + } + + fn default_env() -> (MockEnvTrait, PathBuf) { + let home = unique_test_dir("home"); + let home_s = home.to_string_lossy().into_owned(); + let mut mock = MockEnvTrait::new(); + mock.expect_var() + .withf(|key| key == "PATCH_HUB_CONFIG_PATH") + .returning(|_| Err(VarError::NotPresent.into())); + mock.expect_var() + .withf(move |key| key == "HOME") + .returning(move |_| Ok(home_s.clone())); + mock.expect_var() + .withf(|key| { + matches!( + key, + "PATCH_HUB_PAGE_SIZE" + | "PATCH_HUB_CACHE_DIR" + | "PATCH_HUB_DATA_DIR" + | "PATCH_HUB_GIT_SEND_EMAIL_OPTIONS" + | "PATCH_HUB_PATCH_RENDERER" + ) + }) + .returning(|_| Err(VarError::NotPresent.into())); + (mock, home) + } + + fn app_with_kernel_trees(keys: &[&str], target: Option<&str>) -> (App, ConfigHandle, PathBuf) { + let (env, home) = default_env(); + let (mut state, repo) = bootstrap_parts(&env, OsFileSystem).unwrap(); + for key in keys { + state.kernel_trees.insert( + (*key).to_string(), + serde_json::from_value(serde_json::json!({ + "path": format!("/{key}"), + "branch": "master" + })) + .unwrap(), + ); + } + state.target_kernel_tree = target.map(str::to_string); + let snapshot = state.to_snapshot(); + let config = ConfigActor::spawn(state, repo); + + let dummy_list = MailingList::new("test-list", "Test list"); + let (lore_tx, _lore_rx) = mpsc::channel(1); + let (render_tx, _render_rx) = mpsc::channel(1); + let app = App { + state: AppState { + navigation: NavigationState { + current_screen: CurrentScreen::EditConfig, + }, + lore: LoreUiState { + mailing_list_selection: MailingListSelectionState { + mailing_lists: vec![dummy_list.clone()], + target_list: String::new(), + possible_mailing_lists: vec![dummy_list], + highlighted_list_index: 0, + }, + latest_patchsets: None, + details: None, + }, + user_state: UserLoreState { + bookmarked_patchsets: BookmarkedPatchsetsState { + bookmarked_patchsets: vec![], + patchset_index: 0, + }, + reviewed_patchsets: HashMap::new(), + }, + config_state: ConfigUiState { edit_config: None }, + config: snapshot, + popup: None, + kw: Default::default(), + }, + services: AppServices { + lore_api: LoreApiHandle::new(lore_tx), + render: RenderHandle::new(render_tx), + shell: Box::new(MockShellTrait::new()), + fs: Arc::new(MockFileSystemTrait::new()), + config: config.clone(), + kw_history: Arc::new(MockKwHistoryStore::new()), + kw: None, + }, + }; + (app, config, home) + } + + #[test] + fn help_documents_tree_cycle_and_save_keys() { + let AppPopup::Help { + description, + formatted_keybinds, + .. + } = generate_help_popup() + else { + panic!("expected help popup"); + }; + assert!(description + .as_deref() + .is_some_and(|text| text.contains("selects among existing keys"))); + assert!(formatted_keybinds.contains("ESC / q: Save and exit")); + assert!(formatted_keybinds.contains("ENTER: Edit the highlighted option")); + assert!(formatted_keybinds.contains("←/β†’: Cycle the target kernel tree")); + assert!(!formatted_keybinds.contains("Save changes")); + } + + #[tokio::test] + async fn cycling_the_tree_row_and_saving_updates_the_snapshot() { + let (mut app, config, home) = app_with_kernel_trees(&["linux", "zebra"], None); + app.init_edit_config(); + + for _ in 0..11 { + handle_edit_config(&mut app, InputEvent::NavigateDown) + .await + .unwrap(); + } + handle_edit_config(&mut app, InputEvent::EditConfigField) + .await + .unwrap(); + handle_edit_config(&mut app, InputEvent::NavigateRight) + .await + .unwrap(); + handle_edit_config(&mut app, InputEvent::StageConfigEdit) + .await + .unwrap(); + handle_edit_config(&mut app, InputEvent::SaveConfig) + .await + .unwrap(); + + assert_eq!( + Some("linux"), + app.state.config.target_kernel_tree().as_deref() + ); + assert!(app.state.config_state.edit_config.is_none()); + assert_eq!( + CurrentScreen::MailingListSelection, + app.state.navigation.current_screen + ); + + let raw = fs::read_to_string(home.join(DEFAULT_CONFIG_PATH_SUFFIX)).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert_eq!(parsed["target_kernel_tree"], "linux"); + config.shutdown().await; + } + + #[tokio::test] + async fn saving_with_a_dangling_target_unsets_it_instead_of_crashing() { + let (mut app, config, home) = app_with_kernel_trees(&["zebra"], Some("linux")); + assert_eq!( + Some("linux"), + app.state.config.target_kernel_tree().as_deref() + ); + app.init_edit_config(); + + handle_edit_config(&mut app, InputEvent::SaveConfig) + .await + .unwrap(); + + assert!(app.state.config.target_kernel_tree().is_none()); + let raw = fs::read_to_string(home.join(DEFAULT_CONFIG_PATH_SUFFIX)).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert!(parsed["target_kernel_tree"].is_null()); + config.shutdown().await; + } +} diff --git a/src/app/integration_tests/patchset_actions.rs b/src/app/integration_tests/patchset_actions.rs index e5fd293..881d1b8 100644 --- a/src/app/integration_tests/patchset_actions.rs +++ b/src/app/integration_tests/patchset_actions.rs @@ -121,6 +121,7 @@ async fn apply_failure_sets_failure_popup_and_resets_apply_action() { output("", "apply failed", false), output("", "", true), output("", "", true), + output("", "", true), ]); let mut app = app_with_apply_details(clean_fs(), shell); @@ -130,7 +131,7 @@ async fn apply_failure_sets_failure_popup_and_resets_apply_action() { assert_info_popup_contains( app.state.popup.as_ref(), "Patchset Apply Fail", - &["`git am` failed", "feature", "apply failed"], + &["`git am` failed (back on branch 'feature')", "apply failed"], ); { let calls = calls.lock().unwrap(); @@ -138,6 +139,10 @@ async fn apply_failure_sets_failure_popup_and_resets_apply_action() { command(&["git", "-C", KERNEL_TREE_PATH, "am", "--abort"]), calls[6] ); + assert_eq!( + &calls[8][..5], + command(&["git", "-C", KERNEL_TREE_PATH, "branch", "-D"]) + ); } shutdown_kw(&app).await; } @@ -235,6 +240,7 @@ async fn apply_failure_does_not_record_history() { output("", "apply failed", false), output("", "", true), output("", "", true), + output("", "", true), ]); let mut kw_history = MockKwHistoryStore::new(); kw_history.expect_record_apply().times(0); diff --git a/src/app/screens/edit_config.rs b/src/app/screens/edit_config.rs index 0f9905b..d4ac283 100644 --- a/src/app/screens/edit_config.rs +++ b/src/app/screens/edit_config.rs @@ -13,6 +13,8 @@ use crate::config::{ConfigSnapshot, ConfigUpdateDraft}; pub struct EditConfigState { #[getter(skip)] config_buffer: HashMap, + #[getter(skip)] + cycle_options: Vec, highlighted: usize, is_editing: bool, curr_edit: String, @@ -54,8 +56,33 @@ impl EditConfigState { config.kw_deploy_force().to_string(), ); + let mut keys: Vec = config.kernel_trees().into_iter().cloned().collect(); + keys.sort(); + let raw_target = config + .target_kernel_tree() + .as_ref() + .map(|target| target.trim().to_string()) + .unwrap_or_default(); + let target = if raw_target.is_empty() { + String::new() + } else if keys.iter().any(|key| key == &raw_target) { + raw_target + } else { + tracing::warn!( + target = %raw_target, + "configured target is not a known kernel tree; treating as unset" + ); + String::new() + }; + config_buffer.insert(EditableConfig::TargetKernelTree, target); + + let mut cycle_options = Vec::with_capacity(keys.len() + 1); + cycle_options.push(String::new()); + cycle_options.extend(keys); + EditConfigState { config_buffer, + cycle_options, highlighted: 0, is_editing: false, curr_edit: String::new(), @@ -72,12 +99,48 @@ impl EditConfigState { EditableConfig::try_from(i) .ok() .and_then(|editable_config| { - self.config_buffer - .get(&editable_config) - .map(|value| (editable_config.to_string(), value.clone())) + self.config_buffer.get(&editable_config).map(|value| { + let display = if editable_config == EditableConfig::TargetKernelTree + && value.is_empty() + { + "".to_string() + } else { + value.clone() + }; + (editable_config.to_string(), display) + }) }) } + pub fn highlighted_is_tree_selector(&self) -> bool { + matches!( + EditableConfig::try_from(self.highlighted), + Ok(EditableConfig::TargetKernelTree) + ) + } + + /// Cycles the tree selector through `` and the sorted tree keys. + /// No-op unless the highlighted row is the tree selector and editing. + pub fn cycle_edit(&mut self, forward: bool) { + if !self.is_editing || !self.highlighted_is_tree_selector() { + return; + } + + let current = self + .cycle_options + .iter() + .position(|key| key == &self.curr_edit) + .unwrap_or(0); + let next = if forward { + (current + 1) % self.cycle_options.len() + } else if current == 0 { + self.cycle_options.len() - 1 + } else { + current - 1 + }; + self.curr_edit = self.cycle_options[next].clone(); + } + /// Toggle editing mode pub fn toggle_editing(&mut self) { if !self.is_editing { @@ -106,6 +169,9 @@ impl EditConfigState { /// Remove the last char from the current editing value if not empty pub fn backspace_edit(&mut self) { + if self.highlighted_is_tree_selector() { + return; + } if !self.curr_edit.is_empty() { self.curr_edit.pop(); } @@ -113,6 +179,9 @@ impl EditConfigState { /// Appends a new char to the current editing value pub fn append_edit(&mut self, ch: char) { + if self.highlighted_is_tree_selector() { + return; + } self.curr_edit.push(ch); } @@ -161,6 +230,10 @@ impl EditConfigState { .config_buffer .get(&EditableConfig::KwDeployForce) .cloned(), + target_kernel_tree: self + .config_buffer + .get(&EditableConfig::TargetKernelTree) + .cloned(), } } } @@ -178,6 +251,7 @@ enum EditableConfig { StayOnAppliedBranch, KwRebootAfterDeploy, KwDeployForce, + TargetKernelTree, } impl TryFrom for EditableConfig { @@ -196,6 +270,7 @@ impl TryFrom for EditableConfig { 8 => Ok(EditableConfig::StayOnAppliedBranch), 9 => Ok(EditableConfig::KwRebootAfterDeploy), 10 => Ok(EditableConfig::KwDeployForce), + 11 => Ok(EditableConfig::TargetKernelTree), _ => bail!("Invalid index {} for EditableConfig", value), // Handle out of bounds } } @@ -225,6 +300,9 @@ impl Display for EditableConfig { EditableConfig::KwDeployForce => { write!(f, "Force kw Deploy (true/false)") } + EditableConfig::TargetKernelTree => { + write!(f, "Target Kernel Tree (ENTER, then ←/β†’ to cycle)") + } } } } @@ -234,6 +312,28 @@ mod tests { use super::*; use crate::config::ConfigState; + fn snapshot_with_trees(keys: &[&str], target: Option<&str>) -> ConfigSnapshot { + let mut state = ConfigState::default(); + for key in keys { + state.kernel_trees.insert( + (*key).to_string(), + serde_json::from_value(serde_json::json!({ + "path": format!("/{key}"), + "branch": "master" + })) + .unwrap(), + ); + } + state.target_kernel_tree = target.map(str::to_string); + state.to_snapshot() + } + + fn tree_row(edit: &mut EditConfigState) { + while edit.highlighted() != 11 { + edit.highlight_next(); + } + } + #[test] fn draft_includes_deploy_knobs_with_compiled_in_defaults() { let snapshot = ConfigState::default().to_snapshot(); @@ -242,7 +342,8 @@ mod tests { assert_eq!(Some("false".to_string()), draft.kw_reboot_after_deploy); assert_eq!(Some("true".to_string()), draft.kw_deploy_force); - assert_eq!(11, edit.config_count()); + assert_eq!(Some(String::new()), draft.target_kernel_tree); + assert_eq!(12, edit.config_count()); assert_eq!( Some(( "Reboot After kw Deploy (true/false)".to_string(), @@ -257,5 +358,124 @@ mod tests { )), edit.config(10) ); + assert_eq!( + Some(( + "Target Kernel Tree (ENTER, then ←/β†’ to cycle)".to_string(), + "".to_string() + )), + edit.config(11) + ); + } + + #[test] + fn cycle_edit_wraps_across_none_and_sorted_keys() { + let snapshot = snapshot_with_trees(&["zebra", "linux"], Some("linux")); + let mut edit = EditConfigState::new(&snapshot); + tree_row(&mut edit); + edit.toggle_editing(); + assert_eq!("linux", edit.curr_edit()); + + edit.cycle_edit(true); + assert_eq!("zebra", edit.curr_edit()); + edit.cycle_edit(true); + assert_eq!("", edit.curr_edit()); + edit.cycle_edit(true); + assert_eq!("linux", edit.curr_edit()); + + edit.cycle_edit(false); + assert_eq!("", edit.curr_edit()); + edit.cycle_edit(false); + assert_eq!("zebra", edit.curr_edit()); + } + + #[test] + fn cycle_edit_is_noop_when_not_editing_or_on_other_rows() { + let snapshot = snapshot_with_trees(&["linux"], Some("linux")); + let mut edit = EditConfigState::new(&snapshot); + tree_row(&mut edit); + edit.cycle_edit(true); + assert_eq!("", edit.curr_edit()); + assert_eq!( + Some(( + "Target Kernel Tree (ENTER, then ←/β†’ to cycle)".to_string(), + "linux".to_string() + )), + edit.config(11) + ); + + edit.highlighted = 0; + edit.toggle_editing(); + let page_size = edit.curr_edit().to_string(); + edit.cycle_edit(true); + assert_eq!(page_size, edit.curr_edit().as_str()); + } + + #[test] + fn text_input_is_ignored_on_the_tree_selector() { + let snapshot = snapshot_with_trees(&["linux"], Some("linux")); + let mut edit = EditConfigState::new(&snapshot); + tree_row(&mut edit); + edit.toggle_editing(); + edit.append_edit('x'); + edit.backspace_edit(); + assert_eq!("linux", edit.curr_edit()); + } + + #[test] + fn staged_tree_selection_reaches_the_draft() { + let snapshot = snapshot_with_trees(&["linux", "zebra"], Some("linux")); + let mut edit = EditConfigState::new(&snapshot); + tree_row(&mut edit); + edit.toggle_editing(); + edit.cycle_edit(true); + edit.stage_edit(); + edit.toggle_editing(); + assert_eq!( + Some("zebra".to_string()), + edit.to_update_draft().target_kernel_tree + ); + + tree_row(&mut edit); + edit.toggle_editing(); + edit.cycle_edit(true); + edit.stage_edit(); + edit.toggle_editing(); + assert_eq!( + Some(String::new()), + edit.to_update_draft().target_kernel_tree + ); + assert_eq!( + Some(( + "Target Kernel Tree (ENTER, then ←/β†’ to cycle)".to_string(), + "".to_string() + )), + edit.config(11) + ); + } + + #[test] + fn dangling_target_is_seeded_as_unset() { + let snapshot = snapshot_with_trees(&["zebra"], Some("linux")); + let edit = EditConfigState::new(&snapshot); + let draft = edit.to_update_draft(); + + assert_eq!(Some(String::new()), draft.target_kernel_tree); + assert_eq!( + Some(( + "Target Kernel Tree (ENTER, then ←/β†’ to cycle)".to_string(), + "".to_string() + )), + edit.config(11) + ); + } + + #[test] + fn padded_target_is_trimmed_to_a_known_key() { + let snapshot = snapshot_with_trees(&["linux"], Some(" linux ")); + let edit = EditConfigState::new(&snapshot); + assert_eq!( + Some("linux".to_string()), + edit.to_update_draft().target_kernel_tree + ); } } diff --git a/src/app/view_model.rs b/src/app/view_model.rs index eddf272..eb2a37a 100644 --- a/src/app/view_model.rs +++ b/src/app/view_model.rs @@ -125,6 +125,7 @@ pub struct PatchsetDetailsViewModel { pub struct EditConfigViewModel { pub entries: Vec, pub is_editing_mode: bool, + pub editing_tree_selector: bool, } /// KwOps dashboard. Labels only; actions stay in AppState. @@ -153,6 +154,12 @@ pub struct KwOpsViewModel { pub deploy_label: String, pub build_deploy_label: String, pub branch_guidance: Option, + /// Deploy warnings of the last job, joined with ` | `. + pub warnings: Option, + /// First error line of the last failed job's log. + pub first_error: Option, + /// Full log of the current or last job; the pane only shows its tail. + pub log_path: Option, pub log_tail: String, } @@ -192,7 +199,9 @@ pub enum ScreenViewModel { Latest(LatestPatchsetsViewModel), PatchsetDetails(PatchsetDetailsViewModel), EditConfig(EditConfigViewModel), - KwOps(KwOpsViewModel), + // Boxed to keep the enum small (clippy::large_enum_variant): KwOps + // carries one owned string per dashboard row. + KwOps(Box), } /// Owned, typed projection of [`AppState`] for one TUI frame. @@ -235,7 +244,7 @@ fn project_screen(state: &AppState) -> ScreenViewModel { CurrentScreen::LatestPatchsets => ScreenViewModel::Latest(project_latest(state)), CurrentScreen::PatchsetDetails => ScreenViewModel::PatchsetDetails(project_details(state)), CurrentScreen::EditConfig => ScreenViewModel::EditConfig(project_edit_config(state)), - CurrentScreen::KwOps => ScreenViewModel::KwOps(project_kw_ops(state)), + CurrentScreen::KwOps => ScreenViewModel::KwOps(Box::new(project_kw_ops(state))), } } @@ -417,6 +426,7 @@ fn project_edit_config(state: &AppState) -> EditConfigViewModel { let is_editing_mode = ec.is_editing(); let highlighted = ec.highlighted(); + let editing_tree_selector = is_editing_mode && ec.highlighted_is_tree_selector(); let entries = (0..ec.config_count()) .filter_map(|i| { @@ -424,7 +434,11 @@ fn project_edit_config(state: &AppState) -> EditConfigViewModel { let is_highlighted = i == highlighted; let is_editing = is_editing_mode && is_highlighted; let edit_cursor_value = if is_editing { - ec.curr_edit().to_string() + if editing_tree_selector && ec.curr_edit().is_empty() { + "".to_string() + } else { + ec.curr_edit().to_string() + } } else { String::new() }; @@ -442,6 +456,7 @@ fn project_edit_config(state: &AppState) -> EditConfigViewModel { EditConfigViewModel { entries, is_editing_mode, + editing_tree_selector, } } @@ -505,6 +520,17 @@ fn project_kw_ops(state: &AppState) -> KwOpsViewModel { (false, Some(branch)) => format!("available (r) to {branch}"), (false, None) => "unavailable".to_string(), }; + let job = state.kw.status.as_ref().map(|status| &status.job); + let (warnings, first_error) = match job { + Some(KwJobStatus::Succeeded { warnings, .. }) if !warnings.is_empty() => { + (Some(warnings.join(" | ")), None) + } + Some(KwJobStatus::Failed { first_error, .. }) => (None, first_error.clone()), + _ => (None, None), + }; + let log_path = job + .and_then(KwJobStatus::log_path) + .map(|path| path.display().to_string()); let log_tail = if ops.log_tail.is_empty() { if running { "Waiting for kw output…".to_string() @@ -535,10 +561,7 @@ fn project_kw_ops(state: &AppState) -> KwOpsViewModel { job_status: if start_requested && !running { "starting…".to_string() } else { - format_job_status( - state.kw.status.as_ref().map(|status| &status.job), - ops.cancel_requested, - ) + format_job_status(job, ops.cancel_requested) }, command: format!( "kw {}", @@ -565,6 +588,9 @@ fn project_kw_ops(state: &AppState) -> KwOpsViewModel { } else { None }, + warnings, + first_error, + log_path, log_tail, } } @@ -674,7 +700,13 @@ fn format_job_status(job: Option<&KwJobStatus>, cancel_requested: bool) -> Strin format!("{phase} {branch}") } } - Some(KwJobStatus::Succeeded { branch, .. }) => format!("succeeded on {branch}"), + Some(KwJobStatus::Succeeded { + branch, warnings, .. + }) => match warnings.len() { + 0 => format!("succeeded on {branch}"), + 1 => format!("succeeded on {branch} with 1 warning"), + count => format!("succeeded on {branch} with {count} warnings"), + }, Some(KwJobStatus::Failed { phase: KwPhase::Deploying, exit_code, @@ -844,6 +876,35 @@ mod tests { assert_eq!(1, selected); } + #[test] + fn edit_config_projects_none_placeholder_on_the_tree_row() { + let mut state = app_state_with_kw(None); + state.navigation.current_screen = CurrentScreen::EditConfig; + let mut config = ConfigState::default(); + config.kernel_trees.insert( + "linux".into(), + serde_json::from_value(serde_json::json!({ + "path": "/linux", + "branch": "master" + })) + .unwrap(), + ); + state.config = config.to_snapshot(); + let mut edit = crate::app::screens::edit_config::EditConfigState::new(&state.config); + while edit.highlighted() != 11 { + edit.highlight_next(); + } + edit.toggle_editing(); + state.config_state.edit_config = Some(edit); + + let ScreenViewModel::EditConfig(vm) = project_state(&state).screen else { + panic!("expected EditConfig projection"); + }; + assert!(vm.editing_tree_selector); + assert_eq!("", vm.entries[11].edit_cursor_value); + assert_eq!("", vm.entries[11].value); + } + #[test] fn boot_once_popup_projects_back_out_as_default() { let mut state = app_state_with_kw(None); @@ -1093,10 +1154,12 @@ mod tests { fn deploy_command_follows_reboot_and_force_config() { let mut state = app_state_with_kw(None); state.navigation.current_screen = CurrentScreen::KwOps; - let mut config = ConfigState::default(); - config.kw_reboot_after_deploy = true; - config.kw_deploy_force = false; - state.config = config.to_snapshot(); + state.config = ConfigState { + kw_reboot_after_deploy: true, + kw_deploy_force: false, + ..Default::default() + } + .to_snapshot(); let mut ops = sample_kw_ops(Some("feature")); ops.readiness.deploy_remote = Ok(sample_remote()); state.kw.ops = Some(ops); @@ -1115,6 +1178,7 @@ mod tests { phase: KwPhase::Deploying, exit_code: Some(101), log_path: PathBuf::from("/tmp/deploy.log"), + first_error: None, }, restore_branch: None, })); @@ -1140,6 +1204,7 @@ mod tests { phase: KwPhase::Building, exit_code: Some(1), log_path: PathBuf::from("/tmp/build.log"), + first_error: None, }, restore_branch: None, })); @@ -1150,6 +1215,106 @@ mod tests { panic!("expected KwOps projection"); }; assert_eq!("failed (exit 1)", vm.job_status); + assert_eq!(None, vm.first_error); + assert_eq!(Some("/tmp/build.log".to_string()), vm.log_path); + } + + #[test] + fn failed_build_projects_first_error_and_log_path() { + let mut state = app_state_with_kw(Some(KwStatusSnapshot { + job: KwJobStatus::Failed { + kind: KwJobKind::Build, + phase: KwPhase::Building, + exit_code: Some(2), + log_path: PathBuf::from("/tmp/build.log"), + first_error: Some("init/main.c:1:2: error: #error broken".to_string()), + }, + restore_branch: None, + })); + state.navigation.current_screen = CurrentScreen::KwOps; + state.kw.ops = Some(sample_kw_ops(Some("feature"))); + + let ScreenViewModel::KwOps(vm) = project_state(&state).screen else { + panic!("expected KwOps projection"); + }; + assert_eq!("failed (exit 2)", vm.job_status); + assert_eq!( + Some("init/main.c:1:2: error: #error broken".to_string()), + vm.first_error + ); + assert_eq!(Some("/tmp/build.log".to_string()), vm.log_path); + assert_eq!(None, vm.warnings); + } + + fn succeeded_deploy(warnings: &[&str]) -> KwStatusSnapshot { + KwStatusSnapshot { + job: KwJobStatus::Succeeded { + kind: KwJobKind::Deploy, + kernel_tree_id: "mainline".to_string(), + branch: "patchset-x".to_string(), + log_path: PathBuf::from("/tmp/deploy.log"), + warnings: warnings.iter().map(|w| w.to_string()).collect(), + }, + restore_branch: None, + } + } + + #[test] + fn deploy_with_warnings_projects_count_and_joined_warnings() { + let mut state = app_state_with_kw(Some(succeeded_deploy(&[ + "update-initramfs: failed for /boot/initrd.img-7.2.0 with 1.", + "GRUB did not list kernel 7.2.0", + ]))); + state.navigation.current_screen = CurrentScreen::KwOps; + state.kw.ops = Some(sample_kw_ops(Some("feature"))); + + let ScreenViewModel::KwOps(vm) = project_state(&state).screen else { + panic!("expected KwOps projection"); + }; + assert_eq!("succeeded on patchset-x with 2 warnings", vm.job_status); + assert_eq!( + Some( + "update-initramfs: failed for /boot/initrd.img-7.2.0 with 1. | \ + GRUB did not list kernel 7.2.0" + .to_string() + ), + vm.warnings + ); + assert_eq!(None, vm.first_error); + assert_eq!(Some("/tmp/deploy.log".to_string()), vm.log_path); + } + + #[test] + fn clean_success_projects_no_warning_rows() { + let mut state = app_state_with_kw(Some(succeeded_deploy(&[]))); + state.navigation.current_screen = CurrentScreen::KwOps; + state.kw.ops = Some(sample_kw_ops(Some("feature"))); + + let ScreenViewModel::KwOps(vm) = project_state(&state).screen else { + panic!("expected KwOps projection"); + }; + assert_eq!("succeeded on patchset-x", vm.job_status); + assert_eq!(None, vm.warnings); + + state.kw.status = Some(succeeded_deploy(&["GRUB did not list kernel 7.2.0"])); + let ScreenViewModel::KwOps(vm) = project_state(&state).screen else { + panic!("expected KwOps projection"); + }; + assert_eq!("succeeded on patchset-x with 1 warning", vm.job_status); + } + + #[test] + fn idle_job_projects_no_log_path() { + let mut state = app_state_with_kw(Some(KwStatusSnapshot::idle())); + state.navigation.current_screen = CurrentScreen::KwOps; + state.kw.ops = Some(sample_kw_ops(Some("feature"))); + + let ScreenViewModel::KwOps(vm) = project_state(&state).screen else { + panic!("expected KwOps projection"); + }; + assert_eq!(None, vm.log_path); + assert_eq!(None, vm.warnings); + assert_eq!(None, vm.first_error); } #[test] @@ -1167,6 +1332,7 @@ mod tests { kernel_tree_id: "mainline".to_string(), branch: "patchset-x".to_string(), log_path: PathBuf::from("/tmp/build.log"), + warnings: Vec::new(), }, restore_branch: Some("master".to_string()), }))) diff --git a/src/config/actor.rs b/src/config/actor.rs index e321fc3..2eb64bb 100644 --- a/src/config/actor.rs +++ b/src/config/actor.rs @@ -84,7 +84,7 @@ where } fn apply(&mut self, draft: ConfigUpdateDraft) -> ConfigResult { - let update = validate_update(draft, self.repo.fs())?; + let update = validate_update(draft, self.repo.fs(), &self.state)?; self.state.apply_update(&update); normalize_derived_paths(&mut self.state); ensure_directories(&self.state, self.repo.fs())?; diff --git a/src/config/errors.rs b/src/config/errors.rs index 7c8dbe0..97a0cf9 100644 --- a/src/config/errors.rs +++ b/src/config/errors.rs @@ -22,6 +22,8 @@ pub enum ConfigError { InvalidKwRebootAfterDeploy(String), #[error("invalid kw deploy-force value: {0}")] InvalidKwDeployForce(String), + #[error("unknown kernel tree key '{key}'; {hint}")] + InvalidTargetKernelTree { key: String, hint: String }, #[error("filesystem error: {0}")] Fs(#[from] FileSystemError), } diff --git a/src/config/service.rs b/src/config/service.rs index b5c0ab6..cbf38e9 100644 --- a/src/config/service.rs +++ b/src/config/service.rs @@ -67,6 +67,7 @@ pub(crate) fn ensure_directories( pub(crate) fn validate_update( draft: ConfigUpdateDraft, fs: &dyn FileSystemTrait, + current: &ConfigState, ) -> Result { let page_size = match &draft.page_size { None => None, @@ -169,6 +170,19 @@ pub(crate) fn validate_update( ), }; + let target_kernel_tree = match &draft.target_kernel_tree { + None => None, + Some(s) if s.trim().is_empty() => Some(None), + Some(s) => { + let key = s.trim(); + if current.kernel_trees.contains_key(key) { + Some(Some(key.to_string())) + } else { + return Err(unknown_target_kernel_tree(s, current)); + } + } + }; + Ok(ValidatedConfigUpdate { page_size, cache_dir, @@ -181,9 +195,28 @@ pub(crate) fn validate_update( stay_on_applied_branch, kw_reboot_after_deploy, kw_deploy_force, + target_kernel_tree, }) } +fn unknown_target_kernel_tree(raw: &str, current: &ConfigState) -> ConfigError { + let mut keys: Vec = current.kernel_trees.keys().cloned().collect(); + keys.sort(); + let hint = if keys.is_empty() { + "no kernel trees are configured; unset the target or add trees in the config file" + .to_string() + } else { + format!( + "known keys: {}; unset the target or pick one of these", + keys.join(", ") + ) + }; + ConfigError::InvalidTargetKernelTree { + key: raw.to_string(), + hint, + } +} + fn validate_dir(fs: &dyn FileSystemTrait, dir_path: &str) -> Result<(), ConfigError> { let path = Path::new(dir_path); if fs.exists(path) && fs.is_dir(path) { diff --git a/src/config/state.rs b/src/config/state.rs index b5e7c5f..b0f7ff3 100644 --- a/src/config/state.rs +++ b/src/config/state.rs @@ -144,6 +144,10 @@ impl ConfigState { self.kw_deploy_force = kw_deploy_force; } + fn set_target_kernel_tree(&mut self, target_kernel_tree: Option) { + self.target_kernel_tree = target_kernel_tree; + } + /// Merges validated field updates from the edit-config flow. pub fn apply_update(&mut self, u: &ValidatedConfigUpdate) { if let Some(page_size) = u.page_size { @@ -179,6 +183,9 @@ impl ConfigState { if let Some(kw_deploy_force) = u.kw_deploy_force { self.set_kw_deploy_force(kw_deploy_force); } + if let Some(ref target_kernel_tree) = u.target_kernel_tree { + self.set_target_kernel_tree(target_kernel_tree.clone()); + } } pub fn to_snapshot(&self) -> ConfigSnapshot { @@ -252,7 +259,6 @@ impl ConfigSnapshot { self.page_size } - #[allow(dead_code)] pub fn kernel_trees(&self) -> HashSet<&String> { self.kernel_trees.keys().collect::>() } diff --git a/src/config/tests.rs b/src/config/tests.rs index 220db04..33ce226 100644 --- a/src/config/tests.rs +++ b/src/config/tests.rs @@ -12,7 +12,7 @@ use crate::config::repository::{ConfigRepository, JsonConfigRepository}; use crate::config::service::{bootstrap_parts, validate_update}; use crate::config::state::{normalize_derived_paths, ConfigState}; use crate::config::{ - ConfigError, ConfigSnapshot, ConfigUpdateDraft, ValidatedConfigUpdate, + ConfigError, ConfigSnapshot, ConfigUpdateDraft, KernelTree, ValidatedConfigUpdate, DEFAULT_CONFIG_PATH_SUFFIX, }; use crate::infrastructure::{ @@ -26,6 +26,27 @@ fn os_fs() -> OsFileSystem { OsFileSystem } +fn sample_kernel_tree(path: &str, branch: &str) -> KernelTree { + serde_json::from_value(json!({ + "path": path, + "branch": branch + })) + .unwrap() +} + +fn state_with_trees(env: &dyn EnvTrait) -> ConfigState { + let mut state = ConfigState::new_with_defaults(env); + state.kernel_trees.insert( + "linux".into(), + sample_kernel_tree("/home/user/linux", "master"), + ); + state.kernel_trees.insert( + "amd-gfx".into(), + sample_kernel_tree("/home/user/amd-gfx", "amd-staging-drm-next"), + ); + state +} + fn unique_test_dir(prefix: &str) -> PathBuf { let n = TEST_SEQ.fetch_add(1, Ordering::SeqCst); let p = std::env::temp_dir().join(format!("patch-hub-{prefix}-{}-{}", std::process::id(), n)); @@ -494,6 +515,7 @@ fn validate_update_rejects_invalid_page_size() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!(err, ConfigError::InvalidPageSize(ref s) if s == "xyz")); @@ -507,6 +529,7 @@ fn validate_update_rejects_invalid_patch_renderer() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!( @@ -523,6 +546,7 @@ fn validate_update_rejects_invalid_cover_renderer() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!( @@ -539,6 +563,7 @@ fn validate_update_rejects_invalid_max_log_age() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!( @@ -556,6 +581,7 @@ fn validate_update_rejects_invalid_stay_on_applied_branch() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!( @@ -597,6 +623,7 @@ fn validate_update_rejects_invalid_kw_deploy_bools() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); if expect_reboot_err { @@ -636,6 +663,83 @@ fn apply_update_toggles_kw_deploy_knobs() { assert!(!state.kw_deploy_force()); } +#[test] +fn validate_update_accepts_existing_target_kernel_tree() { + let (env, _home) = default_env(); + let state = state_with_trees(&env); + let update = validate_update( + ConfigUpdateDraft { + target_kernel_tree: Some("linux".into()), + ..Default::default() + }, + &os_fs(), + &state, + ) + .unwrap(); + assert_eq!(Some(Some("linux".to_string())), update.target_kernel_tree); +} + +#[test] +fn validate_update_unsets_target_kernel_tree_on_empty_string() { + let (env, _home) = default_env(); + let state = state_with_trees(&env); + let update = validate_update( + ConfigUpdateDraft { + target_kernel_tree: Some("".into()), + ..Default::default() + }, + &os_fs(), + &state, + ) + .unwrap(); + assert_eq!(Some(None), update.target_kernel_tree); +} + +#[test] +fn validate_update_rejects_unknown_target_kernel_tree() { + let (env, _home) = default_env(); + let state = state_with_trees(&env); + let err = validate_update( + ConfigUpdateDraft { + target_kernel_tree: Some("missing".into()), + ..Default::default() + }, + &os_fs(), + &state, + ) + .unwrap_err(); + assert!(matches!( + err, + ConfigError::InvalidTargetKernelTree { ref key, .. } if key == "missing" + )); + let message = err.to_string(); + assert!(message.contains("known keys: amd-gfx, linux")); + assert!(message.contains("unset the target")); +} + +#[test] +fn apply_update_sets_and_unsets_target_kernel_tree() { + let (env, _home) = default_env(); + let mut state = state_with_trees(&env); + assert!(state.target_kernel_tree().is_none()); + + state.apply_update(&ValidatedConfigUpdate { + target_kernel_tree: Some(Some("linux".into())), + ..Default::default() + }); + assert_eq!(Some("linux"), state.target_kernel_tree().as_deref()); + + state.apply_update(&ValidatedConfigUpdate { + target_kernel_tree: Some(None), + ..Default::default() + }); + assert!(state.target_kernel_tree().is_none()); + + // A draft that omits the field leaves the current value untouched. + state.apply_update(&ValidatedConfigUpdate::default()); + assert!(state.target_kernel_tree().is_none()); +} + #[test] fn validate_update_rejects_cache_dir_that_is_existing_file() { let root = unique_test_dir("not-a-dir"); @@ -647,6 +751,7 @@ fn validate_update_rejects_cache_dir_that_is_existing_file() { ..Default::default() }, &os_fs(), + &ConfigState::default(), ) .unwrap_err(); assert!(matches!(err, ConfigError::InvalidDirectory(_))); @@ -705,3 +810,74 @@ async fn validate_and_apply_persists_to_config_file() { assert_eq!(parsed["page_size"], 77); handle.shutdown().await; } + +#[tokio::test] +async fn validate_and_apply_persists_target_kernel_tree() { + let (env, home) = default_env(); + let (mut state, repo) = bootstrap_parts(&env, os_fs()).unwrap(); + state.kernel_trees.insert( + "linux".into(), + sample_kernel_tree("/home/user/linux", "master"), + ); + let handle = ConfigActor::spawn(state, repo); + let cfg_path = home.join(DEFAULT_CONFIG_PATH_SUFFIX); + + let snapshot = handle + .validate_and_apply(ConfigUpdateDraft { + target_kernel_tree: Some("linux".into()), + ..Default::default() + }) + .await + .unwrap(); + assert_eq!(Some("linux"), snapshot.target_kernel_tree().as_deref()); + + let raw = fs::read_to_string(&cfg_path).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert_eq!(parsed["target_kernel_tree"], "linux"); + + handle + .validate_and_apply(ConfigUpdateDraft { + target_kernel_tree: Some("".into()), + ..Default::default() + }) + .await + .unwrap(); + let raw = fs::read_to_string(&cfg_path).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&raw).unwrap(); + assert!(parsed["target_kernel_tree"].is_null()); + handle.shutdown().await; +} + +#[tokio::test] +async fn invalid_target_kernel_tree_keeps_existing_state() { + let (env, _home) = default_env(); + let (mut state, repo) = bootstrap_parts(&env, os_fs()).unwrap(); + state.kernel_trees.insert( + "linux".into(), + sample_kernel_tree("/home/user/linux", "master"), + ); + state.target_kernel_tree = Some("linux".into()); + let handle = ConfigActor::spawn(state, repo); + + let err = handle + .validate_and_apply(ConfigUpdateDraft { + target_kernel_tree: Some("missing".into()), + ..Default::default() + }) + .await + .unwrap_err(); + assert!(matches!( + err, + ConfigError::InvalidTargetKernelTree { ref key, .. } if key == "missing" + )); + assert_eq!( + Some("linux"), + handle + .get_snapshot() + .await + .unwrap() + .target_kernel_tree() + .as_deref() + ); + handle.shutdown().await; +} diff --git a/src/config/update.rs b/src/config/update.rs index 44b14dc..2ee3b47 100644 --- a/src/config/update.rs +++ b/src/config/update.rs @@ -14,6 +14,8 @@ pub struct ConfigUpdateDraft { pub stay_on_applied_branch: Option, pub kw_reboot_after_deploy: Option, pub kw_deploy_force: Option, + /// Raw `kernel_trees` key. `None` omits the field; empty string unsets. + pub target_kernel_tree: Option, } /// Parsed and validated update ready to merge into [`crate::config::ConfigState`](super::state::ConfigState). @@ -30,4 +32,7 @@ pub struct ValidatedConfigUpdate { pub stay_on_applied_branch: Option, pub kw_reboot_after_deploy: Option, pub kw_deploy_force: Option, + /// `None` leaves the current target unchanged; `Some(None)` unsets it; + /// `Some(Some(key))` selects an existing `kernel_trees` key. + pub target_kernel_tree: Option>, } diff --git a/src/input/mapper.rs b/src/input/mapper.rs index 0120eea..4ea219a 100644 --- a/src/input/mapper.rs +++ b/src/input/mapper.rs @@ -124,6 +124,8 @@ impl InputMapper { return match key.code { KeyCode::Esc => Some(InputEvent::CancelConfigEdit), KeyCode::Backspace => Some(InputEvent::Backspace), + KeyCode::Left => Some(InputEvent::NavigateLeft), + KeyCode::Right => Some(InputEvent::NavigateRight), KeyCode::Char(ch) => Some(InputEvent::TextInput(ch)), KeyCode::Enter => Some(InputEvent::StageConfigEdit), _ => None, @@ -352,6 +354,39 @@ mod tests { assert_eq!(event, Some(InputEvent::EditConfigField)); } + #[test] + fn maps_arrows_to_navigate_while_editing_config_and_keeps_hl_as_text() { + let mut mapper = InputMapper::default(); + let editing = context(CurrentScreen::EditConfig).with_edit_config_editing(true); + + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Left), &editing), + Some(InputEvent::NavigateLeft) + ); + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Right), &editing), + Some(InputEvent::NavigateRight) + ); + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Char('h')), &editing), + Some(InputEvent::TextInput('h')) + ); + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Char('l')), &editing), + Some(InputEvent::TextInput('l')) + ); + + let browsing = context(CurrentScreen::EditConfig); + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Left), &browsing), + None + ); + assert_eq!( + mapper.map_terminal_event(key(KeyCode::Right), &browsing), + None + ); + } + #[test] fn ignores_key_release_events() { let mut mapper = InputMapper::default(); diff --git a/src/kw/actor.rs b/src/kw/actor.rs index 674bc05..e2bfa2b 100644 --- a/src/kw/actor.rs +++ b/src/kw/actor.rs @@ -38,6 +38,7 @@ use crate::{ errors::{KwError, KwStartError, TreeGitError}, handle::KwHandle, history::{KwBuildRecord, KwHistoryStore}, + log_scan, messages::{DeployOptions, KwMessage, StartRequest}, readiness::{self, BootOnceState, KwReadiness, KwVersionCheck, TreeReadiness}, remote::{self, KwRemote}, @@ -109,6 +110,10 @@ struct JobDeploy { remote: KwRemote, options: DeployOptions, extra_args: Vec, + /// Release of the kernel being deployed, from the build record, so + /// the deploy log can be checked for a GRUB menu that never listed + /// it. `None` until a BuildThenDeploy build has been recorded. + kernelrelease: Option, } pub struct KwActor { @@ -615,9 +620,23 @@ impl KwActor { self.record_build_outcome(&job, &outcome); let status = match outcome { JobOutcome::Exited(exit) if exit.success() => { + // kw deploy exits 0 through initramfs and GRUB + // failures; only its log tells. + let warnings = if job.phase == KwPhase::Deploying { + let kernelrelease = job + .deploy + .as_ref() + .and_then(|deploy| deploy.kernelrelease.as_deref()); + self.read_job_log(&job.log_path) + .map(|log| log_scan::deploy_warnings(&log, kernelrelease)) + .unwrap_or_default() + } else { + Vec::new() + }; tracing::info!( kernel_tree_id = job.kernel_tree_id, branch = job.branch, + warnings = warnings.len(), "kw job succeeded" ); KwJobStatus::Succeeded { @@ -625,6 +644,7 @@ impl KwActor { kernel_tree_id: job.kernel_tree_id, branch: job.branch, log_path: job.log_path, + warnings, } } JobOutcome::Exited(exit) => { @@ -638,6 +658,9 @@ impl KwActor { kind: job.kind, phase: job.phase, exit_code: exit.code(), + first_error: self + .read_job_log(&job.log_path) + .and_then(|log| log_scan::first_error(&log)), log_path: job.log_path, } } @@ -647,6 +670,9 @@ impl KwActor { kind: job.kind, phase: job.phase, exit_code: None, + first_error: self + .read_job_log(&job.log_path) + .and_then(|log| log_scan::first_error(&log)), log_path: job.log_path, } } @@ -689,17 +715,19 @@ impl KwActor { // Durable before the phase flips: a deploy spawn failure must not // lose the successful build, and KwOps watching Deploying must // already see the record. - self.record_build_outcome(&job, outcome); - let Some(deploy) = job.deploy.clone() else { + let kernelrelease = self.record_build_outcome(&job, outcome); + let Some(mut deploy) = job.deploy.clone() else { tracing::error!("BuildThenDeploy missing deploy context after a successful build"); self.set_status(KwJobStatus::Failed { kind: job.kind, phase: KwPhase::Deploying, exit_code: None, log_path: job.log_path, + first_error: None, }); return ControlFlow::Break(()); }; + deploy.kernelrelease = kernelrelease; match self.spawn_deploy_process(&job.tree_path, &deploy) { Ok((process, log_path)) => { let (cancel_tx, cancel_rx) = oneshot::channel(); @@ -720,6 +748,7 @@ impl KwActor { let mut job = job; job.phase = KwPhase::Deploying; job.log_path = log_path; + job.deploy = Some(deploy); job.cancel_tx = Some(cancel_tx); self.job = Some(job); ControlFlow::Break(()) @@ -735,6 +764,7 @@ impl KwActor { phase: KwPhase::Deploying, exit_code: None, log_path: job.log_path, + first_error: None, }); ControlFlow::Break(()) } @@ -744,16 +774,17 @@ impl KwActor { /// Persist a finished build (success or failure). Cancel writes /// nothing. History and patchset-link errors are logged, not folded /// into job status β€” the build's real outcome already reached the user. - fn record_build_outcome(&self, job: &JobState, outcome: &JobOutcome) { + /// Returns the recorded kernelrelease, if the build produced one. + fn record_build_outcome(&self, job: &JobState, outcome: &JobOutcome) -> Option { if job.kind == KwJobKind::Deploy || job.phase == KwPhase::Deploying { - return; + return None; } let success = match outcome { JobOutcome::Exited(exit) => exit.success(), // The exit status is lost: record an honest failure rather // than guess, so deploy-alone readiness cannot trust it. JobOutcome::WaitFailed(_) => false, - JobOutcome::Cancelled => return, + JobOutcome::Cancelled => return None, }; // The record describes the accept-time snapshot: the build ran @@ -802,7 +833,7 @@ impl KwActor { .output_dir .as_ref() .map(|path| path.to_string_lossy().into_owned()), - kernelrelease, + kernelrelease: kernelrelease.clone(), log_path: job.log_path.to_string_lossy().into_owned(), built_at: chrono::Utc::now().to_rfc3339(), success, @@ -810,6 +841,20 @@ impl KwActor { if let Err(error) = self.history.record_build(record) { tracing::warn!(%error, branch = job.branch, "failed to record kw build history"); } + kernelrelease + } + + /// Reads a finished job's log for the findings its terminal status + /// carries. An unreadable log only loses those findings: the exit code + /// is still the verdict. + fn read_job_log(&self, log_path: &Path) -> Option { + match self.fs.read_to_string(log_path) { + Ok(log) => Some(log), + Err(error) => { + tracing::warn!(%error, log_path = %log_path.display(), "failed to read kw job log"); + None + } + } } fn set_status(&mut self, job: KwJobStatus) { @@ -928,6 +973,7 @@ fn prepare_deploy_blocking( }); let remote = remote::resolve_deploy_remote(fs, env, tree_path) .map_err(KwStartError::RemoteUnresolved)?; + let mut kernelrelease = None; if kind == KwJobKind::Deploy { let (record, latest) = history.build_records(&request.kernel_tree_id, &request.branch)?; let image = readiness::find_newest_kernel_image(fs, output_dir.unwrap_or(tree_path), arch); @@ -940,6 +986,7 @@ fn prepare_deploy_blocking( image.as_deref(), ) .map_err(KwStartError::DeployAloneRefused)?; + kernelrelease = record.and_then(|record| record.kernelrelease); } let boot_once = readiness::probe_boot_once(fs, env, tree_path); if matches!(boot_once, BootOnceState::On | BootOnceState::Unknown) @@ -951,6 +998,7 @@ fn prepare_deploy_blocking( remote, options, extra_args: request.extra_args.clone(), + kernelrelease, }) } @@ -1397,11 +1445,13 @@ mod tests { fs.expect_is_file() .returning(|path| !path.ends_with(".kw/env.current")); fs.expect_exists().returning(|_| true); - fs.expect_read_to_string().returning(|_| { - Err(FileSystemError::IoError(io::Error::new( - io::ErrorKind::NotFound, - "missing", - ))) + fs.expect_read_to_string().returning(|path| { + read_real_job_log(path).unwrap_or_else(|| { + Err(FileSystemError::IoError(io::Error::new( + io::ErrorKind::NotFound, + "missing", + ))) + }) }); fs.expect_read_dir().returning(|_| { Err(FileSystemError::IoError(io::Error::new( @@ -1411,6 +1461,14 @@ mod tests { }); } + /// Job logs are real files ([`FakeProcess`] creates them and + /// `write_log` appends to them), so fs doubles read `*.log` paths from + /// disk. + fn read_real_job_log(path: &Path) -> Option> { + (path.extension() == Some("log".as_ref())) + .then(|| std::fs::read_to_string(path).map_err(FileSystemError::from)) + } + /// A ready kernel tree whose log dir can be created. fn ready_fs() -> MockFileSystemTrait { let mut fs = MockFileSystemTrait::new(); @@ -1480,7 +1538,9 @@ mod tests { .returning(|path| !path.ends_with(".kw/env.current")); fs.expect_exists().returning(|_| true); fs.expect_read_to_string().returning(move |path| { - if path.ends_with("build.config") { + if let Some(log) = read_real_job_log(path) { + log + } else if path.ends_with("build.config") { Ok("arch=x86\n".to_string()) } else if path.ends_with("kernel.release") { Ok("6.17.0\n".to_string()) @@ -1981,6 +2041,11 @@ mod tests { let mut watch = handle.watch_status().await.unwrap(); handle.start_build(start_request()).await.unwrap(); + process.last_child().write_log( + b" CC init/main.o\n\ + init/main.c:1691:2: error: #error broken\n\ + make: *** [Makefile:248: __sub-make] Error 2\n", + ); process.last_child().finish(2); let status = wait_for_terminal_status(&mut watch).await; @@ -1991,11 +2056,16 @@ mod tests { phase, exit_code, log_path, + first_error, } => { assert_eq!(KwJobKind::Build, kind); assert_eq!(KwPhase::Building, phase); assert_eq!(Some(2), exit_code); assert!(log_path.starts_with(&log_dir)); + assert_eq!( + Some("init/main.c:1691:2: error: #error broken"), + first_error.as_deref() + ); } other => panic!("expected Failed, got {other:?}"), } @@ -2354,6 +2424,78 @@ mod tests { std::fs::remove_dir_all(&log_dir).unwrap(); } + #[tokio::test] + async fn deploy_exit_zero_with_initramfs_failure_succeeds_with_warnings() { + let (handle, process, log_dir) = spawn_deploy_actor( + "deploy-warnings", + deploy_history(Some(matching_build_record())), + deploy_ready_fs(), + ); + let mut watch = handle.watch_status().await.unwrap(); + + handle.start_deploy(deploy_request()).await.unwrap(); + process.last_child().write_log( + b"update-initramfs: Generating /boot/initrd.img-6.17.0\n\ + E: gzip compression (CONFIG_RD_GZIP) not supported by kernel\n\ + update-initramfs: failed for /boot/initrd.img-6.17.0 with 1.\n\ + Generating grub configuration file ...\n\ + Found linux image: /boot/vmlinuz-6.8.0-generic\n\ + done\n", + ); + process.last_child().finish(0); + + match wait_for_terminal_status(&mut watch).await { + KwJobStatus::Succeeded { kind, warnings, .. } => { + assert_eq!(KwJobKind::Deploy, kind); + assert_eq!( + vec![ + "update-initramfs: failed for /boot/initrd.img-6.17.0 with 1.".to_string(), + "GRUB did not list kernel 6.17.0".to_string(), + ], + warnings + ); + } + other => panic!("expected Succeeded, got {other:?}"), + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + + #[tokio::test] + async fn build_then_deploy_checks_grub_for_the_release_it_just_built() { + let (handle, process, log_dir) = + spawn_deploy_actor("chain-warnings", quiet_history(), deploy_ready_fs()); + let mut watch = handle.watch_status().await.unwrap(); + + handle + .start_build_then_deploy(deploy_request()) + .await + .unwrap(); + process.last_child().finish(0); + wait_for_running_phase(&mut watch, KwPhase::Deploying).await; + process.last_child().write_log( + b"Generating grub configuration file ...\n\ + Found linux image: /boot/vmlinuz-6.8.0-generic\n\ + Found linux image: /boot/Image-6.17.0-rc1\n\ + done\n", + ); + process.last_child().finish(0); + + match wait_for_terminal_status(&mut watch).await { + KwJobStatus::Succeeded { warnings, .. } => { + assert_eq!( + vec!["GRUB did not list kernel 6.17.0".to_string()], + warnings + ); + } + other => panic!("expected Succeeded, got {other:?}"), + } + + handle.shutdown().await; + std::fs::remove_dir_all(&log_dir).unwrap(); + } + #[tokio::test] async fn start_deploy_merges_extras_and_follows_reboot_force_options() { let (handle, process, log_dir) = spawn_deploy_actor( @@ -3428,8 +3570,12 @@ mod tests { let env_current_reads = Arc::new(AtomicU64::new(0)); let env_current_reads_in_fs = Arc::clone(&env_current_reads); let mut fs = MockFileSystemTrait::new(); - fs.expect_is_dir().returning(|_| true); - fs.expect_is_file().returning(|_| true); + // With an env active the build artifacts live only under O=; the + // source tree is clean. + fs.expect_is_dir() + .returning(|path| path != Path::new("/home/user/linux/include/config")); + fs.expect_is_file() + .returning(|path| path != Path::new("/home/user/linux/.config")); fs.expect_exists().returning(|_| true); fs.expect_read_to_string().returning(move |path| { if path.ends_with("env.current") { diff --git a/src/kw/argv.rs b/src/kw/argv.rs index d2ce4b5..6b398f9 100644 --- a/src/kw/argv.rs +++ b/src/kw/argv.rs @@ -267,7 +267,7 @@ fn reserved_short_cluster<'a>( fn exact_short<'a>(reserved: &'a [ReservedOption], spelling: &str) -> Option<&'a ReservedOption> { reserved .iter() - .find(|option| option.spellings.iter().any(|s| *s == spelling)) + .find(|option| option.spellings.contains(&spelling)) } fn value_is_attached(token: &str, option: &ReservedOption) -> bool { diff --git a/src/kw/log_scan.rs b/src/kw/log_scan.rs new file mode 100644 index 0000000..bc2fc69 --- /dev/null +++ b/src/kw/log_scan.rs @@ -0,0 +1,318 @@ +//! Scans a finished job's log for what the exit code alone does not say: +//! the first real error of a failed job, and the known non-fatal failures +//! `kw deploy` exits 0 through. +//! +//! Pure functions over the log text; the actor reads the file. + +// The only production caller is the unix-only actor. +#![cfg_attr(not(unix), allow(dead_code))] + +/// Longer lines (deep include paths, long make targets) are cut so one +/// finding stays a single KwOps row. +const MAX_LINE_CHARS: usize = 200; +const MAX_DEPLOY_WARNINGS: usize = 5; + +/// Lines `kw deploy` prints for a failure it does not turn into a non-zero +/// exit: initramfs generation (Debian `update-initramfs`, Arch +/// `mkinitcpio`) and kw's own bootloader/boot-once setup. +const DEPLOY_WARNING_MARKERS: [&str; 4] = [ + "update-initramfs: failed", + "==> ERROR:", + "kw was unable to set up the first boot", + "kw did not find grub.cfg", +]; + +/// The first line of a failed job's log that names what went wrong: a +/// compiler/linker `error:`, a modpost `ERROR:`, a kbuild `*** ...` +/// banner (e.g. "The source tree is not clean"), or a git/kw `error:`. +/// Falls back to the last `make: *** ... Error N` line, which only names +/// the failing target. Kbuild prints the real cause first and a cascade +/// of make errors after it, so the first match is the useful one. +pub fn first_error(log: &str) -> Option { + log.lines() + .map(str::trim) + .find(|line| is_error_line(line)) + .or_else(|| log.lines().map(str::trim).rfind(|line| is_make_error(line))) + .map(cap_line) +} + +/// Known failures in a deploy log that `kw deploy` exits 0 through, at +/// most [`MAX_DEPLOY_WARNINGS`]. With `kernelrelease`, a GRUB update that +/// did not list that kernel is reported too: kw installs arm64 kernels as +/// `Image-`, which Debian's `10_linux` does not pick up, so the +/// deployed kernel is never bootable from the menu. +pub fn deploy_warnings(log: &str, kernelrelease: Option<&str>) -> Vec { + let mut warnings: Vec = Vec::new(); + for line in log.lines().map(str::trim) { + if DEPLOY_WARNING_MARKERS + .iter() + .any(|marker| line.contains(marker)) + { + let line = cap_line(line); + if !warnings.contains(&line) { + warnings.push(line); + } + } + } + let unlisted = kernelrelease.filter(|release| grub_missed_release(log, release)); + warnings.truncate(MAX_DEPLOY_WARNINGS - usize::from(unlisted.is_some())); + if let Some(release) = unlisted { + warnings.push(format!("GRUB did not list kernel {release}")); + } + warnings +} + +fn is_error_line(line: &str) -> bool { + line.contains("fatal error:") + || line.contains(" error:") + || line.starts_with("error:") + || line.starts_with("ERROR:") + || line + .strip_prefix("*** ") + .is_some_and(|banner| !banner.trim().is_empty()) +} + +/// `make[N]: *** [target] Error N`, the summary make prints per failing +/// level. +fn is_make_error(line: &str) -> bool { + line.starts_with("make") + && line.contains(": *** ") + && line + .rsplit_once("Error ") + .is_some_and(|(_, code)| !code.is_empty() && code.chars().all(|c| c.is_ascii_digit())) +} + +/// GRUB ran (`Generating grub configuration file`) but no `Found linux +/// image:` line names an image for `release`. Matched on the `-` +/// suffix so `7.2.0-rc6` is not satisfied by `Image-7.2.0-rc6-phboot`. +fn grub_missed_release(log: &str, release: &str) -> bool { + if !log.contains("Generating grub configuration file") { + return false; + } + let suffix = format!("-{release}"); + !log.lines().map(str::trim).any(|line| { + line.strip_prefix("Found linux image:") + .is_some_and(|image| image.trim().ends_with(&suffix)) + }) +} + +fn cap_line(line: &str) -> String { + match line.char_indices().nth(MAX_LINE_CHARS) { + Some((index, _)) => format!("{}…", &line[..index]), + None => line.to_string(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// `kw build` under an env `O=` with in-tree leftovers in the source + /// tree (lab/logs/20260920-122005). + const UNCLEAN_TREE_BUILD_LOG: &str = "\ +/boot/config-6.8.0-138-generic:883:warning: symbol value '0' invalid for BASE_SMALL + SYNC include/config/auto.conf.cmd +*** +*** The source tree is not clean, please run 'make ARCH=arm64 mrproper' +*** in /opt/ph-lab/work/linux +*** +make[3]: *** [/opt/ph-lab/work/linux/Makefile:709: outputmakefile] Error 1 +make[2]: *** [/opt/ph-lab/work/linux/Makefile:906: include/config/auto.conf.cmd] Error 2 +make[2]: *** [include/config/auto.conf.cmd] Deleting file 'include/generated/rustc_cfg' +make[2]: *** [include/config/auto.conf.cmd] Deleting file 'include/generated/autoconf.h' +make[1]: *** [/opt/ph-lab/work/linux/Makefile:248: __sub-make] Error 2 +make[1]: Leaving directory '/opt/ph-lab/work/out' +"; + + /// The M4 fixture's compile break (lab/logs/20260920-151635/M18). + const COMPILE_ERROR_BUILD_LOG: &str = "\ + CC mm/filemap.o + AS arch/arm64/kernel/entry.o +/opt/ph-lab/work/linux/init/main.c:1691:2: error: #error M4 broken-build fixture + 1691 | #error M4 broken-build fixture + | ^~~~~ +make[4]: *** [/opt/ph-lab/work/linux/scripts/Makefile.build:289: init/main.o] Error 1 +make[3]: *** [/opt/ph-lab/work/linux/scripts/Makefile.build:549: init] Error 2 +make[3]: *** Waiting for unfinished jobs.... + CC kernel/exec_domain.o +make: *** [Makefile:248: __sub-make] Error 2 +"; + + /// tinyconfig deploy to the Ubuntu DUT: kw exits 0 although + /// initramfs generation failed and GRUB never listed the kernel + /// (lab/logs/20261003-111949). + const TINYCONFIG_DEPLOY_LOG: &str = "\ +cp /tmp/kw/kw_pkg/Image-7.2.0-rc6+ /boot/ +generate_debian_temporary_root_file_system VERBOSE 7.2.0-rc6+ remote GRUB +update-initramfs -c -k 7.2.0-rc6+ +update-initramfs: Generating /boot/initrd.img-7.2.0-rc6+ +W: zstd compression (CONFIG_RD_ZSTD) not supported by kernel, using gzip +E: gzip compression (CONFIG_RD_GZIP) not supported by kernel +update-initramfs: failed for /boot/initrd.img-7.2.0-rc6+ with 1. +update-grub +Sourcing file `/etc/default/grub' +Generating grub configuration file ... +Found linux image: /boot/vmlinuz-6.8.0-137-generic +Found initrd image: /boot/initrd.img-6.8.0-137-generic +Found linux image: /boot/vmlinuz-6.8.0-136-generic +Found initrd image: /boot/initrd.img-6.8.0-136-generic +Adding boot menu entry for UEFI Firmware Settings ... +done +"; + + /// The real-boot deploy of the tipc patch: everything worked + /// (lab/logs/20261003-154745-real-boot-tui-tipc). + const CLEAN_DEPLOY_LOG: &str = "\ +* Preparing modules +cp: cannot stat '/home/lima.guest/.cache/kw/envs/L29wdC9waC1sYWIvd29yay9saW51eA==/ph-boot/arch/arm64/boot/dts/*.dtb': No such file or directory +* Sending kernel package (7.2.0-rc6-phboot-g2a475abe5df2.kw.tar) to the remote +update-initramfs: Generating /boot/initrd.img-7.2.0-rc6-phboot-g2a475abe5df2 +Ignoring old or unknown version 7.2.0-rc6-phboot-g2a475abe5df2 (latest is 6.8.0-137-generic) +Generating grub configuration file ... +Found linux image: /boot/vmlinuz-6.8.0-137-generic +Found initrd image: /boot/initrd.img-6.8.0-137-generic +Found linux image: /boot/Image-7.2.0-rc6-phboot-g2a475abe5df2 +Found initrd image: /boot/initrd.img-7.2.0-rc6-phboot-g2a475abe5df2 +Found linux image: /boot/Image-7.2.0-rc6-phboot+ +Found linux image: /boot/Image-7.2.0-rc6+ +done +"; + + #[test] + fn first_error_names_the_kbuild_banner_not_the_make_cascade() { + assert_eq!( + Some( + "*** The source tree is not clean, please run 'make ARCH=arm64 mrproper'" + .to_string() + ), + first_error(UNCLEAN_TREE_BUILD_LOG) + ); + } + + #[test] + fn first_error_names_the_compiler_error() { + assert_eq!( + Some( + "/opt/ph-lab/work/linux/init/main.c:1691:2: error: #error M4 broken-build fixture" + .to_string() + ), + first_error(COMPILE_ERROR_BUILD_LOG) + ); + } + + #[test] + fn first_error_recognizes_modpost_and_git_errors() { + assert_eq!( + Some("ERROR: modpost: \"foo\" [drivers/bar.ko] undefined!".to_string()), + first_error( + " MODPOST Module.symvers\nERROR: modpost: \"foo\" [drivers/bar.ko] undefined!\n" + ) + ); + assert_eq!( + Some("error: pathspec 'nope' did not match any file(s) known to git".to_string()), + first_error("error: pathspec 'nope' did not match any file(s) known to git\n") + ); + assert_eq!( + Some("drivers/foo.c:1:10: fatal error: bar.h: No such file or directory".to_string()), + first_error("drivers/foo.c:1:10: fatal error: bar.h: No such file or directory\n") + ); + } + + #[test] + fn first_error_falls_back_to_the_last_make_error() { + let log = "\ + CC init/main.o +make[2]: *** [scripts/Makefile.build:549: certs] Error 2 +make[2]: *** Waiting for unfinished jobs.... +make: *** [Makefile:248: __sub-make] Error 2 +"; + assert_eq!( + Some("make: *** [Makefile:248: __sub-make] Error 2".to_string()), + first_error(log) + ); + } + + #[test] + fn first_error_is_none_without_any_error_line() { + assert_eq!(None, first_error("")); + assert_eq!(None, first_error(" CC init/main.o\n***\n")); + } + + #[test] + fn first_error_caps_long_lines() { + let line = format!("drivers/x.c:1:1: error: {}", "y".repeat(400)); + let found = first_error(&line).unwrap(); + assert_eq!(MAX_LINE_CHARS + 1, found.chars().count()); + assert!(found.ends_with('…')); + } + + #[test] + fn deploy_warnings_catch_initramfs_failure_and_unlisted_kernel() { + assert_eq!( + vec![ + "update-initramfs: failed for /boot/initrd.img-7.2.0-rc6+ with 1.".to_string(), + "GRUB did not list kernel 7.2.0-rc6+".to_string(), + ], + deploy_warnings(TINYCONFIG_DEPLOY_LOG, Some("7.2.0-rc6+")) + ); + } + + #[test] + fn deploy_warnings_skip_the_grub_check_without_a_release() { + assert_eq!( + vec!["update-initramfs: failed for /boot/initrd.img-7.2.0-rc6+ with 1.".to_string()], + deploy_warnings(TINYCONFIG_DEPLOY_LOG, None) + ); + } + + #[test] + fn clean_deploy_has_no_warnings() { + assert!( + deploy_warnings(CLEAN_DEPLOY_LOG, Some("7.2.0-rc6-phboot-g2a475abe5df2")).is_empty() + ); + } + + #[test] + fn grub_check_needs_an_exact_release_suffix() { + // `Image-7.2.0-rc6-phboot+` must not satisfy release `7.2.0-rc6`. + let log = "\ +Generating grub configuration file ... +Found linux image: /boot/Image-7.2.0-rc6-phboot+ +done +"; + assert_eq!( + vec!["GRUB did not list kernel 7.2.0-rc6".to_string()], + deploy_warnings(log, Some("7.2.0-rc6")) + ); + } + + #[test] + fn grub_check_is_silent_when_grub_did_not_run() { + // systemd-boot / non-GRUB DUTs never print the GRUB banner. + assert!(deploy_warnings("==> Starting build: '7.2.0'\n", Some("7.2.0")).is_empty()); + } + + #[test] + fn deploy_warnings_dedup_and_cap() { + let log = "\ +==> ERROR: module not found: 'a' +==> ERROR: module not found: 'a' +==> ERROR: module not found: 'b' +==> ERROR: module not found: 'c' +==> ERROR: module not found: 'd' +==> ERROR: module not found: 'e' +==> ERROR: module not found: 'f' +kw was unable to set up the first boot +"; + let warnings = deploy_warnings(log, None); + assert_eq!(MAX_DEPLOY_WARNINGS, warnings.len()); + assert_eq!("==> ERROR: module not found: 'a'", warnings[0]); + assert_eq!("==> ERROR: module not found: 'e'", warnings[4]); + + // The unlisted-kernel warning survives the cap. + let log = format!("{log}Generating grub configuration file ...\n"); + let warnings = deploy_warnings(&log, Some("7.2.0")); + assert_eq!(MAX_DEPLOY_WARNINGS, warnings.len()); + assert_eq!("GRUB did not list kernel 7.2.0", warnings[4]); + } +} diff --git a/src/kw/mod.rs b/src/kw/mod.rs index 97ac2bb..f6b2176 100644 --- a/src/kw/mod.rs +++ b/src/kw/mod.rs @@ -7,6 +7,7 @@ pub mod argv; pub mod errors; pub mod handle; pub mod history; +pub mod log_scan; pub mod messages; pub mod readiness; pub mod remote; diff --git a/src/kw/readiness.rs b/src/kw/readiness.rs index ee569ca..d412180 100644 --- a/src/kw/readiness.rs +++ b/src/kw/readiness.rs @@ -62,6 +62,10 @@ pub enum TreeReadiness { /// No `.config` at the build root (the tree itself, or the kw env's /// output dir when one is active). MissingKernelConfig, + /// A kw env is active but the source tree still holds an in-tree + /// `.config` or `include/config/`: kbuild refuses an `O=` build of an + /// unclean source tree, so every build fails within seconds. + InTreeBuildArtifacts, } /// Parses kw's `key=value` config format (`.kw/build.config`, @@ -131,6 +135,12 @@ pub fn probe_tree( if !fs.is_dir(&tree_path.join(".kw")) { return TreeReadiness::MissingKwDir; } + if output_dir.is_some() + && (fs.is_file(&tree_path.join(".config")) + || fs.is_dir(&tree_path.join("include").join("config"))) + { + return TreeReadiness::InTreeBuildArtifacts; + } let build_root = output_dir.unwrap_or(tree_path); if !fs.is_file(&build_root.join(".config")) { return TreeReadiness::MissingKernelConfig; @@ -405,6 +415,11 @@ impl std::fmt::Display for TreeReadiness { "no .config at the build root (the tree, or the kw env's O=)" ) } + TreeReadiness::InTreeBuildArtifacts => write!( + f, + "in-tree .config / include/config block kw env (O=) builds; run \ + `make mrproper` in the tree (the env's O= is untouched)" + ), } } } @@ -906,6 +921,43 @@ last_line_without_newline=yes"; )); } + #[test] + fn probe_tree_with_active_env_refuses_in_tree_config() { + let dir = make_ready_tree("probe-env-in-tree-config"); + let out = TempDir::new("probe-env-in-tree-config-output"); + fs::write(out.path().join(".config"), "").unwrap(); + + assert_eq!( + TreeReadiness::InTreeBuildArtifacts, + probe_tree(&OsFileSystem, dir.path(), Some(out.path())) + ); + } + + #[test] + fn probe_tree_with_active_env_refuses_in_tree_include_config() { + let dir = make_ready_tree("probe-env-in-tree-include-config"); + fs::remove_file(dir.path().join(".config")).unwrap(); + fs::create_dir(dir.path().join("include/config")).unwrap(); + let out = TempDir::new("probe-env-in-tree-include-config-output"); + fs::write(out.path().join(".config"), "").unwrap(); + + assert_eq!( + TreeReadiness::InTreeBuildArtifacts, + probe_tree(&OsFileSystem, dir.path(), Some(out.path())) + ); + } + + #[test] + fn probe_tree_without_env_accepts_in_tree_build_artifacts() { + let dir = make_ready_tree("probe-no-env-in-tree-artifacts"); + fs::create_dir(dir.path().join("include/config")).unwrap(); + + assert_eq!( + TreeReadiness::Ready { arch: None }, + probe_tree(&OsFileSystem, dir.path(), None) + ); + } + #[test] fn read_build_arch_reads_literal_value() { let dir = make_ready_tree("arch-literal"); diff --git a/src/kw/remote.rs b/src/kw/remote.rs index 0df8238..24a0c22 100644 --- a/src/kw/remote.rs +++ b/src/kw/remote.rs @@ -122,10 +122,8 @@ pub fn parse_remote_config(content: &str) -> ParsedRemoteConfig { if !rest.is_empty() { builder.port = Some(rest.to_string()); } - } else if keyword.eq_ignore_ascii_case("user") { - if !rest.is_empty() { - builder.user = Some(rest.to_string()); - } + } else if keyword.eq_ignore_ascii_case("user") && !rest.is_empty() { + builder.user = Some(rest.to_string()); } } if let Some(host) = current.and_then(HostBuilder::finish) { @@ -302,7 +300,10 @@ Host ph-dut ], parsed.hosts ); - assert_eq!("abc@arch-tm:22", choose(SAMPLE_REMOTE_CONFIG).unwrap().endpoint()); + assert_eq!( + "abc@arch-tm:22", + choose(SAMPLE_REMOTE_CONFIG).unwrap().endpoint() + ); } #[test] @@ -525,8 +526,8 @@ Host dut extra let home = home.map(str::to_string); let mut env = MockEnvTrait::new(); env.expect_var().returning(move |key| match key { - "XDG_CONFIG_HOME" => xdg.clone().ok_or_else(|| missing_var()), - "HOME" => home.clone().ok_or_else(|| missing_var()), + "XDG_CONFIG_HOME" => xdg.clone().ok_or_else(missing_var), + "HOME" => home.clone().ok_or_else(missing_var), _ => Err(missing_var()), }); env diff --git a/src/kw/status.rs b/src/kw/status.rs index ccc1860..26eab61 100644 --- a/src/kw/status.rs +++ b/src/kw/status.rs @@ -37,12 +37,18 @@ pub enum KwJobStatus { kernel_tree_id: String, branch: String, log_path: PathBuf, + /// Known deploy failures kw exited 0 through (see + /// [`crate::kw::log_scan::deploy_warnings`]). Always empty for a + /// build-only job. + warnings: Vec, }, Failed { kind: KwJobKind, phase: KwPhase, exit_code: Option, log_path: PathBuf, + /// First error line found in the log, if any. + first_error: Option, }, Cancelled { kind: KwJobKind, @@ -162,6 +168,7 @@ mod tests { kernel_tree_id: "mainline".to_string(), branch: "patchset-x".to_string(), log_path: PathBuf::from("/tmp/build.log"), + warnings: Vec::new(), }, restore_branch: Some("master".to_string()), } diff --git a/src/ui/core.rs b/src/ui/core.rs index bedb5bb..fcc83ac 100644 --- a/src/ui/core.rs +++ b/src/ui/core.rs @@ -38,7 +38,9 @@ impl UiCore { ScreenViewModel::EditConfig(ec_vm) => { UiBody::EditConfig(screens::edit_config::build_scene(ec_vm)) } - ScreenViewModel::KwOps(kw_vm) => UiBody::KwOps(screens::kw_ops::build_scene(kw_vm)), + ScreenViewModel::KwOps(kw_vm) => { + UiBody::KwOps(Box::new(screens::kw_ops::build_scene(kw_vm))) + } }; let navigation = self.build_navigation(&vm.screen, vm.kw_running.as_deref()); diff --git a/src/ui/scene.rs b/src/ui/scene.rs index 63eb479..f420261 100644 --- a/src/ui/scene.rs +++ b/src/ui/scene.rs @@ -94,6 +94,9 @@ pub struct KwOpsScene { pub deploy_label: String, pub build_deploy_label: String, pub branch_guidance: Option, + pub warnings: Option, + pub first_error: Option, + pub log_path: Option, pub log_tail: String, } @@ -105,7 +108,9 @@ pub enum UiBody { Latest(LatestScene), PatchsetDetails(PatchsetDetailsScene), EditConfig(EditConfigScene), - KwOps(KwOpsScene), + // Boxed to keep the enum small (clippy::large_enum_variant): KwOps + // carries one owned string per dashboard row. + KwOps(Box), } /// Pre-computed navigation-bar content. diff --git a/src/ui/screens/edit_config.rs b/src/ui/screens/edit_config.rs index fff8895..db72efd 100644 --- a/src/ui/screens/edit_config.rs +++ b/src/ui/screens/edit_config.rs @@ -72,15 +72,54 @@ pub fn mode_spans(vm: &EditConfigViewModel) -> Vec> { } pub fn keys_hint_span(vm: &EditConfigViewModel) -> Span<'static> { - if vm.is_editing_mode { + if vm.editing_tree_selector { + Span::styled( + "(←/β†’) cycle | (ENTER) confirm | (ESC) cancel", + Style::default().fg(Color::Red), + ) + } else if vm.is_editing_mode { Span::styled( "(ESC) cancel | (ENTER) confirm", Style::default().fg(Color::Red), ) } else { Span::styled( - "(ESC / q) exit | (ENTER) edit | (jk| πŸ‘‡ πŸ‘… ) down up", + "(ESC / q) save and exit | (ENTER) edit | (jk| πŸ‘‡ πŸ‘… ) down up", Style::default().fg(Color::Red), ) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::app::view_model::EditConfigViewModel; + + fn vm(is_editing_mode: bool, editing_tree_selector: bool) -> EditConfigViewModel { + EditConfigViewModel { + entries: vec![], + is_editing_mode, + editing_tree_selector, + } + } + + #[test] + fn keys_hint_shows_cycle_bindings_on_the_tree_row() { + let hint = keys_hint_span(&vm(true, true)); + assert!(hint.content.contains("(←/β†’) cycle")); + assert!(hint.content.contains("(ENTER) confirm")); + } + + #[test] + fn keys_hint_keeps_text_edit_bindings_on_other_rows() { + let hint = keys_hint_span(&vm(true, false)); + assert!(hint.content.contains("(ESC) cancel")); + assert!(!hint.content.contains("cycle")); + } + + #[test] + fn keys_hint_says_save_and_exit_when_browsing() { + let hint = keys_hint_span(&vm(false, false)); + assert!(hint.content.contains("(ESC / q) save and exit")); + } +} diff --git a/src/ui/screens/kw_ops.rs b/src/ui/screens/kw_ops.rs index 0331a2d..69d3fe7 100644 --- a/src/ui/screens/kw_ops.rs +++ b/src/ui/screens/kw_ops.rs @@ -33,6 +33,9 @@ pub fn build_scene(vm: &KwOpsViewModel) -> KwOpsScene { deploy_label: vm.deploy_label.clone(), build_deploy_label: vm.build_deploy_label.clone(), branch_guidance: vm.branch_guidance.clone(), + warnings: vm.warnings.clone(), + first_error: vm.first_error.clone(), + log_path: vm.log_path.clone(), log_tail: vm.log_tail.clone(), } } @@ -76,6 +79,17 @@ fn paint_form(f: &mut Frame, scene: &KwOpsScene, chunk: Rect) { labeled("Remote", &scene.remote), labeled("Boot once", &scene.boot_once), labeled("Job", &scene.job_status), + ]); + if let Some(warnings) = &scene.warnings { + lines.push(labeled_colored("Warnings", warnings, Color::Yellow)); + } + if let Some(first_error) = &scene.first_error { + lines.push(labeled_colored("First error", first_error, Color::Red)); + } + if let Some(log_path) = &scene.log_path { + lines.push(labeled("Log", log_path)); + } + lines.extend([ labeled("Command", &scene.command), labeled("Deploy command", &scene.deploy_command), labeled("Start", &scene.start_label), @@ -123,9 +137,13 @@ pub(crate) fn log_scroll_offset(text: &str, inner_width: u16, inner_height: u16) } fn labeled(label: &str, value: &str) -> Line<'static> { + labeled_colored(label, value, Color::White) +} + +fn labeled_colored(label: &str, value: &str, color: Color) -> Line<'static> { Line::from(vec![ Span::styled(format!("{label}: "), Style::default().fg(Color::Cyan)), - Span::styled(value.to_string(), Style::default().fg(Color::White)), + Span::styled(value.to_string(), Style::default().fg(color)), ]) }