Re: [pve-devel] [PATCH pve_installer] tui-installer: don't aborted install if min requirements not met

[Christoph Heiss <c.heiss@proxmox.com>]

Good catch, that's indeed a place were the GUI- and TUI-installer
diverge in behavior.

Small comment inline about code structuring.

As subject it's completely sufficient to use "tui: [..]", also; bit of
a typo'd subject in general? :^)

On Thu, Jul 06, 2023 at 01:52:12PM +0200, Noel Ullreich wrote:
> If the minimum requirements are not met, the TUI installer will create a
> popup notifying you that the install might not work and then exits the
> installer.
> While the GUI also creates such a popup, it will not exit the installer.
> This patch adapts the behavior of the GUI: the TUI creates a popup
> warning you that min spec is not met but doesn't abort the install.
>
> Signed-off-by: Noel Ullreich <n.ullreich@proxmox.com>
> ---
>  proxmox-tui-installer/src/main.rs   | 17 +++++++++++++++--
>  proxmox-tui-installer/src/system.rs | 14 +-------------
>  2 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
> index 64f21fa..e9a3fb4 100644
> --- a/proxmox-tui-installer/src/main.rs
> +++ b/proxmox-tui-installer/src/main.rs
> @@ -191,6 +191,8 @@ fn main() {
>          Err(err) => initial_setup_error(&mut siv, &err),
>      };
>
> +    has_min_requirements(&runtime_info, &mut siv);
> +
>      siv.clear_global_callbacks(Event::CtrlChar('c'));
>      siv.set_on_pre_event(Event::CtrlChar('c'), trigger_abort_install_dialog);
>
> @@ -243,8 +245,6 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
>              .map_err(|err| format!("Failed to retrieve runtime environment info: {err}"))?
>      };
>
> -    system::has_min_requirements(&runtime_info)?;
> -
>      runtime_info.disks.sort();
>      if runtime_info.disks.is_empty() {
>          Err("The installer could not find any supported hard disks.".to_owned())
> @@ -291,6 +291,19 @@ fn initial_setup_error(siv: &mut CursiveRunnable, message: &str) -> ! {
>      std::process::exit(1);
>  }
>
> +fn has_min_requirements(info: &RuntimeInfo, siv: &mut CursiveRunnable) -> () {
> +    if info.total_memory < 1024 {
> +        siv.add_layer(
> +            Dialog::around(TextView::new(concat!(
> +            "Less than 1 GiB of usable memory detected, installation will probably fail.\n\n",
> +            "See 'System Requirements' in the documentation.")))
> +                .title("Installer setup error")
> +                .button("Ok", Cursive::quit),
> +        );
> +        siv.run();
> +    }
You can move this check into installer_setup_late() directly and use
display_setup_warning(), elimimating has_min_requirements() completely.
It's the appropriate place for non-fatal checks like this, very much
like the HVM support check.

[ Or something along the lines of: move HVM check into
has_min_requirements(), and call that from installer_setup_late(). And
rename it to e.g. check_min_requirements(), to reflect that it does not
return anything. But absolutely optional, just some thoughts of mine. ]

> +}
> +
>  fn display_setup_warning(siv: &mut Cursive, message: &str) {
>      siv.add_layer(Dialog::info(message).title("Warning"));
>  }
> diff --git a/proxmox-tui-installer/src/system.rs b/proxmox-tui-installer/src/system.rs
> index baceab9..7378dba 100644
> --- a/proxmox-tui-installer/src/system.rs
> +++ b/proxmox-tui-installer/src/system.rs
> @@ -1,18 +1,6 @@
>  use std::{fs::OpenOptions, io::Write, process::Command};
>
> -use crate::setup::{KeyboardMapping, RuntimeInfo};
> -
> -pub fn has_min_requirements(info: &RuntimeInfo) -> Result<(), String> {
> -    if info.total_memory < 1024 {
> -        return Err(concat!(
> -            "Less than 1 GiB of usable memory detected, installation will probably fail.\n\n",
> -            "See 'System Requirements' in the documentation."
> -        )
> -        .to_owned());
> -    }
> -
> -    Ok(())
> -}
> +use crate::setup::{KeyboardMapping};
>
>  pub fn set_keyboard_layout(kmap: &KeyboardMapping) -> Result<(), String> {
>      Command::new("setxkbmap")
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>