Skip to content

Detect boot partition regardless of boot flags if mounted on /boot#3303

Merged
Torxed merged 1 commit intoarchlinux:masterfrom
Mintsuki:is_boot-fix
Mar 29, 2025
Merged

Detect boot partition regardless of boot flags if mounted on /boot#3303
Torxed merged 1 commit intoarchlinux:masterfrom
Mintsuki:is_boot-fix

Conversation

@Mintsuki
Copy link
Copy Markdown
Contributor

PR Description:

The boot partition is always to be mounted to /boot (afaik). This allows skipping potentially misleading checks of partition type to determine whether to use a certain partition as /boot, if it is already mounted there.

Tests and Checks

  • I have tested the code!

@Mintsuki Mintsuki requested a review from Torxed as a code owner March 26, 2025 11:24
@Mintsuki
Copy link
Copy Markdown
Contributor Author

@svartkanin request review, please.

@svartkanin
Copy link
Copy Markdown
Collaborator

Looks good from my side, I believe the original code was implemented by @codefiles so I will wait for a potential review otherwise happy to merge in a day or so

@codefiles
Copy link
Copy Markdown
Contributor

@Torxed
Copy link
Copy Markdown
Member

Torxed commented Mar 29, 2025

if self.mountpoint is not None:
	return Path('/boot') == self.mountpoint
else:
	return any(set(self.flags) & set(self._boot_indicator_flags))

I'm wondering if it's wiser to check the flags first? And then use the path as a fallback? At least checking the path lastly would make sense, in case the mount point isn't set yet (which would cause the else to not trigger).

return any(set(self.flags) & set(self._boot_indicator_flags)) or Path('/boot') == self.mountpoint

It seems strange that none of the boot flags from _boot_indicator_flags is set correctly in self.flags, but the mount point would be?

@Mintsuki
Copy link
Copy Markdown
Contributor Author

@Torxed

It seems strange that none of the boot flags from _boot_indicator_flags is set correctly in self.flags, but the mount point would be?

But it is a real scenario that does happen when using a pre-mounted configuration, and in general the _boot_indicator_flags are quite fragile from my testing. They expect certain partition types to be set for a /boot partition to be recognised as such, even when a user is explicitly mounting it on /boot...

IMO if there is a partition mounted on /boot already, it should be assumed that that is the boot partition, regardless of whether it has the right flags set or not.

Other than that, the point being raised about the order of checks and the issue with the else block not being entered under certain conditions is perfectly valid, and I will update the PR to use the line of code you shared.

@Torxed
Copy link
Copy Markdown
Member

Torxed commented Mar 29, 2025

Thanks for being accommodating, I agree that it might be fragile.
Curious why the flags aren't detected on mounted volumes, but I don't have the time to debug why currently. So lets get this in :)

@Torxed Torxed merged commit 9f7c3ba into archlinux:master Mar 29, 2025
8 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