packagesystem: make rpm as build-time configuration#983
packagesystem: make rpm as build-time configuration#983cgwalters merged 1 commit intocoreos:mainfrom
rpm as build-time configuration#983Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the package version comparison logic by introducing a Package struct with an Ord implementation. This is a good simplification of the previous logic based on BTreeMap.
However, I've found a critical issue in parse_evr_vec where the packages are not sorted before being passed to compare_package_slices, which can lead to incorrect comparisons. I've also suggested a small improvement to the Package struct definition to make it more idiomatic and maintainable by deriving Ord and PartialOrd.
src/packagesystem.rs
Outdated
| #[derive(Debug, Eq, PartialEq)] | ||
| struct Package<'a> { | ||
| name: String, | ||
| version: Evr<'a>, | ||
| } | ||
|
|
||
| // assume it is like "grub2-efi-x64-1:2.12-28.fc42.x86_64,shim-x64-15.8-3.x86_64" | ||
| let nevra = rpm::Nevra::parse(pkg); | ||
| let _name = nevra.name(); | ||
| // get name as "grub2" to match the usr/lib/efi path | ||
| let (name, _) = _name.split_once('-').unwrap_or((_name, "")); | ||
| ( | ||
| name.to_string(), | ||
| Evr::new( | ||
| nevra.epoch().to_string(), | ||
| nevra.version().to_string(), | ||
| nevra.release().to_string(), | ||
| ), | ||
| ) | ||
| }) | ||
| .collect() | ||
| impl<'a> Ord for Package<'a> { | ||
| fn cmp(&self, other: &Self) -> std::cmp::Ordering { | ||
| self.name | ||
| .cmp(&other.name) // Compare names first | ||
| .then_with(|| self.version.cmp(&other.version)) // If names equal, compare versions | ||
| } | ||
| } | ||
|
|
||
| // Implement Compare package versions function: | ||
| // Return `Greater` if evr_a > evr_b, | ||
| // Return `Less` if evr_a < evr_b, | ||
| // Return `Equal` if evr_a == evr_b. | ||
| fn compare_package_versions_impl(a: &str, b: &str) -> Vec<(String, Ordering)> { | ||
| let map_a = parse_evr_map(a); | ||
| let map_b = parse_evr_map(b); | ||
|
|
||
| // Compare package versions between `map_a` (current) and `map_b` (target). | ||
| // For each package in `map_b`: | ||
| // - If it exists in `map_a`, compare their versions and record the result. | ||
| // - If not found in `map_a`, assume it's a new package that should be upgraded. | ||
| let mut result = Vec::new(); | ||
| for (name, evr_b) in map_b { | ||
| if let Some(evr_a) = map_a.get(&name) { | ||
| let cmp = evr_a.cmp(&evr_b); | ||
| result.push((name, cmp)); | ||
| } else { | ||
| log::trace!("Found new package {name} in the target"); | ||
| result.push((name, Ordering::Less)); | ||
| } | ||
| impl<'a> PartialOrd for Package<'a> { | ||
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
The Package struct can derive Ord and PartialOrd since its fields (String and Evr) already implement these traits. The derived implementations will compare fields in declaration order (name then version), which matches your custom implementation. Deriving these traits makes the code more concise and idiomatic.
| #[derive(Debug, Eq, PartialEq)] | |
| struct Package<'a> { | |
| name: String, | |
| version: Evr<'a>, | |
| } | |
| // assume it is like "grub2-efi-x64-1:2.12-28.fc42.x86_64,shim-x64-15.8-3.x86_64" | |
| let nevra = rpm::Nevra::parse(pkg); | |
| let _name = nevra.name(); | |
| // get name as "grub2" to match the usr/lib/efi path | |
| let (name, _) = _name.split_once('-').unwrap_or((_name, "")); | |
| ( | |
| name.to_string(), | |
| Evr::new( | |
| nevra.epoch().to_string(), | |
| nevra.version().to_string(), | |
| nevra.release().to_string(), | |
| ), | |
| ) | |
| }) | |
| .collect() | |
| impl<'a> Ord for Package<'a> { | |
| fn cmp(&self, other: &Self) -> std::cmp::Ordering { | |
| self.name | |
| .cmp(&other.name) // Compare names first | |
| .then_with(|| self.version.cmp(&other.version)) // If names equal, compare versions | |
| } | |
| } | |
| // Implement Compare package versions function: | |
| // Return `Greater` if evr_a > evr_b, | |
| // Return `Less` if evr_a < evr_b, | |
| // Return `Equal` if evr_a == evr_b. | |
| fn compare_package_versions_impl(a: &str, b: &str) -> Vec<(String, Ordering)> { | |
| let map_a = parse_evr_map(a); | |
| let map_b = parse_evr_map(b); | |
| // Compare package versions between `map_a` (current) and `map_b` (target). | |
| // For each package in `map_b`: | |
| // - If it exists in `map_a`, compare their versions and record the result. | |
| // - If not found in `map_a`, assume it's a new package that should be upgraded. | |
| let mut result = Vec::new(); | |
| for (name, evr_b) in map_b { | |
| if let Some(evr_a) = map_a.get(&name) { | |
| let cmp = evr_a.cmp(&evr_b); | |
| result.push((name, cmp)); | |
| } else { | |
| log::trace!("Found new package {name} in the target"); | |
| result.push((name, Ordering::Less)); | |
| } | |
| impl<'a> PartialOrd for Package<'a> { | |
| fn partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering> { | |
| Some(self.cmp(other)) | |
| } | |
| result | |
| } | |
| #[derive(Debug, Eq, PartialEq, Ord, PartialOrd)] | |
| struct Package<'a> { | |
| name: String, | |
| version: Evr<'a>, | |
| } |
fe95690 to
64f304e
Compare
Package structrpm as build-time configuration
5e05ea1 to
85006c2
Compare
src/packagesystem.rs
Outdated
| let (name_str, rpm_evr) = { | ||
| #[cfg(not(feature = "rpm"))] | ||
| { | ||
| let (name, epoch, version, release, _arch) = parse_values(pkg); |
There was a problem hiding this comment.
Let's use https://docs.rs/uapi-version/latest/uapi_version/ to parse versions when we're not using rpm - the idea here is that other package systems will have formats different than rpm's. (dpkg is similar but definitely not the same)
The UAPI version scheme is attempting to create one that can be used exactly by projects like bootupd (also systemd and other uapi projects). Now in practice one would need to create a "mapping" from other systems to it. The spec mentions some incompatibilities, but I think one could create a reliable mapping.
Actually at least we could ask for Fedora that /usr/lib/efi uses the UAPI version spec and not RPM which would make it much clearer how to use that scheme on other OSes too.
There was a problem hiding this comment.
There's a subtlety here in that we probably can't easily add the name into a single string and re-parse it into a uapi version as that spec doesn't standardize a separator mechanism.
It's quite tricky as we really have two different cases:
- Status quo of a single string that serializes name+version for multiple packages
- /usr/lib/efi which cleanly splits packages from each other and splits the name vs version cleanly
I think we should treat the status quo "single big version string" as the legacy/degenerate case and map other things to it.
Ideally anyone coming now into bootupd from a non-rpm distribution ideally can just implement /usr/lib/efi with uapi versions or so and in that case we don't need to deal with the legacy of the single version string.
There was a problem hiding this comment.
It's quite tricky as we really have two different cases:
- Status quo of a single string that serializes name+version for multiple packages
- /usr/lib/efi which cleanly splits packages from each other and splits the name vs version cleanly
I think we should treat the status quo "single big version string" as the legacy/degenerate case and map other things to it.
Let me start with introducing the parallel versions struct with rpm-evr firstly, then we can ignore to parse the string.
src/packagesystem.rs
Outdated
| return (name.to_string(), evr); | ||
| } | ||
| #[derive(Debug, Eq, PartialEq)] | ||
| struct Package { |
There was a problem hiding this comment.
Elsewhere in the code here we use the term "component" in some cases because not everything comes from a "package". Though we also use "component" to distinguish bios/efi, so it'd likely be confusing if we reused that term here.
I guess if I could go back I'd say "module" or "backend" perhaps for bios/efi and "component" instead of "package".
But anyways it's probably going to be way less confusing to call this Package so let's keep it as is.
There was a problem hiding this comment.
It is package version saved in ContentMetadata struct, looks module is more clean and neat.
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
|
Looks like this one needs a rebase |
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
86b804f to
a4a67bf
Compare
This continuous from coreos#977, inspired by Colin's coreos#977 (comment).
a4a67bf to
05c715f
Compare
This continuous from #977,
inspired by Colin's #977 (comment).