diff --git a/.pylintrc b/.pylintrc index df9631f76db2..9b2b43a4405d 100644 --- a/.pylintrc +++ b/.pylintrc @@ -92,7 +92,7 @@ persistent=no # Minimum Python version to use for version dependent checks. Will default to # the version used to run pylint. -py-version=3.9 +py-version=3.10 # Discover python modules and packages in the file system subtree. recursive=no diff --git a/changelog/68341.fixed.md b/changelog/68341.fixed.md new file mode 100644 index 000000000000..cff4b7b7087b --- /dev/null +++ b/changelog/68341.fixed.md @@ -0,0 +1,2 @@ +Fixed pkg.installed state from showing warning if python rpm package not installed. +Fixed pkg.installed state from showing warning and using slow process fork for version comparison when rpmdevtools is installed diff --git a/salt/modules/rpm_lowpkg.py b/salt/modules/rpm_lowpkg.py index feb9deaccfa8..cfd0c650bfd0 100644 --- a/salt/modules/rpm_lowpkg.py +++ b/salt/modules/rpm_lowpkg.py @@ -11,7 +11,6 @@ import salt.utils.itertools import salt.utils.path import salt.utils.pkg.rpm -import salt.utils.versions from salt.exceptions import CommandExecutionError, SaltInvocationError from salt.utils.versions import LooseVersion @@ -698,14 +697,23 @@ def version_cmp(ver1, ver2, ignore_epoch=False): """ def normalize(x): - return str(x).split(":", 1)[-1] if ignore_epoch else str(x) + return str(x).split(":", maxsplit=1)[-1] if ignore_epoch else str(x) ver1 = normalize(ver1) ver2 = normalize(ver2) - try: - cmp_func = None - if HAS_RPM: + (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: + cmp_func = None try: cmp_func = rpm.labelCompare except AttributeError: @@ -716,91 +724,27 @@ def normalize(x): "labelCompare function. Not using rpm.labelCompare for " "version comparison." ) - else: - log.warning( - "Please install a package that provides rpm.labelCompare for " - "more accurate version comparisons." - ) - - # 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. - (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 not ver1_r or not ver2_r: - ver1_r = ver2_r = "" - - if cmp_func is None: - ver1 = f"{ver1_e}:{ver1_v}-{ver1_r}" - ver2 = f"{ver2_e}:{ver2_v}-{ver2_r}" - - if salt.utils.path.which("rpmdev-vercmp"): - log.warning( - "Installing the rpmdevtools package may surface dev tools in" - " production." - ) - - # rpmdev-vercmp always uses epochs, even when zero - def _ensure_epoch(ver): - def _prepend(ver): - return f"0:{ver}" - - try: - if ":" not in ver: - return _prepend(ver) - except TypeError: - return _prepend(ver) - return ver - - ver1 = _ensure_epoch(ver1) - ver2 = _ensure_epoch(ver2) - result = __salt__["cmd.run_all"]( - ["rpmdev-vercmp", ver1, ver2], - python_shell=False, - redirect_stderr=True, - ignore_retcode=True, + if cmp_func is not None: + cmp_result = cmp_func( + (ver1_e, ver1_v, ver1_r), (ver2_e, ver2_v, ver2_r) ) - # rpmdev-vercmp returns 0 on equal, 11 on greater-than, and - # 12 on less-than. - if result["retcode"] == 0: - return 0 - elif result["retcode"] == 11: - return 1 - elif result["retcode"] == 12: - return -1 - else: - # We'll need to fall back to salt.utils.versions.version_cmp() - log.warning( - "Failed to interpret results of rpmdev-vercmp output. " - "This is probably a bug, and should be reported. " - "Return code was %s. Output: %s", - result["retcode"], - result["stdout"], + if cmp_result not in (-1, 0, 1): + raise CommandExecutionError( + f"Comparison result '{cmp_result}' is invalid" ) - else: - log.warning( - "Falling back on salt.utils.versions.version_cmp() for version" - " comparisons" - ) - else: - cmp_result = cmp_func((ver1_e, ver1_v, ver1_r), (ver2_e, ver2_v, ver2_r)) - if cmp_result not in (-1, 0, 1): - raise CommandExecutionError( - f"Comparison result '{cmp_result}' is invalid" - ) - return cmp_result - - except Exception as exc: # pylint: disable=broad-except - log.warning( - "Failed to compare version '%s' to '%s' using RPM: %s", ver1, ver2, exc + return cmp_result + except Exception as exc: # pylint: disable=broad-except + log.warning( + "Failed to compare version '%s' to '%s' using RPM: %s", ver1, ver2, exc + ) + else: + log.info( + "Install a package that provides rpm.labelCompare for " + "faster version comparisons." ) - - # We would already have normalized the versions at the beginning of this - # function if ignore_epoch=True, so avoid unnecessary work and just pass - # False for this value. - return salt.utils.versions.version_cmp(ver1, ver2, ignore_epoch=False) + return salt.utils.pkg.rpm.evr_compare( + (ver1_e, ver1_v, ver1_r), (ver2_e, ver2_v, ver2_r) + ) def checksum(*paths, **kwargs): diff --git a/salt/utils/pkg/rpm.py b/salt/utils/pkg/rpm.py index 7574a068e83c..59acb920b1aa 100644 --- a/salt/utils/pkg/rpm.py +++ b/salt/utils/pkg/rpm.py @@ -157,6 +157,157 @@ def combine_comments(comments): return "".join(ret) +def evr_compare( + # evr1: tuple[str | None, str | None, str | None], + evr1, + # evr2: tuple[str | None, str | None, str | None], + evr2, +) -> int: + """ + Compare two RPM package identifiers using full epoch–version–release semantics. + + This is a pure‑Python equivalent of ``rpm.labelCompare()``, returning the same + ordering as the system RPM library without requiring the ``python3-rpm`` bindings. + + The comparison is performed in three stages: + + 1. **Epoch** — compared numerically; missing or empty values are treated as 0. + 2. **Version** — compared using RPM's ``rpmvercmp`` rules: + - Split into digit, alpha, and tilde (``~``) segments. + - Tilde sorts before all other characters (e.g. ``1.0~beta`` < ``1.0``). + - Numeric segments are compared as integers, ignoring leading zeros. + - Numeric segments sort before alpha segments. + 3. **Release** — compared with the same rules as version. + + :param evr1: The first ``(epoch, version, release)`` triple to compare. + Each element may be a string or ``None``. + :param evr2: The second ``(epoch, version, release)`` triple to compare. + Each element may be a string or ``None``. + :return: ``-1`` if ``evr1`` is considered older than ``evr2``, + ``0`` if they are considered equal, + ``1`` if ``evr1`` is considered newer than ``evr2``. + + .. note:: + This comparison is **not** the same as PEP 440, ``LooseVersion``, or + ``StrictVersion``. It is intended for RPM package metadata and will match + the ordering used by tools like ``rpm``, ``dnf``, and ``yum``. + + .. code-block:: python + + >>> label_compare(("0", "1.2.3", "1"), ("0", "1.2.3", "2")) + -1 + >>> label_compare(("1", "1.0", "1"), ("0", "9.9", "9")) + 1 + >>> label_compare(("0", "1.0~beta", "1"), ("0", "1.0", "1")) + -1 + """ + epoch1, version1, release1 = evr1 + epoch2, version2, release2 = evr2 + epoch1 = int(epoch1 or 0) + epoch2 = int(epoch2 or 0) + if epoch1 != epoch2: + return 1 if epoch1 > epoch2 else -1 + cmp_versions = _rpmvercmp(version1 or "", version2 or "") + if cmp_versions != 0: + return cmp_versions + return _rpmvercmp(release1 or "", release2 or "") + + +def _rpmvercmp(a: str, b: str) -> int: + """ + Pure-Python comparator matching RPM's rpmvercmp(). + Handles separators, tilde (~), caret (^), numeric/alpha segments. + """ + # Fast path: identical strings + if a == b: + return 0 + + i = j = 0 + la, lb = len(a), len(b) + + def isalnum_(c: str) -> bool: + return c.isalnum() + + while i < la or j < lb: + # Skip separators: anything not alnum, not ~, not ^ + while i < la and not (isalnum_(a[i]) or a[i] in "~^"): + i += 1 + while j < lb and not (isalnum_(b[j]) or b[j] in "~^"): + j += 1 + + # Tilde: sorts before everything else + if i < la and a[i] == "~" or j < lb and b[j] == "~": + if not (i < la and a[i] == "~"): + return 1 + if not (j < lb and b[j] == "~"): + return -1 + i += 1 + j += 1 + continue + + # Caret: like tilde except base (end) loses to caret + if i < la and a[i] == "^" or j < lb and b[j] == "^": + if i >= la: + return -1 + if j >= lb: + return 1 + if not (i < la and a[i] == "^"): + return 1 + if not (j < lb and b[j] == "^"): + return -1 + i += 1 + j += 1 + continue + + # If either ran out now, stop + if not (i < la and j < lb): + break + + # Segment start positions + si, sj = i, j + + # Decide type from left side + isnum = a[i].isdigit() + if isnum: + while i < la and a[i].isdigit(): + i += 1 + while j < lb and b[j].isdigit(): + j += 1 + else: + while i < la and a[i].isalpha(): + i += 1 + while j < lb and b[j].isalpha(): + j += 1 + + # If right side had no same‑type run, types differ + if sj == j: + return 1 if isnum else -1 + + seg_a = a[si:i] + seg_b = b[sj:j] + + if isnum: + # Strip leading zeros + seg_a_nz = seg_a.lstrip("0") + seg_b_nz = seg_b.lstrip("0") + # Compare by length + if len(seg_a_nz) != len(seg_b_nz): + return 1 if len(seg_a_nz) > len(seg_b_nz) else -1 + # Same length: lexicographic + if seg_a_nz != seg_b_nz: + return 1 if seg_a_nz > seg_b_nz else -1 + else: + # Alpha vs alpha + if seg_a != seg_b: + return 1 if seg_a > seg_b else -1 + # else equal segment → loop continues + + # Tail handling + if i >= la and j >= lb: + return 0 + return -1 if i >= la else 1 + + def version_to_evr(verstring): """ Split the package version string into epoch, version and release. diff --git a/tests/pytests/unit/modules/test_rpm_lowpkg.py b/tests/pytests/unit/modules/test_rpm_lowpkg.py index ff68f92e5d98..a5d2708b8d50 100644 --- a/tests/pytests/unit/modules/test_rpm_lowpkg.py +++ b/tests/pytests/unit/modules/test_rpm_lowpkg.py @@ -2,12 +2,14 @@ :codeauthor: Jayesh Kariya """ +import random +import string import textwrap import pytest import salt.modules.cmdmod -import salt.modules.rpm_lowpkg as rpm +import salt.modules.rpm_lowpkg as rpm_lowpkg import salt.utils.path from tests.support.mock import MagicMock, patch @@ -17,7 +19,6 @@ HAS_RPM = True except ImportError: HAS_RPM = False -# pylint: enable=unused-import def _called_with_root(mock): @@ -27,36 +28,36 @@ def _called_with_root(mock): @pytest.fixture def configure_loader_modules(): - return {rpm: {"rpm": MagicMock(return_value=MagicMock)}} + return {rpm_lowpkg: {"rpm": MagicMock(return_value=MagicMock)}} def test___virtual___openeuler(): patch_which = patch("salt.utils.path.which", return_value=True) with patch.dict( - rpm.__grains__, {"os": "openEuler", "os_family": "openEuler"} + rpm_lowpkg.__grains__, {"os": "openEuler", "os_family": "openEuler"} ), patch_which: - assert rpm.__virtual__() == "lowpkg" + assert rpm_lowpkg.__virtual__() == "lowpkg" def test___virtual___issabel_pbx(): patch_which = patch("salt.utils.path.which", return_value=True) with patch.dict( - rpm.__grains__, {"os": "Issabel Pbx", "os_family": "IssabeL PBX"} + rpm_lowpkg.__grains__, {"os": "Issabel Pbx", "os_family": "IssabeL PBX"} ), patch_which: - assert rpm.__virtual__() == "lowpkg" + assert rpm_lowpkg.__virtual__() == "lowpkg" def test___virtual___virtuozzo(): patch_which = patch("salt.utils.path.which", return_value=True) with patch.dict( - rpm.__grains__, {"os": "virtuozzo", "os_family": "VirtuoZZO"} + rpm_lowpkg.__grains__, {"os": "virtuozzo", "os_family": "VirtuoZZO"} ), patch_which: - assert rpm.__virtual__() == "lowpkg" + assert rpm_lowpkg.__virtual__() == "lowpkg" def test___virtual___with_no_rpm(): patch_which = patch("salt.utils.path.which", return_value=False) - ret = rpm.__virtual__() + ret = rpm_lowpkg.__virtual__() assert isinstance(ret, tuple) assert ret[0] is False @@ -69,8 +70,8 @@ def test_list_pkgs(): Test if it list the packages currently installed in a dict """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - assert rpm.list_pkgs() == {} + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + assert rpm_lowpkg.list_pkgs() == {} assert not _called_with_root(mock) @@ -80,8 +81,8 @@ def test_list_pkgs_root(): called with root parameter """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - rpm.list_pkgs(root="/") + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + rpm_lowpkg.list_pkgs(root="/") assert _called_with_root(mock) @@ -96,8 +97,8 @@ def test_verify(): mock = MagicMock( return_value={"stdout": "", "stderr": "", "retcode": 0, "pid": 12345} ) - with patch.dict(rpm.__salt__, {"cmd.run_all": mock}): - assert rpm.verify("httpd") == {} + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run_all": mock}): + assert rpm_lowpkg.verify("httpd") == {} assert not _called_with_root(mock) @@ -109,8 +110,8 @@ def test_verify_root(): mock = MagicMock( return_value={"stdout": "", "stderr": "", "retcode": 0, "pid": 12345} ) - with patch.dict(rpm.__salt__, {"cmd.run_all": mock}): - rpm.verify("httpd", root="/") + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run_all": mock}): + rpm_lowpkg.verify("httpd", root="/") assert _called_with_root(mock) @@ -122,8 +123,8 @@ def test_file_list(): Test if it list the files that belong to a package. """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - assert rpm.file_list("httpd") == {"errors": [], "files": []} + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + assert rpm_lowpkg.file_list("httpd") == {"errors": [], "files": []} assert not _called_with_root(mock) @@ -134,8 +135,8 @@ def test_file_list_root(): """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - rpm.file_list("httpd", root="/") + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + rpm_lowpkg.file_list("httpd", root="/") assert _called_with_root(mock) @@ -147,8 +148,8 @@ def test_file_dict(): Test if it list the files that belong to a package """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - assert rpm.file_dict("httpd") == {"errors": [], "packages": {}} + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + assert rpm_lowpkg.file_dict("httpd") == {"errors": [], "packages": {}} assert not _called_with_root(mock) @@ -157,8 +158,8 @@ def test_file_dict_root(): Test if it list the files that belong to a package """ mock = MagicMock(return_value="") - with patch.dict(rpm.__salt__, {"cmd.run": mock}): - rpm.file_dict("httpd", root="/") + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run": mock}): + rpm_lowpkg.file_dict("httpd", root="/") assert _called_with_root(mock) @@ -169,12 +170,12 @@ def test_owner(): """ Test if it return the name of the package that owns the file. """ - assert rpm.owner() == "" + assert rpm_lowpkg.owner() == "" ret = "file /usr/bin/salt-jenkins-build is not owned by any package" mock = MagicMock(return_value=ret) - with patch.dict(rpm.__salt__, {"cmd.run_stdout": mock}): - assert rpm.owner("/usr/bin/salt-jenkins-build") == "" + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run_stdout": mock}): + assert rpm_lowpkg.owner("/usr/bin/salt-jenkins-build") == "" assert not _called_with_root(mock) ret = { @@ -187,8 +188,8 @@ def test_owner(): "vim-enhanced-7.4.160-1.e17.x86_64", ] ) - with patch.dict(rpm.__salt__, {"cmd.run_stdout": mock}): - assert rpm.owner("/usr/bin/python", "/usr/bin/vim") == ret + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run_stdout": mock}): + assert rpm_lowpkg.owner("/usr/bin/python", "/usr/bin/vim") == ret assert not _called_with_root(mock) @@ -197,12 +198,12 @@ def test_owner_root(): Test if it return the name of the package that owns the file, using the parameter root. """ - assert rpm.owner() == "" + assert rpm_lowpkg.owner() == "" ret = "file /usr/bin/salt-jenkins-build is not owned by any package" mock = MagicMock(return_value=ret) - with patch.dict(rpm.__salt__, {"cmd.run_stdout": mock}): - rpm.owner("/usr/bin/salt-jenkins-build", root="/") + with patch.dict(rpm_lowpkg.__salt__, {"cmd.run_stdout": mock}): + rpm_lowpkg.owner("/usr/bin/salt-jenkins-build", root="/") assert _called_with_root(mock) @@ -220,8 +221,10 @@ def test_checksum(): } mock = MagicMock(side_effect=[True, 0, True, 1, False, 0]) - with patch.dict(rpm.__salt__, {"file.file_exists": mock, "cmd.retcode": mock}): - assert rpm.checksum("file1.rpm", "file2.rpm", "file3.rpm") == ret + with patch.dict( + rpm_lowpkg.__salt__, {"file.file_exists": mock, "cmd.retcode": mock} + ): + assert rpm_lowpkg.checksum("file1.rpm", "file2.rpm", "file3.rpm") == ret assert not _called_with_root(mock) @@ -231,45 +234,40 @@ def test_checksum_root(): root """ mock = MagicMock(side_effect=[True, 0]) - with patch.dict(rpm.__salt__, {"file.file_exists": mock, "cmd.retcode": mock}): - rpm.checksum("file1.rpm", root="/") + with patch.dict( + rpm_lowpkg.__salt__, {"file.file_exists": mock, "cmd.retcode": mock} + ): + rpm_lowpkg.checksum("file1.rpm", root="/") assert _called_with_root(mock) -@pytest.mark.parametrize("rpm_lib", ["HAS_RPM", "rpmdev-vercmp"]) -def test_version_cmp_rpm_all_libraries(rpm_lib): +@pytest.mark.skipif(not HAS_RPM, reason="python rpm module not available") +def test_version_cmp_rpm_lib(): """ Test package version when each library is installed """ - rpmdev = salt.utils.path.which("rpmdev-vercmp") - patch_cmd = patch.dict(rpm.__salt__, {"cmd.run_all": salt.modules.cmdmod.run_all}) - if rpm_lib == "rpmdev-vercmp": - if rpmdev: - patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", False) - else: - pytest.skip("The rpmdev-vercmp binary is not installed") - elif rpm_lib == "HAS_RPM": - if HAS_RPM: - patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", True) - else: - pytest.skip("The RPM lib is not installed, skipping") - else: - pytest.skip("The Python RPM lib is not installed, skipping") + patch_cmd = patch.dict( + rpm_lowpkg.__salt__, {"cmd.run_all": salt.modules.cmdmod.run_all} + ) + patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", True) with patch_rpm, patch_cmd: - assert rpm.version_cmp("1", "2") == -1 - assert rpm.version_cmp("2.9.1-6.el7_2.3", "2.9.1-6.el7.4") == -1 - assert rpm.version_cmp("3.2", "3.0") == 1 - assert rpm.version_cmp("3.0", "3.0") == 0 - assert rpm.version_cmp("1:2.9.1-6.el7_2.3", "2.9.1-6.el7.4") == 1 - assert rpm.version_cmp("1:2.9.1-6.el7_2.3", "1:2.9.1-6.el7.4") == -1 - assert rpm.version_cmp("2:2.9.1-6.el7_2.3", "1:2.9.1-6.el7.4") == 1 - assert rpm.version_cmp("3:2.9.1-6.el7.4", "3:2.9.1-6.el7.4") == 0 - assert rpm.version_cmp("3:2.9.1-6.el7.4", "3:2.9.1-7.el7.4") == -1 - assert rpm.version_cmp("3:2.9.1-8.el7.4", "3:2.9.1-7.el7.4") == 1 - assert rpm.version_cmp("3.23-6.el9", "3.23") == 0 - assert rpm.version_cmp("3.23", "3.23-6.el9") == 0 - assert rpm.version_cmp("release_web_294-6", "release_web_294_applepay-1") == -1 + assert rpm_lowpkg.version_cmp("1", "2") == -1 + assert rpm_lowpkg.version_cmp("2.9.1-6.el7_2.3", "2.9.1-6.el7.4") == -1 + assert rpm_lowpkg.version_cmp("3.2", "3.0") == 1 + assert rpm_lowpkg.version_cmp("3.0", "3.0") == 0 + assert rpm_lowpkg.version_cmp("1:2.9.1-6.el7_2.3", "2.9.1-6.el7.4") == 1 + assert rpm_lowpkg.version_cmp("1:2.9.1-6.el7_2.3", "1:2.9.1-6.el7.4") == -1 + assert rpm_lowpkg.version_cmp("2:2.9.1-6.el7_2.3", "1:2.9.1-6.el7.4") == 1 + 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("release_web_294-6", "release_web_294_applepay-1") + == -1 + ) def test_version_cmp_rpm(): @@ -284,38 +282,11 @@ def test_version_cmp_rpm(): patch_log = patch("salt.modules.rpm_lowpkg.log", mock_log) patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", True) with patch_label, patch_rpm, patch_log: - assert -1 == rpm.version_cmp("1", "2") + assert -1 == rpm_lowpkg.version_cmp("1", "2") assert not mock_log.warning.called assert mock_label.called -def test_version_cmp_rpmdev_vercmp(): - """ - Test package version if rpmdev-vercmp is installed - - :return: - """ - mock__salt__ = MagicMock(return_value={"retcode": 12}) - mock_log = MagicMock() - patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", False) - patch_which = patch("salt.utils.path.which", return_value=True) - patch_log = patch("salt.modules.rpm_lowpkg.log", mock_log) - - with patch_rpm, patch_which, patch_log: - with patch.dict(rpm.__salt__, {"cmd.run_all": mock__salt__}): - assert -1 == rpm.version_cmp("1", "2") - assert mock__salt__.called - assert mock_log.warning.called - assert ( - mock_log.warning.mock_calls[0][1][0] - == "Please install a package that provides rpm.labelCompare for more accurate version comparisons." - ) - assert ( - mock_log.warning.mock_calls[1][1][0] - == "Installing the rpmdevtools package may surface dev tools in production." - ) - - def test_version_cmp_python(): """ Test package version if falling back to python @@ -324,23 +295,427 @@ def test_version_cmp_python(): """ mock_log = MagicMock() patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", False) - mock_version_cmp = MagicMock(return_value=-1) - patch_cmp = patch("salt.utils.versions.version_cmp", mock_version_cmp) - patch_which = patch("salt.utils.path.which", return_value=False) patch_log = patch("salt.modules.rpm_lowpkg.log", mock_log) - with patch_rpm, patch_cmp, patch_which, patch_log: - assert -1 == rpm.version_cmp("1", "2") - assert mock_version_cmp.called - assert mock_log.warning.called + with patch_rpm, patch_log: + assert -1 == rpm_lowpkg.version_cmp("1", "2") + assert not mock_log.warning.called + assert mock_log.info.called assert ( - mock_log.warning.mock_calls[0][1][0] - == "Please install a package that provides rpm.labelCompare for more accurate version comparisons." + mock_log.info.mock_calls[0][1][0] + == "Install a package that provides rpm.labelCompare for faster version comparisons." ) + + +def _parse_label(label: str): + """Split full label into (epoch, version, release) for rpm.labelCompare.""" + epoch = None + version = None + release = None + if ":" in label: + epoch, rest = label.split(":", 1) + else: + rest = label + if "-" in rest: + version, release = rest.split("-", 1) + else: + version = rest + return (epoch, version, release) + + +VERSION_CASES = [ + # Basic equality and ordering + ("1", "1", 0), + ("1", "2", -1), + ("2", "1", 1), + ("1.0", "1.0", 0), + ("1.0", "2.0", -1), + ("2.0", "1.0", 1), + ("2.0.1", "2.0.1", 0), + ("2.0", "2.0.1", -1), + ("2.0.1", "2.0", 1), + # Epoch precedence + ("0:1.0-1", "1.0-1", 0), + ("1:1.0-1", "0:9.9-9", 1), + ("0:9.9-9", "1:1.0-1", -1), + ("02:1.0-1", "2:1.0-1", 0), + # Version vs release precedence (version decides before release) + ("1.0.1-1", "1.0-9", 1), + ("1.0-9", "1.0.1-1", -1), + # Numeric vs numeric + ("2.0-1", "10.0-1", -1), + ("10.0-1", "2.0-1", 1), + ("2.10-1", "2.2-1", 1), + ("2.02-1", "2.2-1", 0), + ("10.0001", "10.0001", 0), + ("10.0001", "10.1", 0), + ("10.1", "10.0001", 0), + ("10.0001", "10.0039", -1), + ("10.0039", "10.0001", 1), + ("010.0039", "10.0039", 0), + ("4.999.9", "5.0", -1), + ("5.0", "4.999.9", 1), + # Long numeric segments + ("2.12345678901234567890-1", "2.12345678901234567891-1", -1), + # Date-like numbers + ("20101121", "20101121", 0), + ("20101121", "20101122", -1), + ("20101122", "20101121", 1), + # 'p' in numeric-ish segments (treated as alphabetic break) + ("5.5p1", "5.5p1", 0), + ("5.5p1", "5.5p2", -1), + ("5.5p2", "5.5p1", 1), + ("5.5p10", "5.5p10", 0), + ("5.5p1", "5.5p10", -1), + ("5.5p10", "5.5p1", 1), + ("5.5p2", "5.6p1", -1), + ("5.6p1", "5.5p2", 1), + ("5.6p1", "6.5p1", -1), + ("6.5p1", "5.6p1", 1), + # Dot before alpha: dots split segments but alpha still compares after + ("2.0.1a", "2.0.1a", 0), + ("2.0.1a", "2.0.1", 1), + ("2.0.1", "2.0.1a", -1), + ("6.0.rc1", "6.0", 1), + ("6.0", "6.0.rc1", -1), + # other alphanumeric + ("10xyz", "10.1xyz", -1), + ("10.1xyz", "10xyz", 1), + ("xyz10", "xyz10", 0), + ("xyz10", "xyz10.1", -1), + ("xyz10.1", "xyz10", 1), + # capital letters + ("A", "A", 0), + ("A", "B", -1), + ("B", "a", -1), + ("A", "a", -1), + ("Ab", "Ab", 0), + ("A0", "A0", 0), + ("A", "a", -1), + ("a", "A", 1), + ("Ab", "aB", -1), + ("A.1", "a.1", -1), + ("Abc1", "abc1", -1), + ("abc1", "Abc1", 1), + ("A1b2", "A1B2", 1), + ("1A", "1a", -1), + ("1.A", "1.a", -1), + ("1.A.2", "1.a.2", -1), + ("1.0A", "1.0a", -1), + ("1.0a", "1.0A", 1), + ("1.0Aalpha", "1.0aalpha", -1), + # Alphanumeric segment vs pure-numeric segment ordering + # Alphanumeric segments (start with letters) should sort after pure-numeric versions; + # dotted alpha+numeric combos compare as expected and equality is preserved. + ("xyz.4", "xyz.4", 0), + ("xyz.4", "8", -1), + ("8", "xyz.4", 1), + ("xyz.4", "2", -1), + ("2", "xyz.4", 1), + # Alphabetic tails and mixed alpha-numeric comparisons + # Alphabetic suffixes are compared lexicographically; when alpha tails share a prefix + # the shorter tail sorts earlier. Mixed numeric+alpha+numeric tokens use the numeric + # prefix first, then alphabetic ordering to break ties. + ("a", "a", 0), + ("2a-1", "2-1", 1), + ("2-1", "2a-1", -1), + ("2alpha-1", "2beta-1", -1), + ("2.0.1a", "2.0.1a", 0), + ("alpha-1", "beta-1", -1), + ("beta-1", "alpha-1", 1), + ("10b2", "10a1", 1), + ("10a2", "10b2", -1), + ("1.0aa", "1.0aa", 0), + ("1.0a", "1.0aa", -1), + ("1.0aa", "1.0a", 1), + ("1.0~A", "1.0~a", -1), + ("1.0~a", "1.0~A", 1), + ("1.0^A", "1.0^a", -1), + ("1.0^a", "1.0^A", 1), + ("1.0+A", "1.0+a", -1), + ("1.0+a", "1.0+A", 1), + # Tilde: pre-release; sorts older than anything without it + ("2.0~beta-1", "2.0-1", -1), + ("2.0-1", "2.0~beta-1", 1), + ("2.0~beta-1", "2.0~rc-1", -1), # beta < rc lexicographically + ("1.0~rc1", "1.0~rc1", 0), + ("1.0~rc1", "1.0", -1), + ("1.0", "1.0~rc1", 1), + ("1.0~rc1", "1.0~rc2", -1), + ("1.0~rc2", "1.0~rc1", 1), + # Tilde chaining + ("1.0~rc1~git123", "1.0~rc1~git123", 0), + ("1.0~rc1~git123", "1.0~rc1", -1), + ("1.0~rc1", "1.0~rc1~git123", 1), + # Caret: post-release; newer than base but lower than next increment + ("1.0^", "1.0^", 0), + ("1.0^", "1.0", 1), + ("1.0", "1.0^", -1), + ("1.0^git1", "1.0", 1), + ("1.0", "1.0^git1", -1), + ("1.0^git1", "1.0^git1", 0), + ("1.0^git1", "1.0^git2", -1), + ("1.0^git2", "1.0^git1", 1), + ("1.0^git9", "1.0.1", -1), # caret block still less than next numeric bump + ("1.0.1", "1.0^git9", 1), + ("1.0^git1", "1.01", -1), + ("1.01", "1.0^git1", 1), + ("1.0^20160101", "1.0^20160101", 0), + ("1.0^20160101", "1.0.1", -1), + ("1.0.1", "1.0^20160101", 1), + ("1.0^20160101^git1", "1.0^20160101^git1", 0), + ("1.0^20160102", "1.0^20160101^git1", 1), + ("1.0^20160101^git1", "1.0^20160102", -1), + # Caret + tilde + ("1.0~rc1^git1", "1.0~rc1", 1), + ("1.0~rc1", "1.0~rc1^git1", -1), + ("1.0^git1~pre", "1.0^git1", -1), + ("1.0^git1", "1.0^git1~pre", 1), + # Separators: '.', '_', '+' are equivalent and collapse + ("2_0", "2.0", 0), + ("2.0", "2_0", 0), + ("2+0", "2.0", 0), + ("2.0", "2+0", 0), + ("2_0", "2+0", 0), + # Collapsing multiple separators + ("1+.+0", "1.0", 0), + ("1_._0", "1.0", 0), + ("1+_+0", "1.0", 0), + # Plus/underscore equivalences at segment edges + ("a+", "a_", 0), + ("a+", "a+", 0), + ("a+", "a_", 0), + ("a_", "a+", 0), + ("+a", "+a", 0), + ("+a", "_a", 0), + ("_a", "+a", 0), + ("+_", "+_", 0), + ("+_", "_+", 0), + ("+", "_", 0), + ("_", "+", 0), + # Mixed separators with numbers + ("1+2", "1.2", 0), + ("1_2", "1.2", 0), + ("1+_2", "1.2", 0), + # Release segment variations + ("2.0-1-alpha", "2.0-1-beta", -1), + ("2.0-1-beta", "2.0-1-alpha", 1), + ("2.0-1-alpha-1", "2.0-1-alpha-2", -1), + ("2.0-1.alpha-beta.1", "2.0-1.alpha-beta.2", -1), + ("2.0-1.alpha-beta.2", "2.0-1.alpha-beta.1", 1), + ("2.0-1.alpha-beta.gamma", "2.0-1.alpha-beta.delta", 1), + # Release containing periods + trailing zeros (RPM: longer wins, even if .0) + ("2.0-1.0.0", "2.0-1.0", 1), + ("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), + # Different number of segments + ("1.0", "1.0.1", -1), + ("1.0.1", "1.0", 1), + ("1.0", "1.0.0", -1), + ("1.0.0.1", "1.0", 1), + ("1.0a", "1.0.a", 0), + # Different number of release segments + ("1.0-1", "1.0-1.1", -1), + ("1.0-1.1", "1.0-1", 1), + ("1.0-1", "1.0-1.0", -1), + ("1.0-1.0.1", "1.0-1", 1), + # Mixed alpha/numeric with extra segments in version + ("1.0alpha", "1.0alpha.1", -1), + ("1.0alpha.1", "1.0alpha", 1), + ("1.0alpha.0", "1.0alpha", 1), + # Mixed alpha/numeric with extra segments in release + ("1.0-1-alpha", "1.0-1-alpha.1", -1), + ("1.0-1-alpha.1", "1.0-1-alpha", 1), + ("1.0-1-alpha.0", "1.0-1-alpha", 1), + # Multiple hyphens inside the release + ("1.0-1-alpha-beta", "1.0-1-alpha-gamma", -1), + ("1.0-1-alpha-gamma", "1.0-1-alpha-beta", 1), + ("1.0-1-alpha-beta", "1.0-1-alpha", 1), + ("1.0-1-alpha", "1.0-1-alpha-beta", -1), + ("1.0-1-alpha-1", "1.0-1-alpha", 1), + ("1.0-1-alpha", "1.0-1-alpha-1", -1), + ("2.0-1-alpha-beta.2", "2.0-1-alpha-beta.10", -1), + ("2.0-1-alpha-beta.10", "2.0-1-alpha-beta.2", 1), + # Releases containing multiple numeric hyphen segments + ("1.0-1-1", "1.0-1-2", -1), + ("1.0-1-2", "1.0-1-1", 1), + ("1.0-1-01", "1.0-1-1", 0), + # Releases with separators and multiple operator segments + ("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), + # 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 + ("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 + ("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), + ( + "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), + ("2.0.2-1", "2.0.10", -1), + ("2.0.10", "2.0.2-1", 1), + # Special chars in wild + ("2.0_git20210101-1", "2.0_git20201231-1", 1), + ("2.0+dfsg-1", "2.0-1", 1), + # More separator/operator chains + ("1.0+rc1", "1.0_rc1", 0), + ("1.0+rc1", "1.0.rc1", 0), + ("1.0+rc1^git1", "1.0.rc1^git1", 0), + ("1.0~beta+1", "1.0~beta.1", 0), + ("1.0~beta_1", "1.0~beta.1", 0), + # Caret blocks vs longer alpha tail + ("1.0^a", "1.0a", -1), + ("1.0a", "1.0^a", 1), + # Tilde vs caret at same position + ("1.0~a", "1.0^a", -1), # tilde is always older + ("1.0^a", "1.0~a", 1), + # Chained: tilde inside caret block + ("1.0^a~b", "1.0^a", -1), + ("1.0^a", "1.0^a~b", 1), + # Chained: caret after tilde block + ("1.0~a^b", "1.0~a", 1), + ("1.0~a", "1.0~a^b", -1), + # Multi-caret sequence + ("1.0^a^b", "1.0^a^c", -1), + ("1.0^a^c", "1.0^a^b", 1), + # Mixed separators around operators + ("1.0+^git1", "1.0^git1", 0), + ("1.0_^git1", "1.0^git1", 0), + ("1.0+~rc1", "1.0~rc1", 0), + ("1.0_~rc1", "1.0~rc1", 0), +] + + +@pytest.mark.parametrize("label1, label2, expected", VERSION_CASES) +def test_version_cmp_expected(label1, label2, expected): + patch_rpm = patch("salt.modules.rpm_lowpkg.HAS_RPM", False) + with patch_rpm: + result = rpm_lowpkg.version_cmp(label1, label2) assert ( - mock_log.warning.mock_calls[1][1][0] - == "Falling back on salt.utils.versions.version_cmp() for version comparisons" + result == expected + ), f"{label1} vs {label2} => {result}, expected {expected}" + + +@pytest.mark.parametrize("label1, label2, _", VERSION_CASES) +@pytest.mark.skipif(not HAS_RPM, reason="python rpm module not available") +def test_version_cmp_matches_rpm(label1, label2, _): + evr1 = _parse_label(label1) + evr2 = _parse_label(label2) + 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}: " + f"pure-Python={py_result}, rpm={rpm_result}" + ) + + +def test_symmetry_and_transitivity(): + # Small chain that exercises epoch, version, and release ordering properties + chain = [ + "0:1.0-1", + "0:1.0-2", + "0:1.0^git1-1", + "0:1.0.1-1", + "1:0.1-1", + ] + # Check reflexivity and antisymmetry + for a in chain: + assert rpm_lowpkg.version_cmp(a, a) == 0 + # Check ordering across the chain is strictly increasing + for i in range(len(chain) - 1): + a, b = chain[i], chain[i + 1] + assert rpm_lowpkg.version_cmp(a, b) < 0 + assert rpm_lowpkg.version_cmp(b, a) > 0 + + +# ===== Optional fuzzing ===== +@pytest.mark.skipif(not HAS_RPM, reason="python rpm module not available") +def test_fuzz_against_rpm(): + """ + Deterministic fuzz over a restricted alphabet to spot regressions against rpm.labelCompare(). + """ + rng = random.Random(1337) + alpha = string.ascii_lowercase + digits = string.digits + seps = "._+" + ops = "~^" + + # Limit length to keep the space sane for CI + def rand_token(): + t = [] + for _ in range(rng.randint(1, 6)): + choice = rng.random() + if choice < 0.55: + t.append(rng.choice(digits)) + elif choice < 0.85: + t.append(rng.choice(alpha)) + elif choice < 0.95: + t.append(rng.choice(seps)) + else: + t.append(rng.choice(ops)) + return "".join(t).strip("._+") + + def rand_evr(): + # 30% chance epoch, 70% none + epoch = str(rng.randint(0, 3)) if rng.random() < 0.3 else None + version = rand_token() or "1" + release = rand_token() if rng.random() < 0.6 else None + return (epoch, version, release) + + for _ in range(2000): + evr1 = rand_evr() + evr2 = rand_evr() + l1 = ( + ("" if evr1[0] is None else f"{evr1[0]}:") + + evr1[1] + + ("" if evr1[2] is None else f"-{evr1[2]}") ) + l2 = ( + ("" if evr2[0] is None else f"{evr2[0]}:") + + evr2[1] + + ("" if evr2[2] is None else f"-{evr2[2]}") + ) + py = rpm_lowpkg.version_cmp(l1, l2) + rp = rpm.labelCompare(evr1, evr2) + if py < 0: + assert rp < 0, (l1, l2, evr1, evr2, py, rp) + elif py > 0: + assert rp > 0, (l1, l2, evr1, evr2, py, rp) + else: + assert rp == 0, (l1, l2, evr1, evr2, py, rp) @pytest.mark.skip_on_windows @@ -411,6 +786,6 @@ def test_info(): "description": "The GNU Bourne Again shell (Bash) is a shell or command language\ninterpreter that is compatible with the Bourne shell (sh). Bash\nincorporates useful features from the Korn shell (ksh) and the C shell\n(csh). Most sh scripts can be run by bash without modification.", } } - with patch.dict(rpm.__salt__, dunder_salt): - result = rpm.info("bash") + with patch.dict(rpm_lowpkg.__salt__, dunder_salt): + result = rpm_lowpkg.info("bash") assert result == expected, result diff --git a/tests/pytests/unit/modules/test_yumpkg.py b/tests/pytests/unit/modules/test_yumpkg.py index 04fa56986ebb..159829a1f3e4 100644 --- a/tests/pytests/unit/modules/test_yumpkg.py +++ b/tests/pytests/unit/modules/test_yumpkg.py @@ -6,7 +6,7 @@ import salt.modules.cmdmod as cmdmod import salt.modules.pkg_resource as pkg_resource -import salt.modules.rpm_lowpkg as rpm +import salt.modules.rpm_lowpkg as rpm_lowpkg import salt.modules.yumpkg as yumpkg import salt.utils.platform from salt.exceptions import CommandExecutionError, MinionError, SaltInvocationError @@ -1493,7 +1493,7 @@ def test_remove_retcode_error(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: installed}, "repository") ), @@ -1530,7 +1530,7 @@ def test_remove_with_epoch(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: installed}, "repository") ), @@ -1575,7 +1575,7 @@ def test_remove_with_epoch_and_arch_info(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name_and_arch: installed}, "repository") ), @@ -1617,7 +1617,7 @@ def test_remove_with_wildcard(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: installed}, "repository") ), @@ -1657,7 +1657,7 @@ def test_install_with_epoch(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: new}, "repository") ), @@ -1758,7 +1758,7 @@ def test_install_error_reporting(): ) salt_mock = { "cmd.run_all": cmdmod.run_all, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: new}, "repository") ), @@ -1799,7 +1799,7 @@ def test_remove_not_installed(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: None}, "repository") ), @@ -1946,7 +1946,7 @@ def test_purge_not_installed(): ) salt_mock = { "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": MagicMock( return_value=({name: None}, "repository") ), @@ -3230,7 +3230,7 @@ def fake_parse(*args, **kwargs): { "cmd.run": MagicMock(return_value=""), "cmd.run_all": cmd_mock, - "lowpkg.version_cmp": rpm.version_cmp, + "lowpkg.version_cmp": rpm_lowpkg.version_cmp, "pkg_resource.parse_targets": fake_parse, "pkg_resource.format_pkg_list": pkg_resource.format_pkg_list, },