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

[Noel Ullreich <n.ullreich@proxmox.com>]

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