efi: Choose update layout depending on /usr/lib/efi#1089
efi: Choose update layout depending on /usr/lib/efi#1089Johan-Liebert1 merged 2 commits intocoreos:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors EFI metadata generation to support both legacy and newer image layouts by checking for EFI files in /usr/lib/ostree-boot and /usr/lib/efi. It introduces helper functions for metadata generation and file transfers. Review feedback highlighted a critical bug where a path existence check was performed against the host filesystem instead of the target sysroot, as well as suggestions for more idiomatic path handling and fixing a potential double-slash in path construction.
Johan-Liebert1
force-pushed
the
usr-lib-efi-backwards-compat
branch
from
fde73b8 to
14cab82
Compare
Johan-Liebert1
force-pushed
the
usr-lib-efi-backwards-compat
branch
4 times, most recently
from
db36a9c to
d81d546
Compare
jbtrystram
left a comment
jbtrystram
left a comment
There was a problem hiding this comment.
Superficial review since I am not really knowledgeable about this, but it feels a bit weird overall.
If i understand this, this is addressing the case where a disk image was created using a more recent bootupd version that's what actually ships into the image. This is essentially a bug from the build system.
Also, why not simply always symlink usr/lib/bootupd/updates to /lib/ostree-boot/EFI when the second exist but not the first, instead of moving things around ?
I guess there is already migration logic the other way around that will happen later ?
| let Some(efi_components) = get_efi_component_from_usr(sysroot_path, EFILIB)? else { | ||
| anyhow::bail!("Failed to find EFI components"); | ||
| }; |
There was a problem hiding this comment.
That's an unusual way of handling the unwrapping.
Why do you need to bail here ? Can't the error simply be bubbled up ?
There was a problem hiding this comment.
get_efi_component_from_usr returns Result<Option<T>>, the let .. else {} is to handle the Option part after the result is bubbled up by the ? operator
There was a problem hiding this comment.
Ah, T is an option already, I missed that. There is flatten that could work I guess.
| } | ||
| modules_vec.sort_unstable(); | ||
|
|
||
| // change to now to workaround https://github.com/coreos/bootupd/issues/933 |
There was a problem hiding this comment.
Should we account for SOURCE_DATE_EPOCH here ?
See #1075
There was a problem hiding this comment.
This does sound like a one liner fix, afaics we are comparing package names and version for sorting and not the timestamps. This could be a separate PR though. wdyt?
Johan-Liebert1
changed the title
|
Reading this PR, I've realized that the code in #955 is incorrectly copying things from /usr/lib/ostree-boot to /usr/lib/efi in the /usr/lib/efi case when it should only look at what files are present in /usr/lib/ostree-boot to generate the metadata but read the files from /usr/lib/efi (they should be an exact copy but well). |
wouldn't /usr/lib/ostree-boot not have anything in the /usr/lib/efi case? |
|
Damned, that's likely right and don't remember what we checked the last time :/ But the code does not read like that (I need to re-read that). |
In coreos#995 changes were made to move files from `usr/lib/ostree-boot/EFI` to `usr/lib/efi` instead of `usr/lib/bootupd` which caused issues with tools that expect the older layout. We now check for the existence of `usr/lib/efi`, and only use the new layout if we find it. Else, default back to the older layout at `usr/lib/bootupd`. Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Update e2e tests to handle both, new i.e. `/usr/lib/efi`, and old i.e. `/usr/lib/bootupd/updates` layouts, depending upon the existence of `/usr/lib/efi` Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
Johan-Liebert1
force-pushed
the
usr-lib-efi-backwards-compat
branch
from
d81d546 to
fbb5911
Compare
Johan-Liebert1
changed the title
travier
left a comment
travier
left a comment
There was a problem hiding this comment.
We updated this one together with Pragyan. LGTM
3 participants
In #995 changes were made to move files from
usr/lib/ostree-boot/EFItousr/lib/efiwhich caused issues with older bootupd failing to understand the new layout created by newer version of bootupd. We will start putting grub, shim and other related stuff in /usr/lib/efi starting fedora44 while older releases still have them in /usr/lib/ostree-boot, we need bootupd to properly handle both cases.To make the change backwards compatible, we now check for the existence of
usr/lib/efi, and only use the new layout if we find it. Else, default back to the older layout atusr/lib/bootupdFixes: #1057