Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions archinstall/lib/bootloader/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from pathlib import Path

from archinstall.lib.models.bootloader import Bootloader, BootloaderConfiguration
from archinstall.lib.models.device import DiskLayoutConfiguration


def validate_bootloader_layout(
bootloader_config: BootloaderConfiguration | None,
disk_config: DiskLayoutConfiguration | None,
) -> str | None:
"""Validate bootloader configuration against disk layout.

Returns an error message if the configuration would produce an
unbootable system, or None if it is valid.
"""
# 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 not (bootloader_config and bootloader_config.bootloader == Bootloader.Limine and not bootloader_config.uki and disk_config):
return None

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 boot_part == efi_part and efi_part.mountpoint != Path('/boot'):
return (
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation should return a bool and then the caller should decide what to do with a false validation

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Softer not sure I'm following, why can't we pass that as a parameter?

def description(self, mountpoint: str | None = None):
   ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

f'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.'
)
return None
6 changes: 6 additions & 0 deletions archinstall/lib/global_menu.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pathlib import Path
from typing import override

from archinstall.default_profiles.profile import GreeterType
Expand Down Expand Up @@ -492,6 +493,11 @@ def _validate_bootloader(self) -> str | None:
if bootloader == Bootloader.Limine:
if boot_partition.fs_type not in [FilesystemType.FAT12, FilesystemType.FAT16, FilesystemType.FAT32]:
return 'Limine does not support booting with a non-FAT boot partition'
if self._uefi and efi_partition and boot_partition == efi_partition and efi_partition.mountpoint != Path('/boot') and not bootloader_config.uki:
return (
f'Limine requires kernels on a FAT partition. The ESP is mounted at {efi_partition.mountpoint}, '
'enable UKI or add a separate /boot partition'
)

elif bootloader == Bootloader.Refind:
if not self._uefi:
Expand Down
16 changes: 11 additions & 5 deletions archinstall/lib/installer.py
Original file line number Diff line number Diff line change
Expand Up @@ -1466,6 +1466,16 @@ def _add_limine_bootloader(
elif not efi_partition.mountpoint:
raise ValueError('EFI partition is not mounted')

# Limine can only read FAT filesystems. When the ESP doubles as
# the boot partition but is mounted outside /boot (e.g. /efi,
# /boot/efi) and UKI is disabled, kernels end up on the root
# filesystem under /boot/ which Limine cannot access.
if boot_partition == efi_partition and efi_partition.mountpoint != Path('/boot') and not uki_enabled:
raise DiskError(
f'Limine requires kernels on a FAT partition. The ESP is mounted at {efi_partition.mountpoint}, '
'so enable UKI or add a separate /boot partition to install Limine.',
)

info(f'Limine EFI partition: {efi_partition.dev_path}')

parent_dev_path = get_parent_device_path(efi_partition.safe_dev_path)
Expand All @@ -1476,15 +1486,11 @@ def _add_limine_bootloader(
if bootloader_removable:
efi_dir_path = efi_dir_path / 'BOOT'
efi_dir_path_target = efi_dir_path_target / 'BOOT'

boot_limine_path = self.target / 'boot' / 'limine'
boot_limine_path.mkdir(parents=True, exist_ok=True)
config_path = boot_limine_path / 'limine.conf'
else:
efi_dir_path = efi_dir_path / 'arch-limine'
efi_dir_path_target = efi_dir_path_target / 'arch-limine'

config_path = efi_dir_path / 'limine.conf'
config_path = efi_dir_path / 'limine.conf'

efi_dir_path.mkdir(parents=True, exist_ok=True)

Expand Down
10 changes: 10 additions & 0 deletions archinstall/scripts/guided.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from archinstall.lib.applications.application_handler import ApplicationHandler
from archinstall.lib.args import ArchConfig, ArchConfigHandler
from archinstall.lib.authentication.authentication_handler import AuthenticationHandler
from archinstall.lib.bootloader.utils import validate_bootloader_layout
from archinstall.lib.configuration import ConfigurationOutput
from archinstall.lib.disk.filesystem import FilesystemHandler
from archinstall.lib.disk.utils import disk_layouts
Expand Down Expand Up @@ -211,6 +212,15 @@ def main(arch_config_handler: ArchConfigHandler | None = None) -> None:
config.write_debug()
config.save()

# Safety net for silent/config-file flow. The TUI menu blocks Install via
# GlobalMenu._validate_bootloader() before reaching this point.
if err_msg := validate_bootloader_layout(
arch_config_handler.config.bootloader_config,
arch_config_handler.config.disk_config,
):
error(err_msg)
return

if arch_config_handler.args.dry_run:
return

Expand Down