* [pve-devel] [PATCH installer 0/7] refactor; improve installation progress
@ 2023-07-26 14:03 Christoph Heiss
2023-07-26 14:03 ` [pve-devel] [PATCH installer 1/7] tui: move install progress dialog into own view module Christoph Heiss
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw)
To: pve-devel
First 5 patches splits out the gigantic install progress dialog into its
own module; then applying some divide-and-conquer to make it more
readable and get the inner >150 lines closure down to a more
maintainable state.
Patch 6 does some code de-duplication for the low-lever installer.
Patch 7 improves the auto-reboot message/UX by counting down the
timeout, to match its GUI counterpart.
Christoph Heiss (7):
tui: move install progress dialog into own view module
tui: install progress: split out low-level installer spawing into own
function
tui: install progress: split out reboot handling into own function
tui: install progress: split out prompt logic into own function
tui: install progress: handle errors in ui message loop more
gracefully
low-level: avoid open-coding config reading, parsing and merging
low-level, tui: count down auto-reboot timeout
proxmox-low-level-installer | 70 +++--
proxmox-tui-installer/src/main.rs | 235 +--------------
.../src/views/install_progress.rs | 275 ++++++++++++++++++
proxmox-tui-installer/src/views/mod.rs | 3 +
4 files changed, 318 insertions(+), 265 deletions(-)
create mode 100644 proxmox-tui-installer/src/views/install_progress.rs
--
2.41.0
^ permalink raw reply [flat|nested] 8+ messages in thread* [pve-devel] [PATCH installer 1/7] tui: move install progress dialog into own view module 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss @ 2023-07-26 14:03 ` Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 2/7] tui: install progress: split out low-level installer spawing into own function Christoph Heiss ` (5 subsequent siblings) 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw) To: pve-devel While doing so, also pull the main progress thread task into a separate function, as well as the spawning of the low-level installer. No functional changes. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- proxmox-tui-installer/src/main.rs | 235 +--------------- .../src/views/install_progress.rs | 251 ++++++++++++++++++ proxmox-tui-installer/src/views/mod.rs | 3 + 3 files changed, 261 insertions(+), 228 deletions(-) create mode 100644 proxmox-tui-installer/src/views/install_progress.rs diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs index 7bfaf9b..a356f6d 100644 --- a/proxmox-tui-installer/src/main.rs +++ b/proxmox-tui-installer/src/main.rs @@ -1,24 +1,12 @@ -use std::{ - collections::HashMap, - env, - io::{BufRead, BufReader, Write}, - net::IpAddr, - path::PathBuf, - str::FromStr, - sync::{Arc, Mutex}, - thread, - time::Duration, -}; +use std::{collections::HashMap, env, net::IpAddr, path::PathBuf}; use cursive::{ event::Event, theme::{ColorStyle, Effect, PaletteColor, Style}, - utils::Counter, view::{Nameable, Offset, Resizable, ViewWrapper}, views::{ Button, Checkbox, Dialog, DummyView, EditView, Layer, LinearLayout, PaddedView, Panel, - ProgressBar, ResizedView, ScrollView, SelectView, StackView, TextContent, TextView, - ViewRef, + ResizedView, ScrollView, SelectView, StackView, TextView, ViewRef, }, Cursive, CursiveRunnable, ScreenId, View, XY, }; @@ -29,7 +17,7 @@ mod options; use options::*; mod setup; -use setup::{InstallConfig, LocaleInfo, ProxmoxProduct, RuntimeInfo, SetupInfo}; +use setup::{LocaleInfo, ProxmoxProduct, RuntimeInfo, SetupInfo}; mod system; @@ -38,8 +26,8 @@ use utils::Fqdn; mod views; use views::{ - BootdiskOptionsView, CidrAddressEditView, FormView, TableView, TableViewItem, - TimezoneOptionsView, + BootdiskOptionsView, CidrAddressEditView, FormView, InstallProgressView, TableView, + TableViewItem, TimezoneOptionsView, }; // TextView::center() seems to garble the first two lines, so fix it manually here. @@ -720,215 +708,6 @@ fn install_progress_dialog(siv: &mut Cursive) -> InstallerView { // Ensure the screen is updated independently of keyboard events and such siv.set_autorefresh(true); - let cb_sink = siv.cb_sink().clone(); - let state = siv.user_data::<InstallerState>().unwrap(); - let progress_text = TextContent::new("starting the installation .."); - - let progress_task = { - let progress_text = progress_text.clone(); - let options = state.options.clone(); - move |counter: Counter| { - let child = { - use std::process::{Command, Stdio}; - - #[cfg(not(debug_assertions))] - let (path, args, envs): (&str, [&str; 1], [(&str, &str); 0]) = - ("proxmox-low-level-installer", ["start-session"], []); - - #[cfg(debug_assertions)] - let (path, args, envs) = ( - PathBuf::from("./proxmox-low-level-installer"), - ["-t", "start-session-test"], - [("PERL5LIB", ".")], - ); - - Command::new(path) - .args(args) - .envs(envs) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .spawn() - }; - - let mut child = match child { - Ok(child) => child, - Err(err) => { - let _ = cb_sink.send(Box::new(move |siv| { - siv.add_layer( - Dialog::text(err.to_string()) - .title("Error") - .button("Ok", Cursive::quit), - ); - })); - return; - } - }; - - let inner = || { - let reader = child.stdout.take().map(BufReader::new)?; - let mut writer = child.stdin.take()?; - - serde_json::to_writer(&mut writer, &InstallConfig::from(options)).unwrap(); - writeln!(writer).unwrap(); - - let writer = Arc::new(Mutex::new(writer)); - - for line in reader.lines() { - let line = match line { - Ok(line) => line, - Err(_) => break, - }; - - let msg = match line.parse::<UiMessage>() { - Ok(msg) => msg, - Err(stray) => { - eprintln!("low-level installer: {stray}"); - continue; - } - }; - - match msg { - UiMessage::Info(s) => cb_sink.send(Box::new(|siv| { - siv.add_layer(Dialog::info(s).title("Information")); - })), - UiMessage::Error(s) => cb_sink.send(Box::new(|siv| { - siv.add_layer(Dialog::info(s).title("Error")); - })), - UiMessage::Prompt(s) => cb_sink.send({ - let writer = writer.clone(); - Box::new(move |siv| { - yes_no_dialog( - siv, - "Prompt", - &s, - Box::new({ - let writer = writer.clone(); - move |_| { - if let Ok(mut writer) = writer.lock() { - let _ = writeln!(writer, "ok"); - } - } - }), - Box::new(move |_| { - if let Ok(mut writer) = writer.lock() { - let _ = writeln!(writer); - } - }), - ); - }) - }), - UiMessage::Progress(ratio, s) => { - counter.set(ratio); - progress_text.set_content(s); - Ok(()) - } - UiMessage::Finished(success, msg) => { - counter.set(100); - progress_text.set_content(msg.to_owned()); - cb_sink.send(Box::new(move |siv| { - let title = if success { "Success" } else { "Failure" }; - - // For rebooting, we just need to quit the installer, - // our caller does the actual reboot. - siv.add_layer( - Dialog::text(msg) - .title(title) - .button("Reboot now", Cursive::quit), - ); - - let autoreboot = siv - .user_data::<InstallerState>() - .map(|state| state.options.autoreboot) - .unwrap_or_default(); - - if autoreboot && success { - let cb_sink = siv.cb_sink(); - thread::spawn({ - let cb_sink = cb_sink.clone(); - move || { - thread::sleep(Duration::from_secs(5)); - let _ = cb_sink.send(Box::new(Cursive::quit)); - } - }); - } - })) - } - } - .unwrap(); - } - - Some(()) - }; - - if inner().is_none() { - cb_sink - .send(Box::new(|siv| { - siv.add_layer( - Dialog::text("low-level installer exited early") - .title("Error") - .button("Exit", Cursive::quit), - ); - })) - .unwrap(); - } - } - }; - - let progress_bar = ProgressBar::new().with_task(progress_task).full_width(); - let inner = PaddedView::lrtb( - 1, - 1, - 1, - 1, - LinearLayout::vertical() - .child(PaddedView::lrtb(1, 1, 0, 0, progress_bar)) - .child(DummyView) - .child(TextView::new_with_content(progress_text).center()) - .child(PaddedView::lrtb( - 1, - 1, - 1, - 0, - LinearLayout::horizontal().child(abort_install_button()), - )), - ); - - InstallerView::with_raw(state, inner) -} - -enum UiMessage { - Info(String), - Error(String), - Prompt(String), - Finished(bool, String), - Progress(usize, String), -} - -impl FromStr for UiMessage { - type Err = String; - - fn from_str(s: &str) -> Result<Self, Self::Err> { - let (ty, rest) = s.split_once(": ").ok_or("invalid message: no type")?; - - match ty { - "message" => Ok(UiMessage::Info(rest.to_owned())), - "error" => Ok(UiMessage::Error(rest.to_owned())), - "prompt" => Ok(UiMessage::Prompt(rest.to_owned())), - "finished" => { - let (state, rest) = rest.split_once(", ").ok_or("invalid message: no state")?; - Ok(UiMessage::Finished(state == "ok", rest.to_owned())) - } - "progress" => { - let (percent, rest) = rest.split_once(' ').ok_or("invalid progress message")?; - Ok(UiMessage::Progress( - percent - .parse::<f64>() - .map(|v| (v * 100.).floor() as usize) - .map_err(|err| err.to_string())?, - rest.to_owned(), - )) - } - unknown => Err(format!("invalid message type {unknown}, rest: {rest}")), - } - } + let state = siv.user_data::<InstallerState>().cloned().unwrap(); + InstallerView::with_raw(&state, InstallProgressView::new(siv)) } diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs new file mode 100644 index 0000000..e9d764a --- /dev/null +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -0,0 +1,251 @@ +use std::{ + io::{self, BufRead, BufReader, Write}, + process, + str::FromStr, + sync::{Arc, Mutex}, + thread, + time::Duration, +}; + +use cursive::{ + utils::Counter, + view::{Resizable, ViewWrapper}, + views::{Dialog, DummyView, LinearLayout, PaddedView, ProgressBar, TextContent, TextView}, + CbSink, Cursive, +}; + +use crate::{ + abort_install_button, options::InstallerOptions, setup::InstallConfig, yes_no_dialog, + InstallerState, +}; + +pub struct InstallProgressView { + view: PaddedView<LinearLayout>, +} + +impl InstallProgressView { + pub fn new(siv: &mut Cursive) -> Self { + let cb_sink = siv.cb_sink().clone(); + let state = siv.user_data::<InstallerState>().unwrap(); + let progress_text = TextContent::new("starting the installation .."); + + let progress_task = { + let progress_text = progress_text.clone(); + let options = state.options.clone(); + move |counter: Counter| progress_task(counter, cb_sink, options, progress_text) + }; + + let progress_bar = ProgressBar::new().with_task(progress_task).full_width(); + let view = PaddedView::lrtb( + 1, + 1, + 1, + 1, + LinearLayout::vertical() + .child(PaddedView::lrtb(1, 1, 0, 0, progress_bar)) + .child(DummyView) + .child(TextView::new_with_content(progress_text).center()) + .child(PaddedView::lrtb( + 1, + 1, + 1, + 0, + LinearLayout::horizontal().child(abort_install_button()), + )), + ); + + Self { view } + } +} + +fn progress_task( + counter: Counter, + cb_sink: CbSink, + options: InstallerOptions, + progress_text: TextContent, +) { + let child = { + use std::process::{Command, Stdio}; + + #[cfg(not(debug_assertions))] + let (path, args, envs): (&str, [&str; 1], [(&str, &str); 0]) = + ("proxmox-low-level-installer", ["start-session"], []); + + #[cfg(debug_assertions)] + let (path, args, envs) = ( + std::path::PathBuf::from("./proxmox-low-level-installer"), + ["-t", "start-session-test"], + [("PERL5LIB", ".")], + ); + + Command::new(path) + .args(args) + .envs(envs) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + }; + + let mut child = match child { + Ok(child) => child, + Err(err) => { + let _ = cb_sink.send(Box::new(move |siv| { + siv.add_layer( + Dialog::text(err.to_string()) + .title("Error") + .button("Ok", Cursive::quit), + ); + })); + return; + } + }; + + let inner = || { + let reader = child.stdout.take().map(BufReader::new)?; + let mut writer = child.stdin.take()?; + + serde_json::to_writer(&mut writer, &InstallConfig::from(options)).unwrap(); + writeln!(writer).unwrap(); + + let writer = Arc::new(Mutex::new(writer)); + + for line in reader.lines() { + let line = match line { + Ok(line) => line, + Err(_) => break, + }; + + let msg = match line.parse::<UiMessage>() { + Ok(msg) => msg, + Err(stray) => { + eprintln!("low-level installer: {stray}"); + continue; + } + }; + + match msg { + UiMessage::Info(s) => cb_sink.send(Box::new(|siv| { + siv.add_layer(Dialog::info(s).title("Information")); + })), + UiMessage::Error(s) => cb_sink.send(Box::new(|siv| { + siv.add_layer(Dialog::info(s).title("Error")); + })), + UiMessage::Prompt(s) => cb_sink.send({ + let writer = writer.clone(); + Box::new(move |siv| { + yes_no_dialog( + siv, + "Prompt", + &s, + Box::new({ + let writer = writer.clone(); + move |_| { + if let Ok(mut writer) = writer.lock() { + let _ = writeln!(writer, "ok"); + } + } + }), + Box::new(move |_| { + if let Ok(mut writer) = writer.lock() { + let _ = writeln!(writer); + } + }), + ); + }) + }), + UiMessage::Progress(ratio, s) => { + counter.set(ratio); + progress_text.set_content(s); + Ok(()) + } + UiMessage::Finished(success, msg) => { + counter.set(100); + progress_text.set_content(msg.to_owned()); + cb_sink.send(Box::new(move |siv| { + let title = if success { "Success" } else { "Failure" }; + + // For rebooting, we just need to quit the installer, + // our caller does the actual reboot. + siv.add_layer( + Dialog::text(msg) + .title(title) + .button("Reboot now", Cursive::quit), + ); + + let autoreboot = siv + .user_data::<InstallerState>() + .map(|state| state.options.autoreboot) + .unwrap_or_default(); + + if autoreboot && success { + let cb_sink = siv.cb_sink(); + thread::spawn({ + let cb_sink = cb_sink.clone(); + move || { + thread::sleep(Duration::from_secs(5)); + let _ = cb_sink.send(Box::new(Cursive::quit)); + } + }); + } + })) + } + } + .unwrap(); + } + + Some(()) + }; + + if inner().is_none() { + cb_sink + .send(Box::new(|siv| { + siv.add_layer( + Dialog::text("low-level installer exited early") + .title("Error") + .button("Exit", Cursive::quit), + ); + })) + .unwrap(); + } +} + +impl ViewWrapper for InstallProgressView { + cursive::wrap_impl!(self.view: PaddedView<LinearLayout>); +} + +enum UiMessage { + Info(String), + Error(String), + Prompt(String), + Finished(bool, String), + Progress(usize, String), +} + +impl FromStr for UiMessage { + type Err = String; + + fn from_str(s: &str) -> Result<Self, Self::Err> { + let (ty, rest) = s.split_once(": ").ok_or("invalid message: no type")?; + + match ty { + "message" => Ok(UiMessage::Info(rest.to_owned())), + "error" => Ok(UiMessage::Error(rest.to_owned())), + "prompt" => Ok(UiMessage::Prompt(rest.to_owned())), + "finished" => { + let (state, rest) = rest.split_once(", ").ok_or("invalid message: no state")?; + Ok(UiMessage::Finished(state == "ok", rest.to_owned())) + } + "progress" => { + let (percent, rest) = rest.split_once(' ').ok_or("invalid progress message")?; + Ok(UiMessage::Progress( + percent + .parse::<f64>() + .map(|v| (v * 100.).floor() as usize) + .map_err(|err| err.to_string())?, + rest.to_owned(), + )) + } + unknown => Err(format!("invalid message type {unknown}, rest: {rest}")), + } + } +} diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs index aa24fa4..18dbdcf 100644 --- a/proxmox-tui-installer/src/views/mod.rs +++ b/proxmox-tui-installer/src/views/mod.rs @@ -12,6 +12,9 @@ use crate::utils::CidrAddress; mod bootdisk; pub use bootdisk::*; +mod install_progress; +pub use install_progress::*; + mod table_view; pub use table_view::*; -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 2/7] tui: install progress: split out low-level installer spawing into own function 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 1/7] tui: move install progress dialog into own view module Christoph Heiss @ 2023-07-26 14:03 ` Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 3/7] tui: install progress: split out reboot handling " Christoph Heiss ` (4 subsequent siblings) 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw) To: pve-devel No functional changes. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- .../src/views/install_progress.rs | 46 +++++++++---------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs index e9d764a..870c6a3 100644 --- a/proxmox-tui-installer/src/views/install_progress.rs +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -58,35 +58,35 @@ impl InstallProgressView { } } +fn spawn_low_level_installer() -> io::Result<process::Child> { + use std::process::{Command, Stdio}; + + #[cfg(not(debug_assertions))] + let (path, args, envs): (&str, [&str; 1], [(&str, &str); 0]) = + ("proxmox-low-level-installer", ["start-session"], []); + + #[cfg(debug_assertions)] + let (path, args, envs) = ( + std::path::PathBuf::from("./proxmox-low-level-installer"), + ["-t", "start-session-test"], + [("PERL5LIB", ".")], + ); + + Command::new(path) + .args(args) + .envs(envs) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() +} + fn progress_task( counter: Counter, cb_sink: CbSink, options: InstallerOptions, progress_text: TextContent, ) { - let child = { - use std::process::{Command, Stdio}; - - #[cfg(not(debug_assertions))] - let (path, args, envs): (&str, [&str; 1], [(&str, &str); 0]) = - ("proxmox-low-level-installer", ["start-session"], []); - - #[cfg(debug_assertions)] - let (path, args, envs) = ( - std::path::PathBuf::from("./proxmox-low-level-installer"), - ["-t", "start-session-test"], - [("PERL5LIB", ".")], - ); - - Command::new(path) - .args(args) - .envs(envs) - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .spawn() - }; - - let mut child = match child { + let mut child = match spawn_low_level_installer() { Ok(child) => child, Err(err) => { let _ = cb_sink.send(Box::new(move |siv| { -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 3/7] tui: install progress: split out reboot handling into own function 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 1/7] tui: move install progress dialog into own view module Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 2/7] tui: install progress: split out low-level installer spawing into own function Christoph Heiss @ 2023-07-26 14:03 ` Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 4/7] tui: install progress: split out prompt logic " Christoph Heiss ` (3 subsequent siblings) 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw) To: pve-devel No functional changes. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- .../src/views/install_progress.rs | 55 ++++++++++--------- 1 file changed, 28 insertions(+), 27 deletions(-) diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs index 870c6a3..bd27628 100644 --- a/proxmox-tui-installer/src/views/install_progress.rs +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -80,6 +80,33 @@ fn spawn_low_level_installer() -> io::Result<process::Child> { .spawn() } +fn prepare_for_reboot(siv: &mut Cursive, success: bool, msg: &str) { + let title = if success { "Success" } else { "Failure" }; + + // For rebooting, we just need to quit the installer, our caller does the actual reboot. + siv.add_layer( + Dialog::text(msg) + .title(title) + .button("Reboot now", Cursive::quit), + ); + + let autoreboot = siv + .user_data::<InstallerState>() + .map(|state| state.options.autoreboot) + .unwrap_or_default(); + + if autoreboot && success { + let cb_sink = siv.cb_sink(); + thread::spawn({ + let cb_sink = cb_sink.clone(); + move || { + thread::sleep(Duration::from_secs(5)); + let _ = cb_sink.send(Box::new(Cursive::quit)); + } + }); + } +} + fn progress_task( counter: Counter, cb_sink: CbSink, @@ -161,33 +188,7 @@ fn progress_task( UiMessage::Finished(success, msg) => { counter.set(100); progress_text.set_content(msg.to_owned()); - cb_sink.send(Box::new(move |siv| { - let title = if success { "Success" } else { "Failure" }; - - // For rebooting, we just need to quit the installer, - // our caller does the actual reboot. - siv.add_layer( - Dialog::text(msg) - .title(title) - .button("Reboot now", Cursive::quit), - ); - - let autoreboot = siv - .user_data::<InstallerState>() - .map(|state| state.options.autoreboot) - .unwrap_or_default(); - - if autoreboot && success { - let cb_sink = siv.cb_sink(); - thread::spawn({ - let cb_sink = cb_sink.clone(); - move || { - thread::sleep(Duration::from_secs(5)); - let _ = cb_sink.send(Box::new(Cursive::quit)); - } - }); - } - })) + cb_sink.send(Box::new(move |siv| prepare_for_reboot(siv, success, &msg))) } } .unwrap(); -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 4/7] tui: install progress: split out prompt logic into own function 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss ` (2 preceding siblings ...) 2023-07-26 14:03 ` [pve-devel] [PATCH installer 3/7] tui: install progress: split out reboot handling " Christoph Heiss @ 2023-07-26 14:03 ` Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 5/7] tui: install progress: handle errors in ui message loop more gracefully Christoph Heiss ` (2 subsequent siblings) 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw) To: pve-devel No functional changes. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- .../src/views/install_progress.rs | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs index bd27628..3046a26 100644 --- a/proxmox-tui-installer/src/views/install_progress.rs +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -80,6 +80,27 @@ fn spawn_low_level_installer() -> io::Result<process::Child> { .spawn() } +fn show_prompt<W: Write + 'static>(siv: &mut Cursive, text: &str, writer: Arc<Mutex<W>>) { + yes_no_dialog( + siv, + "Prompt", + text, + Box::new({ + let writer = writer.clone(); + move |_| { + if let Ok(mut writer) = writer.lock() { + let _ = writeln!(writer, "ok"); + } + } + }), + Box::new(move |_| { + if let Ok(mut writer) = writer.lock() { + let _ = writeln!(writer); + } + }), + ); +} + fn prepare_for_reboot(siv: &mut Cursive, success: bool, msg: &str) { let title = if success { "Success" } else { "Failure" }; @@ -159,26 +180,7 @@ fn progress_task( })), UiMessage::Prompt(s) => cb_sink.send({ let writer = writer.clone(); - Box::new(move |siv| { - yes_no_dialog( - siv, - "Prompt", - &s, - Box::new({ - let writer = writer.clone(); - move |_| { - if let Ok(mut writer) = writer.lock() { - let _ = writeln!(writer, "ok"); - } - } - }), - Box::new(move |_| { - if let Ok(mut writer) = writer.lock() { - let _ = writeln!(writer); - } - }), - ); - }) + Box::new(move |siv| show_prompt(siv, &s, writer)) }), UiMessage::Progress(ratio, s) => { counter.set(ratio); -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 5/7] tui: install progress: handle errors in ui message loop more gracefully 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss ` (3 preceding siblings ...) 2023-07-26 14:03 ` [pve-devel] [PATCH installer 4/7] tui: install progress: split out prompt logic " Christoph Heiss @ 2023-07-26 14:03 ` Christoph Heiss 2023-07-26 14:04 ` [pve-devel] [PATCH installer 6/7] low-level: avoid open-coding config reading, parsing and merging Christoph Heiss 2023-07-26 14:04 ` [pve-devel] [PATCH installer 7/7] low-level, tui: count down auto-reboot timeout Christoph Heiss 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:03 UTC (permalink / raw) To: pve-devel This at least gives /some/ feedback to the user he can potentially report, instead of a single, hardcoded message. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- .../src/views/install_progress.rs | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs index 3046a26..2025a4c 100644 --- a/proxmox-tui-installer/src/views/install_progress.rs +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -149,29 +149,36 @@ fn progress_task( }; let inner = || { - let reader = child.stdout.take().map(BufReader::new)?; - let mut writer = child.stdin.take()?; + let reader = child + .stdout + .take() + .map(BufReader::new) + .ok_or("failed to get stdin reader")?; - serde_json::to_writer(&mut writer, &InstallConfig::from(options)).unwrap(); - writeln!(writer).unwrap(); + let mut writer = child.stdin.take().ok_or("failed to get stdin writer")?; + + serde_json::to_writer(&mut writer, &InstallConfig::from(options)) + .map_err(|err| format!("failed to serialize install config: {err}"))?; + writeln!(writer).map_err(|err| format!("failed to write install config: {err}"))?; let writer = Arc::new(Mutex::new(writer)); for line in reader.lines() { let line = match line { Ok(line) => line, - Err(_) => break, + Err(err) => return Err(format!("low-level installer exited early: {err}")), }; let msg = match line.parse::<UiMessage>() { Ok(msg) => msg, Err(stray) => { + // Not a fatal error, so don't abort the installation by returning eprintln!("low-level installer: {stray}"); continue; } }; - match msg { + let result = match msg.clone() { UiMessage::Info(s) => cb_sink.send(Box::new(|siv| { siv.add_layer(Dialog::info(s).title("Information")); })), @@ -192,18 +199,23 @@ fn progress_task( progress_text.set_content(msg.to_owned()); cb_sink.send(Box::new(move |siv| prepare_for_reboot(siv, success, &msg))) } + }; + + if let Err(err) = result { + eprintln!("error during message handling: {err}"); + eprintln!(" message was: '{msg:?}'"); } - .unwrap(); } - Some(()) + Ok(()) }; - if inner().is_none() { + if let Err(err) = inner() { + let message = format!("install failed: {err}"); cb_sink .send(Box::new(|siv| { siv.add_layer( - Dialog::text("low-level installer exited early") + Dialog::text(message) .title("Error") .button("Exit", Cursive::quit), ); @@ -216,6 +228,7 @@ impl ViewWrapper for InstallProgressView { cursive::wrap_impl!(self.view: PaddedView<LinearLayout>); } +#[derive(Debug, Clone)] enum UiMessage { Info(String), Error(String), -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 6/7] low-level: avoid open-coding config reading, parsing and merging 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss ` (4 preceding siblings ...) 2023-07-26 14:03 ` [pve-devel] [PATCH installer 5/7] tui: install progress: handle errors in ui message loop more gracefully Christoph Heiss @ 2023-07-26 14:04 ` Christoph Heiss 2023-07-26 14:04 ` [pve-devel] [PATCH installer 7/7] low-level, tui: count down auto-reboot timeout Christoph Heiss 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:04 UTC (permalink / raw) To: pve-devel Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- proxmox-low-level-installer | 45 +++++++++++++++---------------------- 1 file changed, 18 insertions(+), 27 deletions(-) diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer index 814961e..745402a 100755 --- a/proxmox-low-level-installer +++ b/proxmox-low-level-installer @@ -53,6 +53,22 @@ sub usage { exit($cmd ne 'help' ? 1 : 0); } +sub read_and_merge_config { + my $config_raw; + while (my $line = <>) { + if ($line =~ /^\s*\{/) { + $config_raw = $line; + last; + } + } + + my $config = eval { from_json($config_raw, { utf8 => 1 }) }; + die "failed to parse config from stdin - $@\n" if $@; + + Proxmox::Install::Config::merge($config); + log_info("got installation config: ". to_json(Proxmox::Install::Config::get(), { utf8 => 1, canonical => 1 }) ."\n"); +} + my $cmd = shift; if (!$cmd || $cmd eq 'help' || !exists($commands->{$cmd})) { usage($cmd // ''); @@ -87,19 +103,7 @@ if ($cmd eq 'dump-env') { file_write_all($run_env_file, $run_env_serialized); } elsif ($cmd eq 'start-session') { Proxmox::UI::init_stdio({}, $env); - - my $config_raw; - while (my $line = <>) { - if ($line =~ /^\s*\{/) { - $config_raw = $line; - last; - } - } - my $config = eval { from_json($config_raw, { utf8 => 1 }) }; - die "failed to parse config from stdin - $@\n" if $@; - - Proxmox::Install::Config::merge($config); - log_info("got installation config: ". to_json(Proxmox::Install::Config::get(), { utf8 => 1, canonical => 1 }) ."\n"); + read_and_merge_config(); eval { Proxmox::Install::extract_data() }; if (my $err = $@) { @@ -119,20 +123,7 @@ if ($cmd eq 'dump-env') { } } elsif ($cmd eq 'start-session-test') { Proxmox::UI::init_stdio({}, $env); - - my $config_raw; - while (my $line = <>) { - if ($line =~ /^\s*\{/) { - $config_raw = $line; - last; - } - } - my $config = eval { from_json($config_raw, { utf8 => 1 }) }; - die "failed to parse config from stdin - $@\n" if $@; - - Proxmox::Install::Config::merge($config); - - print STDERR "got config: ". to_json(Proxmox::Install::Config::get(), { utf8 => 1, canonical => 1 }) ."\n"; + read_and_merge_config(); my $res = Proxmox::UI::prompt("Reply anything?") ? 'ok' : 'not ok'; Proxmox::UI::message("Test Message - got $res"); -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [pve-devel] [PATCH installer 7/7] low-level, tui: count down auto-reboot timeout 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss ` (5 preceding siblings ...) 2023-07-26 14:04 ` [pve-devel] [PATCH installer 6/7] low-level: avoid open-coding config reading, parsing and merging Christoph Heiss @ 2023-07-26 14:04 ` Christoph Heiss 6 siblings, 0 replies; 8+ messages in thread From: Christoph Heiss @ 2023-07-26 14:04 UTC (permalink / raw) To: pve-devel The GUI installer already has the same functionality, with this the TUI installer gains the same. It is a nice touch anyway, primarily to indicate to the user that the installer is not frozen or similar. Signed-off-by: Christoph Heiss <c.heiss@proxmox.com> --- proxmox-low-level-installer | 25 +++++++++++-------- .../src/views/install_progress.rs | 12 +++++++-- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/proxmox-low-level-installer b/proxmox-low-level-installer index 745402a..0141170 100755 --- a/proxmox-low-level-installer +++ b/proxmox-low-level-installer @@ -69,6 +69,19 @@ sub read_and_merge_config { log_info("got installation config: ". to_json(Proxmox::Install::Config::get(), { utf8 => 1, canonical => 1 }) ."\n"); } +sub send_reboot_ui_message { + if (Proxmox::Install::Config::get_autoreboot()) { + my $secs = 5; + while ($secs > 0) { + Proxmox::UI::finished(1, "Installation finished - auto-rebooting in $secs seconds .."); + sleep 1; + $secs -= 1; + } + } else { + Proxmox::UI::finished(1, "Installation complete - reboot now?"); + } +} + my $cmd = shift; if (!$cmd || $cmd eq 'help' || !exists($commands->{$cmd})) { usage($cmd // ''); @@ -115,11 +128,7 @@ if ($cmd eq 'dump-env') { Proxmox::UI::finished(0, $err); } } else { - if (Proxmox::Install::Config::get_autoreboot()) { - Proxmox::UI::finished(1, "Installation finished - auto-rebooting in ~ 5 seconds"); - } else { - Proxmox::UI::finished(1, "Installation complete - reboot now?"); - } + send_reboot_ui_message(); } } elsif ($cmd eq 'start-session-test') { Proxmox::UI::init_stdio({}, $env); @@ -137,11 +146,7 @@ if ($cmd eq 'dump-env') { } } - if (Proxmox::Install::Config::get_autoreboot()) { - Proxmox::UI::finished(1, "Installation finished - auto-rebooting in ~ 5 seconds"); - } else { - Proxmox::UI::finished(1, "Installation complete - reboot now?"); - } + send_reboot_ui_message(); } exit(0); diff --git a/proxmox-tui-installer/src/views/install_progress.rs b/proxmox-tui-installer/src/views/install_progress.rs index 2025a4c..c8d9aab 100644 --- a/proxmox-tui-installer/src/views/install_progress.rs +++ b/proxmox-tui-installer/src/views/install_progress.rs @@ -9,7 +9,7 @@ use std::{ use cursive::{ utils::Counter, - view::{Resizable, ViewWrapper}, + view::{Nameable, Resizable, ViewWrapper}, views::{Dialog, DummyView, LinearLayout, PaddedView, ProgressBar, TextContent, TextView}, CbSink, Cursive, }; @@ -102,13 +102,21 @@ fn show_prompt<W: Write + 'static>(siv: &mut Cursive, text: &str, writer: Arc<Mu } fn prepare_for_reboot(siv: &mut Cursive, success: bool, msg: &str) { + const DIALOG_ID: &str = "autoreboot-dialog"; let title = if success { "Success" } else { "Failure" }; + // If the dialog was previously created, just update its content and we're done. + if let Some(mut dialog) = siv.find_name::<Dialog>(DIALOG_ID) { + dialog.set_content(TextView::new(msg)); + return; + } + // For rebooting, we just need to quit the installer, our caller does the actual reboot. siv.add_layer( Dialog::text(msg) .title(title) - .button("Reboot now", Cursive::quit), + .button("Reboot now", Cursive::quit) + .with_name(DIALOG_ID), ); let autoreboot = siv -- 2.41.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-26 14:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-07-26 14:03 [pve-devel] [PATCH installer 0/7] refactor; improve installation progress Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 1/7] tui: move install progress dialog into own view module Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 2/7] tui: install progress: split out low-level installer spawing into own function Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 3/7] tui: install progress: split out reboot handling " Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 4/7] tui: install progress: split out prompt logic " Christoph Heiss 2023-07-26 14:03 ` [pve-devel] [PATCH installer 5/7] tui: install progress: handle errors in ui message loop more gracefully Christoph Heiss 2023-07-26 14:04 ` [pve-devel] [PATCH installer 6/7] low-level: avoid open-coding config reading, parsing and merging Christoph Heiss 2023-07-26 14:04 ` [pve-devel] [PATCH installer 7/7] low-level, tui: count down auto-reboot timeout Christoph Heiss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox