Add versions to ContentMetadata#986
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 introduces a versions field to ContentMetadata to enable more robust package version comparisons using a structured Module type. This is a valuable improvement over string-based comparison. The implementation is mostly solid, with new logic for parsing and comparing versions, and updated tests.
My review focuses on a couple of correctness issues and a performance improvement. I've identified two places where the newly created Vec<Module> is not sorted before being stored, which violates an assumption made by the comparison function and could lead to incorrect behavior. I've also suggested a more efficient and idiomatic way to handle version comparison logic in ContentMetadata::can_upgrade_to to avoid unnecessary data cloning.
1cca830 to
49a9b40
Compare
| }); | ||
|
|
||
| // Map the version into Module struct | ||
| let mut modules_vec: Vec<Module> = pkgs.keys().map(|pkg_str| parse_evr(pkg_str)).collect(); |
There was a problem hiding this comment.
Not sure if should map the version str into Module struct when using rpm -q.
There was a problem hiding this comment.
Hum, I don't understand the question here. Do you mean that if we do rpm -q we should rely on timestamps and not versions?
There was a problem hiding this comment.
I meant not sure if it is necessary to do the map, and it will compare using version if we do not have the Module struct. But agree to do the map.
Copy the change here:
if let (Some(self_versions), Some(target_versions)) = (&self.versions, &target.versions) {
compare_package_slices(self_versions, target_versions)
} else {
compare_package_versions(&self.version, &target.version)
}
49a9b40 to
7a56c7b
Compare
7a56c7b to
96c9373
Compare
|
Rebase the main branch to include CI workflow fix, and will fix the CI failed. |
2189aaa to
fde3fd4
Compare
bd591d2 to
fc222a0
Compare
Also add `uapi_version::Version` to parse versions according to Colin's comment coreos#983 (comment)
Inspired by Colin's comment coreos#977 (comment) We need `versions` to save struct like: ``` "versions": [ {"name": "pkg1", "rpm-evr": "1.0"}, {"name": "pkg2", "rpm-evr": "2.0"} ] ``` Then we read and compare versions, do not need to parse the version in str format. Co-worked by walters@verbum.org
fc222a0 to
040e314
Compare
|
Rebase to main branch and update test, and will merge this as the CI is passed. |
travier
left a comment
travier
left a comment
There was a problem hiding this comment.
LGTM. I'm afraid that if we get a source package name that includes a "-" that we will not get the name properly recognized but we can probably worry about that if that happens. Neither grub2, shim, not systemd-boot (systemd source package name) are in this case.
4 participants
packagesystem: add struct
Moduleabout package name and evrAlso add
uapi_version::Versionto parse versions according toColin's comment #983 (comment)
Add
versionstoContentMetadataInspired by Colin's comment #977 (comment)
We need
versionsto save struct like:Then we read and compare versions, do not need to parse the
version in str format.
Co-worked by walters@verbum.org