Skip to content

Fix rpm version comparison #70231

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.

Sign up for GitHub

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

Open
vzhestkov wants to merge 3 commits into
base: 3006.x
Choose a base branch
from
+44 −34
Open

Fix rpm version comparison #70231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc940f8
Avoid ignoring the release as it is harming the behaviour
vzhestkov Sep 2, 2026
7e5d131
Fix the rpm version comparison test
vzhestkov Sep 2, 2026
4e7d654
Add few more test cases
vzhestkov Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions salt/modules/rpm_lowpkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,12 +704,6 @@ def normalize(x):

(ver1_e, ver1_v, ver1_r) = salt.utils.pkg.rpm.version_to_evr(ver1)
(ver2_e, ver2_v, ver2_r) = salt.utils.pkg.rpm.version_to_evr(ver2)
# If one EVR is missing a release but not the other and they
# otherwise would be equal, ignore the release. This can happen if
# e.g. you are checking if a package version 3.2 is satisfied by
# 3.2-1.
if not ver1_r or not ver2_r:
ver1_r = ver2_r = ""

if HAS_RPM:
try:
Expand Down
11 changes: 9 additions & 2 deletions salt/utils/pkg/rpm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,14 @@ def evr_compare(
cmp_versions = _rpmvercmp(version1 or "", version2 or "")
if cmp_versions != 0:
return cmp_versions
return _rpmvercmp(release1 or "", release2 or "")
if release1 is None or release2 is None:
if release1 is not None:
return 1
elif release2 is not None:
return -1
else:
return 0
return _rpmvercmp(release1, release2)


def _rpmvercmp(a: str, b: str) -> int:
Expand Down Expand Up @@ -343,6 +350,6 @@ def version_to_evr(verstring):
release = verstring[idx_r + 1 :]
else:
version = verstring[idx_e + 1 :]
release = ""
release = None

return epoch, version, release
61 changes: 35 additions & 26 deletions tests/pytests/unit/modules/test_rpm_lowpkg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,8 +262,8 @@ def test_version_cmp_rpm_lib():
assert rpm_lowpkg.version_cmp("3:2.9.1-6.el7.4", "3:2.9.1-6.el7.4") == 0
assert rpm_lowpkg.version_cmp("3:2.9.1-6.el7.4", "3:2.9.1-7.el7.4") == -1
assert rpm_lowpkg.version_cmp("3:2.9.1-8.el7.4", "3:2.9.1-7.el7.4") == 1
assert rpm_lowpkg.version_cmp("3.23-6.el9", "3.23") == 0
assert rpm_lowpkg.version_cmp("3.23", "3.23-6.el9") == 0
assert rpm_lowpkg.version_cmp("3.23-6.el9", "3.23") == 1
assert rpm_lowpkg.version_cmp("3.23", "3.23-6.el9") == -1
assert (
rpm_lowpkg.version_cmp("release_web_294-6", "release_web_294_applepay-1")
== -1
Expand Down Expand Up @@ -509,9 +509,10 @@ def _parse_label(label: str):
("2.0-1.0.1", "2.0-1.0", 1),
("2.0-1.alpha.1", "2.0-1.alpha.2", -1),
("2.0-1.alpha.01", "2.0-1.alpha.1", 0),
# Empty release equivalence and ordering
("2.0", "2.0-", 0),
("2.0-", "2.0-1", 0),
# No release vs empty and non-empty
("2.0-", "2.0", 1),
("2.0", "2.0-", -1),
Comment on lines +513 to +514

@bdrx312 bdrx312 Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to rpmdev-vercmp, these should be 0 not 1 and -1

$ rpmdev-vercmp '2.0-' '2.0'
2.0 == 2.0

@vzhestkov vzhestkov Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's wrong just to call it this way. I mean rpmdev-vercmp shold compare separately either epoch or version or release. In case of using 2.0- in reality it should be treated as the release with empty string value, while 2.0 is just a version with no release at all. In this particular call actually 2.0- as version value is compared to 2.0 as a version value only, assuming there is no release in both cases. rpmdev-vercmp must be called separatelly for epoch and if it's equal or omitted, versions should be compared and if identical, releases. But for 2.0- in this context it's version where - is just dropped by rpm logic as it shouldn't be there.

> zypper vcmp '2.0-' '2.0'
2.0- is newer than 2.0

zypper here is intended to compare full [epoch]:version-release strings.

For some reason different version comparison libs are using slightly different logic causing some mess. In this exable above zypper is using libsolv internally which is using exact copy of logic from rpm itself.

@bdrx312 bdrx312 Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From all research that I have done, an rpm without a release is not a valid case. An rpm that does not have a release field is invalid and I believe the tool should be rejecting it.

https://rpm.org/docs/latest/man/rpm-version.7#Comparing

Only the VERSION component is mandatory in an EVR label. All RPM packages have a VERSION and a RELEASE, however.

The release is not required to be in the EVR but the RPM information is still required to have the release field.

What is the actual use case where you are even running into this problem? It seems these RPMs may be invalid and whatever tool is building them needs to be fixed?

The lua rpm.vercmp does seem to agree with your comparison though:

 $ rpm --eval '%{lua:print(rpm.vercmp("2.0-","2.0"))}'
1

("2.0-", "2.0-1", -1),
# Different number of segments
("1.0", "1.0.1", -1),
("1.0.1", "1.0", 1),
Expand Down Expand Up @@ -548,43 +549,43 @@ def _parse_label(label: str):
("1.0-1.alpha-beta-1", "1.0-1.alpha-beta-2", -1),
("1.0-1.alpha-beta-2", "1.0-1.alpha-beta-1", 1),
# Explicit empty vs multiple-hyphen release forms
("1.0-", "1.0-1-alpha", 0),
("1.0-1-alpha", "1.0-", 0),
("1.0-", "1.0-1-alpha", -1),
("1.0-1-alpha", "1.0-", 1),
# Longer release wins when numeric/alpha tie in earlier segments
("3.0-1.0.0-0", "3.0-1.0.0", 1),
("3.0-1.0.0", "3.0-1.0.0-0", -1),
# Same version, one side has no release -> treat as equal
("1.0-1", "1.0", 0),
("1.0", "1.0-1", 0),
("2.3.4-5", "2.3.4", 0),
("2.3.4", "2.3.4-5", 0),
# Different versions, release ignored if one side missing -> version decides
# Same version, one side has no release, couldn't be treated as equal
("1.0-1", "1.0", 1),
("1.0", "1.0-1", -1),
("2.3.4-5", "2.3.4", 1),
("2.3.4", "2.3.4-5", -1),
# Different versions -> version decides
("1.0-2", "1.1", -1),
("1.1", "1.0-2", 1),
("2.0-3", "2.0.1", -1),
("2.0.1", "2.0-3", 1),
# Epoch differences still take precedence even when release missing
# Epoch differences still take precedence
("1:1.0-1", "1.0", 1),
("1.0", "1:1.0-1", -1),
("0:2.0-1", "1:1.9", -1),
("1:1.9", "0:2.0-1", 1),
# Both sides have version segments that compare alphabetically; release ignored when missing
("1.0a-1", "1.0a", 0),
("1.0a", "1.0a-2", 0),
("1.0~rc1-1", "1.0~rc1", 0),
("1.0~rc1", "1.0~rc1-1", 0),
# Operator blocks: caret/tilde interactions, release ignored when missing
("1.0^git1-1", "1.0^git1", 0),
("1.0^git1", "1.0^git1-2", 0),
# Both sides have version segments that compare alphabetically
("1.0a-1", "1.0a", 1),
("1.0a", "1.0a-2", -1),
("1.0~rc1-1", "1.0~rc1", 1),
("1.0~rc1", "1.0~rc1-1", -1),
# Operator blocks: caret/tilde interactions
("1.0^git1-1", "1.0^git1", 1),
("1.0^git1", "1.0^git1-2", -1),
(
"1.0~beta-1",
"1.0",
-1,
), # tilde makes version older than base even if release present on left
("1.0", "1.0~beta-1", 1),
# One side has complex release, other has no release; version decides when different
("2.0.1-10.alpha", "2.0.1", 0),
("2.0.1", "2.0.1-10.alpha", 0),
# One side has complex release, other has no release
("2.0.1-10.alpha", "2.0.1", 1),
("2.0.1", "2.0.1-10.alpha", -1),
("2.0.2-1", "2.0.10", -1),
("2.0.10", "2.0.2-1", 1),
# Special chars in wild
Expand Down Expand Up @@ -616,6 +617,11 @@ def _parse_label(label: str):
("1.0_^git1", "1.0^git1", 0),
("1.0+~rc1", "1.0~rc1", 0),
("1.0_~rc1", "1.0~rc1", 0),
# Some extra pairs to check
("3.20.2-13569.1", "3.20-13457.1", 1),
("3.20-13457.1", "3.20.2-13569.1", -1),
("4.99.1-1", "4.99-2", 1),
("4.99-2", "4.99.1-1", -1),
]


Expand All @@ -634,7 +640,10 @@ def test_version_cmp_expected(label1, label2, expected):
def test_version_cmp_matches_rpm(label1, label2, _):
evr1 = _parse_label(label1)
evr2 = _parse_label(label2)
py_result = rpm_lowpkg.version_cmp(label1, label2)
with patch("salt.modules.rpm_lowpkg.HAS_RPM", False):
# Enforce using version comparison from salt.utils.pkg.rpm
# to prevent using rpm implementation we are using to check here.
py_result = rpm_lowpkg.version_cmp(label1, label2)
rpm_result = rpm.labelCompare(evr1, evr2)
assert py_result == rpm_result, (
f"Mismatch for {label1} vs {label2}: "
Expand Down
Loading