* [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met
@ 2023-07-07 11:53 Noel Ullreich
2023-07-10 7:27 ` Christoph Heiss
0 siblings, 1 reply; 4+ messages in thread
From: Noel Ullreich @ 2023-07-07 11:53 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met
2023-07-07 11:53 [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met Noel Ullreich
@ 2023-07-10 7:27 ` Christoph Heiss
2023-07-10 9:00 ` Noel Ullreich
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Heiss @ 2023-07-10 7:27 UTC (permalink / raw)
To: Proxmox VE development discussion, Noel Ullreich
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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met
2023-07-10 7:27 ` Christoph Heiss
@ 2023-07-10 9:00 ` Noel Ullreich
2023-07-10 9:08 ` Christoph Heiss
0 siblings, 1 reply; 4+ messages in thread
From: Noel Ullreich @ 2023-07-10 9:00 UTC (permalink / raw)
To: Christoph Heiss, Proxmox VE development discussion
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
>>
>>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met
2023-07-10 9:00 ` Noel Ullreich
@ 2023-07-10 9:08 ` Christoph Heiss
0 siblings, 0 replies; 4+ messages in thread
From: Christoph Heiss @ 2023-07-10 9:08 UTC (permalink / raw)
To: Proxmox VE development discussion, Noel Ullreich
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).
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-10 9:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-07 11:53 [pve-devel] [PATCH pve_installer v2] tui: don't abort install if min ram requirement is not met Noel Ullreich
2023-07-10 7:27 ` Christoph Heiss
2023-07-10 9:00 ` Noel Ullreich
2023-07-10 9:08 ` Christoph Heiss
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox