Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for the riscv64 architecture in the discoverable partition specification. The reviewer pointed out that for full support, the riscv64 architecture must also be added to the ARCH_USES_EFI constant in crates/lib/src/install.rs to ensure proper EFI-related installation steps are performed.
| } else if #[cfg(target_arch = "riscv64")] { | ||
| ROOT_RISCV64 |
There was a problem hiding this comment.
While adding riscv64 support here is correct, it appears that riscv64 also needs to be added to the ARCH_USES_EFI constant in crates/lib/src/install.rs to ensure full support for this architecture.
On RISC-V 64-bit systems, UEFI is the standard boot environment for modern Linux distributions. Without updating ARCH_USES_EFI, the installation process (specifically clean_boot_directories) will skip essential EFI-related steps like mounting and cleaning the ESP, which will likely lead to a broken installation on RISC-V hardware.
Please consider updating crates/lib/src/install.rs as well:
pub(crate) const ARCH_USES_EFI: bool = cfg!(any(target_arch = "x86_64", target_arch = "aarch64", target_arch = "riscv64"));
cgwalters
left a comment
There was a problem hiding this comment.
Looks sane, hmm....I wonder if we could change this whole thing to a table instead of list-of-const, then we wouldn't need this lookaside function.
(We'd need a mapping between rust arch const and the DPS names though)
I'm trying to use bootc on a riscv64 machine, this_arch_root returns "Unsupported architecture" error
The root partition GUID is known: ROOT_RISCV64