Fix Limine install with ESP mounted outside /boot#4442
Fix Limine install with ESP mounted outside /boot#4442svartkanin merged 4 commits intoarchlinux:masterfrom
Conversation
Place limine.conf next to the EFI binary on the ESP so it is found regardless of ESP mountpoint, and block unbootable layouts (non-UKI Limine with ESP not at /boot and no separate /boot partition) in GlobalMenu validation, guided.main() and _add_limine_bootloader(). Fixes archlinux#4333
|
apparently already fixed #4439, untested |
Not sure... |
|
As far as I can see, #4441 fixed a Path.copy() regression that broke Limine install even with a standard layout (that's the #4439 case). This PR targets a different scenario: ESP mounted outside /boot (e.g. /efi) and used as the boot partition, without UKI. In that case kernels land on the root ext4 under /boot/, which Limine cannot read, so the install "succeeds" but the system won't boot. The guards here (menu validator + installer + guided.py) prevent that config from reaching the install step. |
| pass | ||
|
|
||
|
|
||
| def _check_bootloader_layout(config: ArchConfig) -> str | None: |
There was a problem hiding this comment.
I think it would be better to put this into a util, something like a new lib/bootloader/utils.py would be good
There was a problem hiding this comment.
And this should be checked in the other scripts as well I suppose
There was a problem hiding this comment.
Extracted the check into lib/bootloader/utils.validate_bootloader_layout. On "other scripts": minimal.py hardcodes Systemd-boot and only_hd.py doesn't install a bootloader, so only guided.py needed it.
While at it I noticed this overlaps with global_menu._validate_bootloader (same Limine/ESP case in two places), and several menu-side gaps remain (Systemd/Efistub on BIOS, Efistub with non-FAT boot). Feels like a good follow-up PR - consolidate all bootloader layout validation into the new util and close those gaps.
I'll research it a bit later.
| ) | ||
|
|
||
| if efi_part and boot_part == efi_part and efi_part.mountpoint != Path('/boot'): | ||
| return ( |
There was a problem hiding this comment.
The validation should return a bool and then the caller should decide what to do with a false validation
There was a problem hiding this comment.
Also can we simplify this altogether and use it in all different places (global_menu, guided and installer)
class BooloaderValidationFailure(Enum):
LimineLayout = auto()
def description(self) -> str:
match self:
case BooloaderValidationFailure.LimineLayout:
return (
'Limine requires kernels on a FAT partition. The ESP is mounted at {efi_part.mountpoint}, '
'enable UKI or add a separate /boot partition to install Limine.'
)
def validate_bootloader_layout(
bootloader_config: BootloaderConfiguration | None,
disk_config: DiskLayoutConfiguration | None,
) -> BooloaderValidationFailure | None:
if not (bootloader_config and disk_config):
return None
# Limine can only read FAT. When the ESP is the boot partition but
# mounted outside /boot and UKI is disabled, the kernel ends up on the
# root filesystem which Limine cannot access.
if bootloader_config.bootloader == Bootloader.Limine and not bootloader_config.uki:
efi_part = next((p for m in disk_config.device_modifications if (p := m.get_efi_partition())), None)
boot_part = next((p for m in disk_config.device_modifications if (p := m.get_boot_partition())), None)
if efi_part and efi_part == boot_part and efi_part.mountpoint != Path('/boot'):
return BooloaderValidationFailure.LimineLayout
return None
There was a problem hiding this comment.
Thanks for the consolidation push. Tried the Enum.description() shape first but hit a snag: {efi_part.mountpoint} needs resolved partition context that isn't available inside an Enum method. Went with this instead:
class BootloaderValidationFailureKind(Enum):
LimineNonFatBoot = auto()
LimineLayout = auto()
@dataclass(frozen=True)
class BootloaderValidationFailure:
kind: BootloaderValidationFailureKind
description: str
description is built inside validate_bootloader_layout where partitions are in scope. Callers use failure.description (or match on .kind if they need to branch). Happy to rename or flatten if you'd prefer a different shape.
While there, pulled the FAT-boot check into the same util so all three call sites (GlobalMenu, guided.py, Installer._add_limine_bootloader) go through one function.
There was a problem hiding this comment.
@Softer not sure I'm following, why can't we pass that as a parameter?
def description(self, mountpoint: str | None = None):
...
There was a problem hiding this comment.
Agreed the current dataclass is redundant - description is derived from kind, no reason to store both. But a flat enum with description(mountpoint=None) trades one smell for another: the optional param lets LimineLayout.description() silently render "mounted at None" and accepts a mountpoint on kinds that don't need it.
What about a variant-per-context shape instead? Flat enum for context-free kinds, a frozen dataclass per parametrized kind, joined by a | None union:
class BootloaderValidationFailure(Enum):
LimineNonFatBoot = auto()
@property
def description(self) -> str: ...
@dataclass(frozen=True)
class LimineLayoutFailure:
efi_mountpoint: Path
@property
def description(self) -> str: ...
BootloaderValidationResult = BootloaderValidationFailure | LimineLayoutFailure | None
Each variant carries exactly the fields it needs (required at construction), callers still use failure.description, and new parametrized kinds add a new dataclass instead of inflating a shared signature.
What do you think of this option?
Move the boot-partition FAT check from GlobalMenu into validate_bootloader_layout so all three call sites (GlobalMenu, guided.py, Installer._add_limine_bootloader) share one function. Return a BootloaderValidationFailure dataclass (kind + description) instead of str | None, so callers can match on the failure kind and the description is built where partition context is in scope.
| from archinstall.lib.models.bootloader import Bootloader, BootloaderConfiguration | ||
| from archinstall.lib.models.device import DiskLayoutConfiguration, FilesystemType | ||
|
|
||
| _FAT_FILESYSTEMS = (FilesystemType.FAT12, FilesystemType.FAT16, FilesystemType.FAT32) |
There was a problem hiding this comment.
can this be a member function of the FilesystemType instead? That would it's encapsulate there and not spread across the codebase
There was a problem hiding this comment.
Hm... Yes, you're right. Reworked it.
Fixes #4333.
Selecting Limine with the ESP mounted at
/efior/boot/efi(without UKI or a separate/boot) left the system unbootable:limine.confended up on the root fs instead of the ESP, and the kernel path in the config pointed to a non-FAT volume that Limine cannot read.Changes
installer.py- writelimine.confnext to the EFI binary on the ESP so it is found regardless of ESP mountpoint.global_menu.py-_validate_bootloader()blocksInstallwhenLimine + ESP==boot + mountpoint != /boot + !UKI; preview explains why.guided.py- same pre-flight check inmain()before any filesystem operations, covering--silent/--configflow.installer.py-_add_limine_bootloader()raisesDiskErroron the same condition as a safety net.Same rule at three layers (UI / CLI / lib) for defense in depth. Can extract into a helper if preferred.
Tests
QEMU/OVMF, rebuilt ISO from this branch, fresh 30G qcow2 each run.
ESP=/boot, removable, non-UKI- installs and boots normally.ESP=/efi, removable, UKI- installs; Limine loads UKI from ESP.ESP=/efi, removable, non-UKI, separate FAT /boot- installs; Limine reads kernel from/boot.ESP=/efi, removable, non-UKI, --silent- exits witherror()before touching the disk.Installdisabled with explanation in preview; enabling UKI clears it.