Re: [PATCH installer] assistant: add support for splitting ISO into (i)PXE-compatible files

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

Thanks for the review!

On Mon Mar 30, 2026 at 6:01 PM CEST, Thomas Lamprecht wrote:
[..]
>> +      --pxe [<TYPE>]
>> +          Instead of producing an ISO file, generate a 'initrd.img' and 'vmlinuz' file for use with
>> +          (i)PXE servers. The '--output' option must point to a directory to place these files in.
>> +
>> +          <TYPE> is optional. Possible value are 'syslinux', 'ipxe'. If <TYPE> is specified, a
>> +          configuration file is produced for the specified loader.
>
> The help text lists 'syslinux' as a possible value but FromStr doesn't
> handle it, so passing --pxe syslinux gives a confusing "unknown PXE
> loader 'syslinux'" error, let's drop that for now if it's currently not
> planned.

Yeah, bit of a leftover that slipped through.

[..]
>> +    fn parse(args: &mut cli::Arguments) -> Result<Self> {
>> +        let pxe_loader = match args.opt_value_from_str("--pxe") {
>> +            Ok(val) => val,
>> +            Err(cli::Error::OptionWithoutAValue(_)) => Some(PxeLoader::default()),
>> +            Err(err) => Err(err)?,
>> +        };
>
> This has a subtle interaction with pico_args' optional value handling:
> if --pxe directly precedes the positional input argument (e.g.
> `prepare-iso --fetch-from http --pxe input.iso`), pico_args will try
> to parse "input.iso" as a PxeLoader and fail with "unknown PXE loader
> 'input.iso'". Only works when --pxe is followed by another --flag or
> is given an explicit type value.
>
> Not a blocker since the same applies to other optional-value patterns
> with pico_args, but might be worth a note in the help text (e.g.
> "use '--pxe none' when no other flags follow"). Alternatively, use
> --pxe as flag and --pxe-type <type> where the value is not optional.

I'll switch it to the latter, with a separate option. Seems sane(r) than
trying to quash everything into one flag.

[..]
>> diff --git a/proxmox-installer-common/src/setup.rs b/proxmox-installer-common/src/setup.rs
>> [...]
>> +    pub fn full_name(&self) -> &'static str {
>> +        match self {
>> +            Self::PVE => "Proxmox Virtual Environment",
>> +            Self::PMG => "Proxmox Mail Gateway",
>> +            Self::PBS => "Proxmox Backup Server",
>> +            Self::PDM => "Proxmox Datacenter Manager",
>> +        }
>> +    }
>
> Minor: This is the third way to get a product display name (Display
> gives the short lowercase form, ProductConfig::fullname the "Proxmox
> VE"-style form, and now this one the full unabbreviated form). Fine for
> now, but worth keeping in mind and not lets add yet another one in the
> future ^^

Good point. Maybe using `PRODUCT_LONG` here from the cd-info would also
be fine too? It's the exact same string; apart from using "Proxmox VE"
instead of "Proxmox Virtual Environment".

If nothing speaks against that, I'd probably go that route instead,
otherwise I'll just keep it like it.

(_Really_ bikeshedding now, but it's the first thing users see, so my
thought here was to have it in exactly the same spelling as in the GRUB
menu of the ISO.)