Skip to content

Conversation

@yasminvalim
Copy link
Contributor

TICKET: As part of https://fedoraproject.org/wiki/Changes/BootLoaderUpdatesPhase1, for package mode installations, we will update the bootloader (copy from /usr to boot or ESP) as part of the posttrans scriptlet.

This is a slightly different path as the current update path where we have the systemd-run logic, and we notably don't have any prepared metadata.

We can start by creating a command that does a simple copy (like the scripts do) and then iterate on merging the logic between the package mode and the image mode use case.

See change and current sample/basic installation scripts:

https://fedoraproject.org/wiki/Changes/BootLoaderUpdatesPhase1
https://src.fedoraproject.org/rpms/grub2/pull-request/136
https://src.fedoraproject.org/rpms/shim/pull-request/4

See related issue: #926

Extract duplicated ESP mounting, validation, and copying logic from install() and package_mode_copy_to_boot_impl() into shared helper function  to eliminate dupe code.
Addressing review: add unit test  that installs shim into a container and ensures that the files are properly setup in the right place
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a new copy-to-boot subcommand to bootupd. This is intended for package-based systems to update bootloader files from /usr/lib/efi to the ESP. The implementation adds a new method to the Component trait, with a concrete implementation for the Efi component. The logic for finding the ESP and copying files is well-encapsulated in new helper functions. The existing install logic is refactored to use this new shared code, which is a great improvement. Comprehensive tests for the new functionality have been added. The changes are well-structured and clear. I have one suggestion for simplification.

Comment on lines +250 to +252
let esp_device = esp_devices
.first()
.ok_or_else(|| anyhow::anyhow!("No ESP device found"))?;

Choose a reason for hiding this comment

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

medium

The .ok_or_else() call here is on a value that is guaranteed to be Some. The find_colocated_esps function returns None if no ESPs are found, which is handled by the let-else statement on line 246. If it returns Some(vec), the vector is guaranteed to be non-empty. Therefore, esp_devices.first() will never be None, and this error-handling code is unreachable.

You can simplify this by using .unwrap(), which is safe in this context. This also removes a potentially confusing error message.

Suggested change
let esp_device = esp_devices
.first()
.ok_or_else(|| anyhow::anyhow!("No ESP device found"))?;
let esp_device = esp_devices.first().unwrap();

@yasminvalim
Copy link
Contributor Author

Same as: #1021

I encountered some issues with the git history on my previous branch, so I've pushed this new branch to resolve it.

@travier
Copy link
Member

travier commented Nov 21, 2025

#1021 (comment):

@travier the command that we want to use is for package mode, also for image mode, is this right?

This command will be used exclusively on package mode.

@HuijingHei
Copy link
Member

#1021 (comment):

@travier the command that we want to use is for package mode, also for image mode, is this right?

This command will be used exclusively on package mode.

Thank you for the confirmation, then it would be easy for us to add such tmt test.

name = "copy-to-boot",
about = "Copy bootloader files from /usr/lib/efi to boot/ESP (package mode)"
)]
CopyToBoot,
Copy link
Member

Choose a reason for hiding this comment

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

Look at more about this, this means to use like bootupctl backend copy-to-boot from root /, is this right? How about if we want to use like copying from specified directory, so I think maybe it would better to be under install, but need to confirm with @travier , WDYT?

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