Fix GRUB with non-/boot ESP

[Weissnix4711]

• This fix issue: EFI system partition can't be mounted at /efi #2001

PR Description:

Changes get_efi_partition() logic to include partitions with ESP flag. Also had to change get_boot_partition() to prevent it returning the ESP even if that partition does not have the boot flag. Not that there currently is a way to set the ESP flag from the guided menu. But I hope that'll be added eventually.

Grub can now use a custom boot directory with UEFI, though this is untested and probably should never be done. Will actually use the correct ESP mount path now, and will make the config to the correct location.

All untested. There's probably better ways to do this too, but I don't have time to faff about with this, so if someone could take over from here, please do.

Tests and Checks

• I have tested the code!
  

[Torxed]

Nicely done, and clean fix.
I suspect this might collide and cause some merge conflicts with your overhauls @svartkanin, just a heads up!

[svartkanin]

@Weissnix4711 apologies for the delay, unfortunately the PR conflicts with some other changes that got merged. Would you mind resolving the conflicts then I think it's good to go

Also could you please run the flake8/mypy checks as these seem to fail

[Weissnix4711]

Should be good now. 🤞

[codefiles]

Should be good now. 🤞

It is not; there are errors in the mypy check.

archinstall/lib/installer.py:879: error: Item "None" of "Optional[PartitionModification]" has no attribute "dev_path" [union-attr]
archinstall/lib/installer.py:884: error: Item "None" of "Optional[PartitionModification]" has no attribute "mountpoint" [union-attr]
archinstall/lib/installer.py:899: error: Item "None" of "Optional[PartitionModification]" has no attribute "mountpoint" [union-attr]
archinstall/lib/installer.py:901: error: Item "None" of "Optional[PartitionModification]" has no attribute "dev_path" [union-attr]
archinstall/lib/installer.py:903: error: Item "None" of "Optional[PartitionModification]" has no attribute "safe_dev_path" [union-attr]
archinstall/lib/installer.py:915: error: Item "None" of "Optional[PartitionModification]" has no attribute "dev_path" [union-attr]
...
Found 6 errors in 1 file (checked 108 source files)
Error: Process completed with exit code 1.

You have the function parameters boot_partition and efi_partition typed as Optional[disk.PartitionModification] which means they could be None. In the case of None there would be no attributes so that needs to be checked for before attempting to accessing them.

[Torxed]

@Weissnix4711 apologies for the delay in merging this.
Would you mind fixing the remaining mypy checks as well as merging in the latest changes from master?

That way we can get this in for the next release due in a couple of days :)

[Torxed]

I merged the latest changes from master.
Once concern I have is the dismissal of the boot partition.

If boot_partition is None, --boot-directory= is skipped, leaving --efi-directory alone.
I would if efi_partition is empty, I'd expect the --bot-directory to be alone.

So I'm wondering if this makes more sense?

f'--efi-directory={f"{efi_partition.mountpoint} " if efi_partition else f"{boot_partition.mountpoint}/efi"}'
f'--boot-directory={boot_partition.mountpoint} '

As I think /boot is required elsewhere, and /efi is an optional enhancement?
And make boot_partition non optional:

boot_partition: disk.PartitionModification,

[Torxed]

(This will be the last planned PR before I release the new version)

[Weissnix4711]

If boot_partition is None, --boot-directory= is skipped

Yup. In this case grub-install defaults to /boot. This is presumably what we want if there is no boot partition (ie. /boot will be on root), no?

Not sure if defaulting to {boot_partition.mountpoint}/efi is a good idea when efi_partition is None. This might likely result in a situation where the firmware is unable to find the EFI binaries. Certainly it makes setting --removable redundant.

[Torxed]

Ah ok. We should probably set the boot directory explicitly anyway.

The EFI I'm open for suggestions what the best sane place is.

The --removable started as a workaround for some known bugs actually. But what you say makes sense too :)

[Weissnix4711]

The efi dir should really only be on some subdir of a suitable FAT32 partition. If efi_partition is None, that presumably means we found no suitable partition, so should error out or install only as legacy (i386-pc). At least that's my thinking.

It is possible the system may support other filesystems beyond the scope of the UEFI spec, and so the user may wish to proceed with setting the efi directory to something 'weird', but I don't see an easy way of achieving this customisability. Also, the user will realistically run through the manual install process anyway.

Keeping --removable would certainly be useful. In most 'normal' installations, where the efi dir is actually set to the mountpoint of the ESP, this removes any chance of efibootmgr messing up the boot entry and the firmware should just pick up the efi binaries regardless.

I will have it set boot_directory explicitly.

[Weissnix4711]

As for typing fixes, I'm not 100% sure how to proceed. I'm not a python dev :)

Logically, at least one of boot or efi dir must be passed to the _add_grub_bootloader function. How this is done using python's type hints, I don't know.

I was originally hoping someone would just take over this PR for me lol

[Torxed]

Hehe, I can fix the typing stuff in a couple of hours :)

As for all the other reasoning, I agree with it. I forgot this logic is specifically for EFI boot. And that we're really only need to consider that scenario :D

[svartkanin]

I pushed the fixes for mypy and flake8 should be good now

[Torxed]

Awesome, I'm on my phone so hard to check. But could we add an exception if both boot and efi is none? :)

[svartkanin]

@Torxed I don't think the boot partition can be none actually, we're doing a validation here https://github.com/Weissnix4711/archinstall/blob/e87bf500c30eff5277be8cb2373b045e6a0d3888/archinstall/lib/installer.py#L1128-L1128 before calling the grub bootloader installation so that wouldn't pass

[Torxed]

Fair enough, works for now. It's best to put a validation on the function performing the task tho since people might get creative and call the boot loader functions directly hehe.
But for this release this is fine :D

[svartkanin]

Yeah that's fair, however, the method header does specify the boot partition argument as a object type not an optional object. So it would be a user error if someone calls it with none :)

[HanM23]

Hi guys,

Could you please tell me if this has been tested with luks (luks2) encryption e.g

• grub as bootloader
• /efi fat32 partition
• encrypted btrfs filesystem with recommanded subvolume layout ?

Thanks

EDIT : That does not work with encrypted or unencrypted root. I always end up with that error /efi/grub/grub.cfg no such file or directory