-
Notifications
You must be signed in to change notification settings - Fork 33
Extend generate-update-metadata()
to read from /usr
#938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
01b09e0
to
be19115
Compare
54d905f
to
fa2e65c
Compare
fa2e65c
to
2a8961f
Compare
ae28fbc
to
efa3b42
Compare
Ready for reviewing now. I did testing under fedora-bootc container, and upgrade Build new version according to #926 (comment): Download and prepare the repo
Build patch
Run testing under fedora-bootc container
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks sane to me, a variety of comments. I don't have anything blocking.
I think we should be testing this scenario in CI, I doubt we are? I am not up to date on things, does it require us pulling in a copr?
src/efi.rs
Outdated
let dest_efidir = component_updatedir(sysroot_path, self); | ||
|
||
if ostreebootdir.exists() { | ||
// New EFI dir /usr/lib/efi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if this this specific to the new Fedora grub/shim packages? I suspect it is...we may want to somehow make this a build time or even runtime conditional so in theory it's more pluggable/controllable by others.
I'd at least factor it out into a const
somewhere that explains where it came from.
Hmm actually, this topic also strongly relates to #766 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the reviewing!
Do you know if this this specific to the new Fedora grub/shim packages? I suspect it is...we may want to somehow make this a build time or even runtime conditional so in theory it's more pluggable/controllable by others.
Yes.
I'd at least factor it out into a
const
somewhere that explains where it came from.
SGTM.
Hmm actually, this topic also strongly relates to #766 right?
Actually this is related to issue #926 (comment), but we can extend it support #766 too.
src/efi.rs
Outdated
Ok(acc) | ||
}); | ||
packagesystem::query_files(sysroot_path, all_files?.into_iter())? | ||
} else if ostreebootdir.exists() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be cleaner perhaps if we checked first for ostreebootdir
, and migrated it to usr/lib/efi
if that doesn't exist? It should be an error if both exist.
Then we get closer to thinking of the ostreebootdir
one as legacy.
src/efi.rs
Outdated
}) | ||
.collect::<Vec<String>>(); | ||
|
||
Command::new("mv") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have an opportunity here to make usr/lib/efi
the standard path actually if it exists, maybe we could make things as simple as the equivalent of ln -sr /usr/lib/efi /usr/lib/bootupd/updates/EFI
?
EDIT: Ah I see it's not that simple based on find_all_efi_dirs
.
Hmmm...but actually I like the idea of that layout, what if we tried to adopt that as the standard and migrate our current EFI layout to it? (It'd break updates for older bootupd though...without having dual layouts for a while, ug)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...but actually I like the idea of that layout, what if we tried to adopt that as the standard and migrate our current EFI layout to it? (It'd break updates for older bootupd though...without having dual layouts for a while, ug)
Yes, you are right, that will break old bootupd. The current EFI.json is like:
# cat EFI.json | jq
{
"timestamp": "2025-03-27T10:27:15Z",
"version": "grub2-efi-x64-1:2.12-28.fc42.x86_64,shim-x64-15.8-3.x86_64"
}
We only get /usr/lib/efi/grub2/2.12-34.fc43/EFI
(-> grub2-2.12-34.fc43
) and /usr/lib/efi/shim/15.8-4/EFI
(-> shim-15.8-4
) from the path without rpmdb, does this make sense? For silverblue we install both shim-ia32
and shim-x64
, it only shows once, we do not care about this if we only concerns the version.
{
"timestamp": "<now>",
"version": "grub2-2.12-34.fc43,shim-15.8-4"
}
When we do the update, we sync all the files under /usr/lib/bootupd/updates/EFI
to /boot/efi/EFI
, means we only apply once, but if there are 2 or more directories, we need to sync each EFI directory, any good suggestion for this?
src/efi.rs
Outdated
@@ -615,6 +656,29 @@ fn find_file_recursive<P: AsRef<Path>>(dir: P, target_file: &str) -> Result<Vec< | |||
Ok(result) | |||
} | |||
|
|||
// Find EFI dirs under usr/lib/efi | |||
// for exmaple: usr/lib/efi/shim/15.8-4/EFI, usr/lib/efi/grub2/2.12-34.fc42/EFI | |||
fn find_all_efi_dirs(sysroot_lib: &Path) -> Result<Vec<PathBuf>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW looking at this layout, I think we can now avoid invoking rpm
to query the file information for these which would be a huge side benefit.
So again I think this relates to #766 in that perhaps we make this layout our new "API" for adding content in the ESP?
Actually thinking about things here...you know, it probably wouldn't be terribly hard to change what rpm-ostree does to automatically do this instead (via an opt-in). That'd require some coordination but the powerful benefit is we'd effectively automatically "backport" support for /usr/lib/efi
even for older OSes which seems like it'd help us a lot here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to clarify this, what we currently do in generate-update-metadata()
is move files under legacy ostree /usr/lib/ostree-boot/efi/EFI
to /usr/lib/bootupd/updates/EFI
, then invoke rpm to query the file information (need to insert the /boot/efi/EFI
to get the correct path) and create EFI.json
to include the package info
What we want to change is: retrieve /usr/lib/efi
and get EFI path, then add the package info from path and create EFI.json. The change might be easy.
What I am concern is for the installation and update, if there are 2 or more directories, we need to change the current logic to sync each EFI directory, instead of only /usr/lib/bootupd/updates/EFI
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want to change is: retrieve /usr/lib/efi and get EFI path, then add the package info from path and create EFI.json. The change might be easy.
Yeah I think so.
What I am concern is for the installation and update, if there are 2 or more directories, we need to change the current logic to sync each EFI directory, instead of only /usr/lib/bootupd/updates/EFI. WDYT?
Yeah, I think that would make sense. However it would mean we need to bridge between the current list of files in bootupd-state.json
vs the split directories.
Anyways in the short term what you're doing here (not changing the payload layout) is probably what we have to do in order to retain backwards compat (i.e. older clients can upgrade).
But after this work lands it'd probably be useful to try to start some work on making /usr/lib/efi
style layout be supported; something like bootupctl backend install --format-version=2
as an opt in or so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start some work on making
/usr/lib/efi
style layout be supported;
SGTM.
src/efi.rs
Outdated
// Find EFI dirs under usr/lib/efi | ||
// for exmaple: usr/lib/efi/shim/15.8-4/EFI, usr/lib/efi/grub2/2.12-34.fc42/EFI | ||
fn find_all_efi_dirs(sysroot_lib: &Path) -> Result<Vec<PathBuf>> { | ||
const LIBDIRS: &[&str] = &["grub2", "shim"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have : const LIBDIRS: &[&str] = &["grub2", "shim","."];
So that we can keep in things in /usr/lib/efi/<version>/EFI/<files>
, EFI.json metadata can contain the rpm details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can remove the limitation and scan all EFI directories like /usr/lib/efi/<name>/<version>/EFI/<files>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That can be one method, we would only need to find a name for putting things to esp, which is trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not consider files outside of a <name>/<version>
directory here as we need to be able to know "where" they come from / attach they to a package/source, even if it's user specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is reliance on <name>/<version>
to remove dependency on rpm -qf
which does not work when files are copied around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is reliance on
<name>/<version>
to remove dependency onrpm -qf
which does not work when files are copied around?
Yes, I think it will not work. In future, will rely on that to get meta for package.
4cac136
to
7241cdf
Compare
Look at more about #938 (comment), to clarify:
Consequently, need to change the install to copy each directory to the destination "/boot/efi". WDYT? |
@HuijingHei thank you for thinking this through, looks pretty exhaustive and cover most of the use-cases.
|
Not sure how to point the file |
I see this in cli/bootupd.rs is restricted to
The package installs the binary for all the boards, but rpi needs it be in Also want to understand what will happen if: |
I think probably no, as it can not find the correct path. |
515a6a1
to
ded5a25
Compare
ded5a25
to
71fae7b
Compare
ac2f9fa
to
faeed73
Compare
faeed73
to
0891e76
Compare
f5cf6cb
to
c93cc11
Compare
Yes per previous discussion it's a backwards incompatible change to use the new path - the old bootupd (before this PR) won't know what to do with data generated by bootupd after this PR. Let's define some terms here: Layouts:/boot/efi/EFIThe default place for Fedora grub/shim RPM packages. /usr/lib/ostree-bootWhere rpm-ostree moves the above to by default today and what bootupd consumes. /usr/lib/bootupd/updates/EFIThis is content owned by bootupd. /usr/lib/efi/<component>/<version>What the proposed new Fedora layout uses (and we could try to champion cross-OS/distro). So for now we need to go: /usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI But ideally in a future PR we teach bootupd to also know how to directly check for updates in /usr/lib/efi/<component> for EFI. We'll need to ship the code that knows how to consume those long before people can switch the default though. |
Will do this in future PR. |
Build new version according to #926 (comment): Do testing in
Edit: |
Copy Cloin's comment from coreos#938 (comment): Now we have: `/usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI` With this patch, will extend `/usr/lib/efi/<component> -> /usr/lib/bootupd/updates/EFI`
c93cc11
to
ca42239
Compare
Copy Cloin's comment from coreos#938 (comment): Now we have: `/usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI` With this patch, will extend `/usr/lib/efi/<component> -> /usr/lib/bootupd/updates/EFI`
ca42239
to
2f01d35
Compare
Copy Cloin's comment from coreos#938 (comment): Now we have: `/usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI` With this patch, will extend `/usr/lib/efi/<component> -> /usr/lib/bootupd/updates/EFI`
2f01d35
to
7839d81
Compare
Updated to use path instead of
See the
|
One issue about this PR is the old timestamp
|
Copy Cloin's comment from coreos#938 (comment): Now we have: `/usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI` With this patch, will extend `/usr/lib/efi/<component> -> /usr/lib/bootupd/updates/EFI`
7839d81
to
8f30e1a
Compare
Copy Cloin's comment from #938 (comment):
Now we have:
/usr/lib/ostree-boot -> /usr/lib/bootupd/updates/EFI
With this patch, will extend
/usr/lib/efi/<component> -> /usr/lib/bootupd/updates/EFI
See #926