Skip to content

Limine: use UUID for accessing boot partition if not same as ESP#3267

Merged
svartkanin merged 1 commit intoarchlinux:masterfrom
Mintsuki:limine-update3
Mar 20, 2025
Merged

Limine: use UUID for accessing boot partition if not same as ESP#3267
svartkanin merged 1 commit intoarchlinux:masterfrom
Mintsuki:limine-update3

Conversation

@Mintsuki
Copy link
Copy Markdown
Contributor

@Mintsuki Mintsuki commented Mar 17, 2025

No description provided.

@Mintsuki Mintsuki changed the title limine update3 Limine: use UUID for accessing boot partition Mar 17, 2025
@Mintsuki Mintsuki changed the title Limine: use UUID for accessing boot partition Limine: use UUID for accessing boot partition if not same as ESP Mar 17, 2025
@Mintsuki Mintsuki force-pushed the limine-update3 branch 2 times, most recently from ca80144 to 848d00a Compare March 17, 2025 07:04
@Mintsuki Mintsuki marked this pull request as ready for review March 17, 2025 07:09
@Mintsuki Mintsuki requested a review from Torxed as a code owner March 17, 2025 07:09
@Mintsuki Mintsuki marked this pull request as draft March 17, 2025 10:47
@Torxed
Copy link
Copy Markdown
Member

Torxed commented Mar 19, 2025

@svartkanin I was debugging this today a bit, and realized an issue here: https://github.com/archlinux/archinstall/pull/3267/files#diff-283d51110d004467e254215ad93e4dfcf0188beb22d9f878bb7088c12076dd2eR1310

if efi_partition and boot_partition != efi_partition:
	path_root = f'uuid({boot_partition.partuuid})'

Which will also happen here:

if efi_partition and boot_partition != efi_partition:
bootctl_options.append(f'--esp-path={efi_partition.mountpoint}')
bootctl_options.append(f'--boot-path={boot_partition.mountpoint}')

Since the move to PartitionModification, all paths will be compared like this:

class-instance == class-instance

Rather than two paths, making them always True.

I just wanna check in and make sure I'm not missing anything on how we want to use PartitionModification, should be safe to patch this right:

- if efi_partition and boot_partition != efi_partition:
+ if efi_partition and boot_partition.mountpoint != efi_partition.mountpoint:

Perhaps we should also add a __eq__ operator on the classes to avoid confusion, just raising an exception if people to do compare the classes?

@codefiles
Copy link
Copy Markdown
Contributor

codefiles commented Mar 19, 2025

Which will also happen here:

if efi_partition and boot_partition != efi_partition:
bootctl_options.append(f'--esp-path={efi_partition.mountpoint}')
bootctl_options.append(f'--boot-path={boot_partition.mountpoint}')

Since the move to PartitionModification, all paths will be compared like this:

class-instance == class-instance

Rather than two paths, making them always True.

I just wanna check in and make sure I'm not missing anything on how we want to use PartitionModification, should be safe to patch this right:

- if efi_partition and boot_partition != efi_partition:
+ if efi_partition and boot_partition.mountpoint != efi_partition.mountpoint:

Perhaps we should also add a __eq__ operator on the classes to avoid confusion, just raising an exception if people to do compare the classes?

@Torxed, I wrote that code (d76f4a0), it is fine.

If this issue stems from you trying to use /efi as the ESP, there is currently no code to handle the fact that mkinitcpio is configured by default to place img files in /boot. See the related issue #3185.

@Mintsuki
Copy link
Copy Markdown
Contributor Author

Which will also happen here:

if efi_partition and boot_partition != efi_partition:
bootctl_options.append(f'--esp-path={efi_partition.mountpoint}')
bootctl_options.append(f'--boot-path={boot_partition.mountpoint}')

Since the move to PartitionModification, all paths will be compared like this:

class-instance == class-instance

Rather than two paths, making them always True.
I just wanna check in and make sure I'm not missing anything on how we want to use PartitionModification, should be safe to patch this right:

- if efi_partition and boot_partition != efi_partition:
+ if efi_partition and boot_partition.mountpoint != efi_partition.mountpoint:

Perhaps we should also add a __eq__ operator on the classes to avoid confusion, just raising an exception if people to do compare the classes?

@Torxed, I wrote that code (d76f4a0), it is fine.

Can you please explain what is wrong with this PR and #3272 ? Why does this not work? It is true that the comparison is probably fine, but the 2 variables are the exact same, always...

@codefiles
Copy link
Copy Markdown
Contributor

@Mintsuki, sorry, I edited my comment. Please see the context that I added.

@Mintsuki
Copy link
Copy Markdown
Contributor Author

@Mintsuki, sorry, I edited my comment. Please see the context that I added.

fwiw it does not work if the ESP is set to /boot/efi, or pretty much anything really

@codefiles
Copy link
Copy Markdown
Contributor

@Mintsuki, let's discuss this in an issue first. It is a waste of time to try to create a solution before properly understanding/identifying the issue.

@Mintsuki
Copy link
Copy Markdown
Contributor Author

@Mintsuki, let's discuss this in an issue first. It is a waste of time to try to create a solution before properly understanding/identifying the issue.

sure, as you please

@Mintsuki Mintsuki marked this pull request as ready for review March 20, 2025 08:13
@Mintsuki
Copy link
Copy Markdown
Contributor Author

@codefiles as per #3275 , this PR should be fine (despite the fact that I have never gotten it to work with separate /boot/ESP (as FAT partitions)). Marked ready for review.

@svartkanin svartkanin merged commit 931f47a into archlinux:master Mar 20, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants