[pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met

[pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met

[Noel Ullreich]

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>
---
changes from v1:
* moved the min ram check into `installer_setup_late`
* fixed spelling in the subject line

 proxmox-tui-installer/src/main.rs   | 14 ++++++++++++--
 proxmox-tui-installer/src/system.rs | 14 +-------------
 2 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 64f21fa..99a4a11 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -209,6 +209,8 @@ fn main() {
         in_test_mode,
     });
 
+    installer_setup_late(&mut siv);
+
     switch_to_next_screen(&mut siv, InstallerStep::Licence, &license_dialog);
     siv.run();
 }
@@ -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())
@@ -256,6 +256,7 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
 /// Anything that can be done late in the setup and will not result in fatal errors.
 fn installer_setup_late(siv: &mut Cursive) {
     let state = siv.user_data::<InstallerState>().unwrap();
+    let mem = state.runtime_info.total_memory;
 
     if !state.in_test_mode {
         let kmap_id = &state.options.timezone.kb_layout;
@@ -266,6 +267,15 @@ fn installer_setup_late(siv: &mut Cursive) {
         }
     }
 
+    if mem < 1024 {
+            display_setup_warning(
+                siv,
+                concat!(
+                "Less than 1 GiB of usable memory detected, installation will probably fail.\n\n",
+                "See 'System Requirements' in the documentation."),
+            );
+        }
+
     if setup_info().config.product == setup::ProxmoxProduct::PVE {
         let cpu_hvm = procfs::read_cpuinfo().map(|info| info.hvm).unwrap_or(false);
         if !cpu_hvm {
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

Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met

[Christoph Heiss]

On Fri, Jul 07, 2023 at 01:53:46PM +0200, Noel Ullreich wrote:
> [..]
>
> diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
> index 64f21fa..99a4a11 100644
> --- a/proxmox-tui-installer/src/main.rs
> +++ b/proxmox-tui-installer/src/main.rs
> @@ -209,6 +209,8 @@ fn main() {
>          in_test_mode,
>      });
>
> +    installer_setup_late(&mut siv);
> +
Please remove this, it does not have any (visible) effect here anyway.
installer_setup_late() is already called by switch_to_next_screen(),
as otherwise dialogs will get layered over it by the main content.

>      switch_to_next_screen(&mut siv, InstallerStep::Licence, &license_dialog);
>      siv.run();
>  }
> @@ -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())
> @@ -256,6 +256,7 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
>  /// Anything that can be done late in the setup and will not result in fatal errors.
>  fn installer_setup_late(siv: &mut Cursive) {
>      let state = siv.user_data::<InstallerState>().unwrap();
> +    let mem = state.runtime_info.total_memory;
Nit: Can be folded into the `if` condition directly. It isn't terribly
long nor the condition complicated, and it results in a nice "isolated"
block.

>
>      if !state.in_test_mode {
>          let kmap_id = &state.options.timezone.kb_layout;
> @@ -266,6 +267,15 @@ fn installer_setup_late(siv: &mut Cursive) {
>          }
>      }
>
> +    if mem < 1024 {
> +            display_setup_warning(
> +                siv,
> +                concat!(
> +                "Less than 1 GiB of usable memory detected, installation will probably fail.\n\n",
> +                "See 'System Requirements' in the documentation."),
> +            );
> +        }
Formatting is completely off here - just let rustfmt have a go at it :^)

> +
>      if setup_info().config.product == setup::ProxmoxProduct::PVE {
>          let cpu_hvm = procfs::read_cpuinfo().map(|info| info.hvm).unwrap_or(false);
>          if !cpu_hvm {
> 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};
Nit: Drop the extra { } pair. Something like that is also automatically
fixed by rustfmt.

>
>  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
>
>

Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met

[Noel Ullreich]

On 10-07-2023 09:27, Christoph Heiss wrote:
> On Fri, Jul 07, 2023 at 01:53:46PM +0200, Noel Ullreich wrote:
>> [..]
>>
>> diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
>> index 64f21fa..99a4a11 100644
>> --- a/proxmox-tui-installer/src/main.rs
>> +++ b/proxmox-tui-installer/src/main.rs
>> @@ -209,6 +209,8 @@ fn main() {
>>           in_test_mode,
>>       });
>>
>> +    installer_setup_late(&mut siv);
>> +
> Please remove this, it does not have any (visible) effect here anyway.
> installer_setup_late() is already called by switch_to_next_screen(),
> as otherwise dialogs will get layered over it by the main content.
>
>>       switch_to_next_screen(&mut siv, InstallerStep::Licence, &license_dialog);
>>       siv.run();
>>   }
>> @@ -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())
>> @@ -256,6 +256,7 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
>>   /// Anything that can be done late in the setup and will not result in fatal errors.
>>   fn installer_setup_late(siv: &mut Cursive) {
>>       let state = siv.user_data::<InstallerState>().unwrap();
>> +    let mem = state.runtime_info.total_memory;
> Nit: Can be folded into the `if` condition directly. It isn't terribly
> long nor the condition complicated, and it results in a nice "isolated"
> block.

This doesn't work:

```
error[E0499]: cannot borrow `*siv` as mutable more than once at a time
    --> proxmox-tui-installer/src/main.rs:263:39
     |
256 |     let state = siv.user_data::<InstallerState>().unwrap();
     |                 --------------------------------- first mutable 
borrow occurs here
...
263 |                 display_setup_warning(siv, &format!("Failed to 
apply keyboard layout: {err}"));
     |                                       ^^^ second mutable borrow 
occurs here
...
268 |     if state.runtime_info.total_memory < 1024 {
     |        ------------------------------- first borrow later used here

For more information about this error, try `rustc --explain E0499`.
error: could not compile `proxmox-tui-installer` due to previous error
```

That's why I created this extra variable.

>
>>       if !state.in_test_mode {
>>           let kmap_id = &state.options.timezone.kb_layout;
>> @@ -266,6 +267,15 @@ fn installer_setup_late(siv: &mut Cursive) {
>>           }
>>       }
>>
>> +    if mem < 1024 {
>> +            display_setup_warning(
>> +                siv,
>> +                concat!(
>> +                "Less than 1 GiB of usable memory detected, installation will probably fail.\n\n",
>> +                "See 'System Requirements' in the documentation."),
>> +            );
>> +        }
> Formatting is completely off here - just let rustfmt have a go at it :^)
>
>> +
>>       if setup_info().config.product == setup::ProxmoxProduct::PVE {
>>           let cpu_hvm = procfs::read_cpuinfo().map(|info| info.hvm).unwrap_or(false);
>>           if !cpu_hvm {
>> 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};
> Nit: Drop the extra { } pair. Something like that is also automatically
> fixed by rustfmt.
>
>>   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
>>
>>

Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met

[Christoph Heiss]

On Mon, Jul 10, 2023 at 11:00:58AM +0200, Noel Ullreich wrote:
>
> On 10-07-2023 09:27, Christoph Heiss wrote:
> > On Fri, Jul 07, 2023 at 01:53:46PM +0200, Noel Ullreich wrote:
> > > [..]
> > > @@ -256,6 +256,7 @@ fn installer_setup(in_test_mode: bool) -> Result<(LocaleInfo, RuntimeInfo), Stri
> > >   /// Anything that can be done late in the setup and will not result in fatal errors.
> > >   fn installer_setup_late(siv: &mut Cursive) {
> > >       let state = siv.user_data::<InstallerState>().unwrap();
> > > +    let mem = state.runtime_info.total_memory;
> > Nit: Can be folded into the `if` condition directly. It isn't terribly
> > long nor the condition complicated, and it results in a nice "isolated"
> > block.
>
> This doesn't work:
>
> ```
> error[E0499]: cannot borrow `*siv` as mutable more than once at a time
>    --> proxmox-tui-installer/src/main.rs:263:39
>     |
> 256 |     let state = siv.user_data::<InstallerState>().unwrap();
>     |                 --------------------------------- first mutable borrow
> occurs here
> ...
> 263 |                 display_setup_warning(siv, &format!("Failed to apply
> keyboard layout: {err}"));
>     |                                       ^^^ second mutable borrow occurs
> here
> ...
> 268 |     if state.runtime_info.total_memory < 1024 {
>     |        ------------------------------- first borrow later used here
>
> For more information about this error, try `rustc --explain E0499`.
> error: could not compile `proxmox-tui-installer` due to previous error
> ```
>
> That's why I created this extra variable.
Right, did not think about that, even had the same problem occur to me
last week while working on some other patches.

Then it is fine as-is of course, maybe just rename it to `total_memory`
as `mem` is quite generic (but that is really just bike-shedding it this
point).