package: move to compare `version` instead of `timestamp` by HuijingHei · Pull Request #977 · coreos/bootupd

HuijingHei · 2025-08-04T07:04:06Z

Use rpm::Evr crate to parse and compare version when checking for update.

Inspired by Timothee's pointer:
#938 (comment)
Fixes: #933

Additional info:
Use rpm 0.16 instead of 0.17 as 1.84 does not support
feature edition2024 that is requried by 0.17:

error: failed to parse manifest at `/root/.cargo/registry/src/
index.crates.io-6f17d22bba15001f/rpm-0.17.0/Cargo.toml`

Caused by:
  feature `edition2024` is required

  The package requires the Cargo feature called `edition2024`,
but that feature is not stabilized in this version of Cargo
(1.84.1 (66221abde 2024-11-19)).

gemini-code-assist

Code Review

The code changes introduce the use of the rpm::Evr crate to parse and compare package versions, replacing the previous timestamp-based approach. A critical issue was identified in the package version string parsing logic that could lead to incorrect behavior with certain package name formats. A suggestion to improve code clarity was also provided.

src/packagesystem.rs

HuijingHei · 2025-08-04T09:58:01Z

Ready for review now, thank you!

HuijingHei · 2025-08-05T04:59:15Z

Fix CI error and add related unit test if there is new package.

mike-nguyen · 2025-08-06T18:54:18Z

Is there a reason why the rpm_evr_compare function from the rpm crate can't be used (https://docs.rs/rpm/latest/rpm/fn.rpm_evr_compare.html)?

HuijingHei · 2025-08-07T06:22:18Z

Is there a reason why the rpm_evr_compare function from the rpm crate can't be used (https://docs.rs/rpm/latest/rpm/fn.rpm_evr_compare.html)?

I think the problem is we need to ignore epoch in our comparison, e.g. grub2-efi-x64-1:2.12-28.fc42.x86_64 should be the same as grub2-2.12-28.fc42, but actually no (as the first one has epoch 1, so it is greater than second without epoch)

HuijingHei · 2025-08-07T06:43:01Z

Also tried with rpm::Nevra::parse(pkg) (see https://docs.rs/rpm/latest/rpm/struct.Nevra.html#method.parse), the result is not expected, I think this is because the package name has -.

"grub2-efi-x64-1:2.12-28.fc42.x86_64" -- epoch should be "1"

[src/main.rs:9:13] &nevra = Nevra {
    name: "grub2",
    evr: Evr {
        epoch: "efi-x64-1",
        version: "2.12",
        release: "28.fc42",
    },
    arch: "x86_64",
}

"shim-x64-15.8-3.x86_64" -- version should be 15.8, release should be 3

[src/main.rs:9:13] &nevra = Nevra {
    name: "shim",
    evr: Evr {
        epoch: "",
        version: "x64",
        release: "15.8-3",
    },
    arch: "x86_64",
}

"grub2-2.12-28.fc42" -- arch should be ""

[src/main.rs:9:13] &nevra = Nevra {
    name: "grub2",
    evr: Evr {
        epoch: "",
        version: "2.12",
        release: "28",
    },
    arch: "fc42",
}

"shim-15.8-3" -- the result is correct and that we want

[src/main.rs:9:13] &nevra = Nevra {
    name: "shim",
    evr: Evr {
        epoch: "",
        version: "15.8",
        release: "3",
    },
    arch: "",
}

HuijingHei · 2025-08-07T09:51:45Z

Update the logic to parse str using rpm::Evr::parse() if it is like grub2-2.12-28.fc42,shim-15.8-3, but still need to parse the str if like grub2-efi-x64-1:2.12-28.fc42.x86_64,shim-x64-15.8-3.x86_64, as the name includes -.

travier · 2025-08-08T15:32:07Z

Also tried with rpm::Nevra::parse(pkg) (see docs.rs/rpm/latest/rpm/struct.Nevra.html#method.parse), the result is not expected, I think this is because the package name has -.

I'm wondering if this is an issue with how the RPM versions is defined or a bug in the library as epoch/versions should ever start with a number, but I'm not sure.

That's likely comes from the parsing in the library: https://docs.rs/rpm/latest/src/rpm/version.rs.html#96

I don't see "must start with a number" constraint in https://rpm.org/docs/4.20.x/manual/spec.html#preamble

There apparently isn't: semver/semver#145 (comment)

But we can probably push in that direction.

Would be good to file an issue in https://github.com/rpm-rs/rpm/issues?q=sort%3Aupdated-desc+is%3Aissue+is%3Aopen

Conclusion: Manually stripping the name first and then using the evra parser should work best, which looks like what you did.

travier · 2025-08-08T15:49:44Z

Is there a reason why the rpm_evr_compare function from the rpm crate can't be used (docs.rs/rpm/latest/rpm/fn.rpm_evr_compare.html)?

I think the problem is we need to ignore epoch in our comparison, e.g. grub2-efi-x64-1:2.12-28.fc42.x86_64 should be the same as grub2-2.12-28.fc42, but actually no (as the first one has epoch 1, so it is greater than second without epoch)

Ideally we should not ignore the epoch.

It looks like the issue is that the epoch is not used in the path where the grub binaries are stored: https://src.fedoraproject.org/rpms/grub2/pull-request/170#_1__7

%global grub_version_dir %{grub_root_dir}/%{version}-%{release}

travier · 2025-08-08T15:52:03Z

@lsandov1 we should probably change the installation path to include the epoch as well as the version and release.

HuijingHei · 2025-08-10T14:01:14Z

Would be good to file an issue in https://github.com/rpm-rs/rpm/issues?q=sort%3Aupdated-desc+is%3Aissue+is%3Aopen

SGTM, create issue rpm-rs/rpm#275

HuijingHei · 2025-08-10T14:09:54Z

@lsandov1 we should probably change the installation path to include the epoch as well as the version and release.

WDYT about %global grub_version_dir %{grub_root_dir}/%{evr} (see https://src.fedoraproject.org/fork/lsandova/rpms/grub2/blob/rawhide/f/grub.macros#_241)?

Use `rpm::Evr` crate to parse and compare `version` when checking for update. Inspired by Timothee's pointer: coreos#938 (comment) Fixes: coreos#933 Additional info: Use rpm `0.16.1` instead of `0.17` as 1.84 does not support feature `edition2024` that is requried by 0.17: ``` error: failed to parse manifest at `/root/.cargo/registry/src/ index.crates.io-6f17d22bba15001f/rpm-0.17.0/Cargo.toml` Caused by: feature `edition2024` is required The package requires the Cargo feature called `edition2024`, but that feature is not stabilized in this version of Cargo (1.84.1 (66221abde 2024-11-19)). ```

cgwalters

I'm OK with this as is generally.

cgwalters · 2025-08-13T08:10:12Z

src/packagesystem.rs

-    );
+fn parse_evr_map(input: &str) -> BTreeMap<String, Evr> {
+    input
+        .split(',')


It does seem like we can reliably split on this since AFAICS , is illegal in all of RPM, the uapi versions, dpkg, arch etc.

That said if we are moving to make the versions actually parsed, it would probably make sense to also switch to a proper array instead of a single string.

For compatibility though we'd need to do both for a while at least, i.e. have both version (string) and versions (array) in the model/JSON.

For compatibility though we'd need to do both for a while at least, i.e. have both version (string) and versions (array) in the model/JSON.

What do you think about like this? Then we do not need to parse the str.

{ "version": "pkg1-1.0,pkg2-2.0", // old single string "versions": [ // new proper array {"name": "pkg1", "evr": "1.0"}, {"name": "pkg2", "evr": "2.0"} ] }

How about replacing evr with rpm-evr to make explicit what it is? On other operating systems the version might be different.

BTW recently in bootc-dev/bootc@17c03f7 I added the uapi_versions crate to bootc. It would make sense to me to use it here too optionally for systems not using rpm.

But anyways let's start with introducing the parallel versions struct with rpm-evr or so.

cgwalters · 2025-08-13T08:12:26Z

src/packagesystem.rs

+            }
+
+            // 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);


Per my earlier comment I think this is OK as is but I'd like to not add new hardcoded RPM dependencies. We can just make this a build-time configuration option in the future.

Do you mean that we only import rpm if using cargo build --features rpm, else using cargo build without rpm, but we also use cmp() to compare rpm::Evr (see https://docs.rs/rpm/latest/rpm/struct.Evr.html#method.cmp), so we still need rpm, is this right?

I think it's OK to have an rpm feature and use it.

But we could have a generic version that uses https://docs.rs/uapi-version/latest/uapi_version/ to compare versions and require that package systems map to a compatible version scheme (if it's not compatible).

cgwalters · 2025-08-13T08:14:19Z

src/packagesystem.rs

+    for (_, ord) in &diffs {
+        match ord {
+            Ordering::Less => {
+                has_less = true;


This can just be return Ordering::less right? No need for the bool.

Nice catch, update in #983

cgwalters · 2025-08-13T08:17:34Z

src/packagesystem.rs

+// If any package is Ordering::Less, return Ordering::Less, means upgradable,
+// Else if any package is Ordering::Greater, return Ordering::Greater,
+// Else (all equal), return Ordering::Equal.
+pub(crate) fn compare_package_versions(a: &str, b: &str) -> Ordering {


I think this is OK but it'd be more idiomatic to create a struct Package that has impl Ord.

And also I think per above we should split on the , outside this function, i.e. we're comparing two slices [Package] - and if everything compares equal then the longer one wins.

SGTM, thank you for the pointer, update in #983

Inspired by Colin's coreos#977 (comment) Copy the comment: ``` Create a struct Package that has impl Ord. Compare two slices `[Package]` - and if everything compares equal then the longer one wins. ```

This continuous from coreos#977, inspired by Colin's coreos#977 (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

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei mentioned this pull request Aug 4, 2025

step1: Extend generate-update-metadata() to read from /usr #938

Merged

gemini-code-assist bot reviewed Aug 4, 2025

View reviewed changes

src/packagesystem.rs Outdated Show resolved Hide resolved

src/packagesystem.rs Outdated Show resolved Hide resolved

HuijingHei added the do-not-merge/work-in-progress label Aug 4, 2025

HuijingHei force-pushed the fix-version-compare branch 5 times, most recently from bc9afdc to 8d3aec1 Compare August 4, 2025 08:06

HuijingHei removed the do-not-merge/work-in-progress label Aug 4, 2025

HuijingHei force-pushed the fix-version-compare branch from 8d3aec1 to da4de80 Compare August 4, 2025 09:57

HuijingHei requested a review from cgwalters August 4, 2025 09:58

HuijingHei force-pushed the fix-version-compare branch 2 times, most recently from bdbd4b3 to a7bdb10 Compare August 5, 2025 02:57

HuijingHei changed the title ~~efi: move to compare version instead of timestamp~~ package: move to compare version instead of timestamp Aug 5, 2025

HuijingHei force-pushed the fix-version-compare branch from a7bdb10 to 2690df4 Compare August 5, 2025 13:35

HuijingHei requested a review from travier August 6, 2025 03:12

HuijingHei force-pushed the fix-version-compare branch 3 times, most recently from 8f99338 to 4a9513a Compare August 7, 2025 09:39

HuijingHei force-pushed the fix-version-compare branch from 4a9513a to 6c79a89 Compare August 13, 2025 02:26

HuijingHei force-pushed the fix-version-compare branch from 6c79a89 to fdb91fe Compare August 13, 2025 02:32

cgwalters approved these changes Aug 13, 2025

View reviewed changes

cgwalters merged commit f0fcca8 into coreos:main Aug 13, 2025
12 checks passed

HuijingHei mentioned this pull request Aug 15, 2025

packagesystem: make rpm as build-time configuration #983

Merged

HuijingHei deleted the fix-version-compare branch August 15, 2025 11:01

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Aug 18, 2025

packagesystem: make rpm as build-time configuration

64f304e

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Aug 18, 2025

packagesystem: make rpm as build-time configuration

bbf851d

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Aug 18, 2025

packagesystem: make rpm as build-time configuration

5e05ea1

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Aug 18, 2025

packagesystem: make rpm as build-time configuration

85006c2

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei mentioned this pull request Aug 20, 2025

Add versions to ContentMetadata #986

Merged

cgwalters mentioned this pull request Sep 2, 2025

extend-payload-to-esp: extends any src_input_dir to esp #935

Open

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Sep 8, 2025

packagesystem: make rpm as build-time configuration

75ba529

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

HuijingHei added a commit to HuijingHei/bootupd that referenced this pull request Sep 12, 2025

packagesystem: make rpm as build-time configuration

05c715f

This continuous from coreos#977, inspired by Colin's coreos#977 (comment).

Conversation

HuijingHei commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

HuijingHei commented Aug 4, 2025

Uh oh!

HuijingHei commented Aug 5, 2025

Uh oh!

mike-nguyen commented Aug 6, 2025

Uh oh!

HuijingHei commented Aug 7, 2025

Uh oh!

HuijingHei commented Aug 7, 2025

Uh oh!

HuijingHei commented Aug 7, 2025

Uh oh!

travier commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

travier commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

travier commented Aug 8, 2025

Uh oh!

HuijingHei commented Aug 10, 2025

Uh oh!

HuijingHei commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuijingHei Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HuijingHei Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

HuijingHei commented Aug 4, 2025 •

edited

Loading

travier commented Aug 8, 2025 •

edited

Loading

travier commented Aug 8, 2025 •

edited

Loading

HuijingHei commented Aug 10, 2025 •

edited

Loading

HuijingHei Aug 15, 2025 •

edited

Loading

HuijingHei Aug 15, 2025 •

edited

Loading