From f46a7030aae85c08377bc13129836c8169d886fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Thu, 5 Mar 2026 15:49:10 +0100 Subject: [PATCH 1/3] fix(pip): normalize names in list_freeze_parse and detect modern pip satisfied message --- changelog/68784.fixed.md | 11 ++++ salt/modules/pip.py | 11 ++-- salt/states/pip_state.py | 47 +++++++------- tests/pytests/unit/modules/test_pip.py | 50 +++++++++++++-- tests/pytests/unit/states/test_pip.py | 86 ++++++++++++++++++++++++++ 5 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 changelog/68784.fixed.md diff --git a/changelog/68784.fixed.md b/changelog/68784.fixed.md new file mode 100644 index 000000000000..af47fee0e0bf --- /dev/null +++ b/changelog/68784.fixed.md @@ -0,0 +1,11 @@ +Fix `pip.installed` state reinstalling packages on every run even when the +correct version is already present: + +- `pip.list_freeze_parse` now normalizes package names (lowercase, hyphens) + consistent with `pip.list`, so that packages whose `pip freeze` name uses + underscores or mixed case (e.g. `requests_oauthlib`) are correctly detected + as already installed when looked up by their normalized name. +- The post-install check in `pip.installed` now also recognizes + `"Requirement already satisfied:"` (modern pip ≥ 10.0) in addition to the + old `"Requirement already up-to-date:"` message, preventing packages + confirmed as already present from being falsely reported as changed. diff --git a/salt/modules/pip.py b/salt/modules/pip.py index 09a421288a6c..5316f6cdf684 100644 --- a/salt/modules/pip.py +++ b/salt/modules/pip.py @@ -1298,7 +1298,9 @@ def list_freeze_parse( cwd = _pip_bin_env(cwd, bin_env) packages = {} - if prefix is None or "pip".startswith(prefix): + normal_prefix = normalize(prefix) if prefix else None + + if prefix is None or "pip".startswith(normal_prefix): packages["pip"] = version(bin_env, cwd) for line in freeze( @@ -1332,11 +1334,12 @@ def list_freeze_parse( logger.error("Can't parse line '%s'", line) continue + normal_name = normalize(name) if prefix: - if name.lower().startswith(prefix.lower()): - packages[name] = version_ + if normal_name.startswith(normal_prefix): + packages[normal_name] = version_ else: - packages[name] = version_ + packages[normal_name] = version_ return packages diff --git a/salt/states/pip_state.py b/salt/states/pip_state.py index e8f04841cb04..0d97d325f40b 100644 --- a/salt/states/pip_state.py +++ b/salt/states/pip_state.py @@ -19,6 +19,7 @@ """ import logging +import re import sys import types @@ -210,13 +211,10 @@ def _check_pkg_version_format(pkg): ret["result"] = False if not from_vcs and "=" in pkg and "==" not in pkg: ret["comment"] = ( - "Invalid version specification in package {}. '=' is " - "not supported, use '==' instead.".format(pkg) + f"Invalid version specification in package {pkg}. '=' is not supported, use '==' instead." ) return ret - ret["comment"] = "pip raised an exception while parsing '{}': {}".format( - pkg, exc - ) + ret["comment"] = f"pip raised an exception while parsing '{pkg}': {exc}" return ret if install_req.req is None: @@ -293,8 +291,8 @@ def _check_if_installed( and _fulfills_version_spec(pip_list[prefix], version_spec) ) or (not any(version_spec)): ret["result"] = True - ret["comment"] = "Python package {} was already installed".format( - state_pkg_name + ret["comment"] = ( + f"Python package {state_pkg_name} was already installed" ) return ret if force_reinstall is False and upgrade: @@ -340,8 +338,8 @@ def _check_if_installed( return ret if _pep440_version_cmp(pip_list[prefix], desired_version) == 0: ret["result"] = True - ret["comment"] = "Python package {} was already installed".format( - state_pkg_name + ret["comment"] = ( + f"Python package {state_pkg_name} was already installed" ) return ret @@ -869,8 +867,9 @@ def prepro(pkg): ) if editable: comments.append( - "Package will be installed in editable mode (i.e. " - 'setuptools "develop mode") from {}.'.format(editable) + 'Package will be installed in editable mode (i.e. setuptools "develop mode") from {}.'.format( + editable + ) ) ret["comment"] = " ".join(comments) return ret @@ -1039,18 +1038,14 @@ def prepro(pkg): ret["changes"]["requirements"] = True if ret["changes"].get("requirements"): comments.append( - "Successfully processed requirements file {}.".format( - requirements - ) + f"Successfully processed requirements file {requirements}." ) else: comments.append("Requirements were already installed.") if editable: comments.append( - "Package successfully installed from VCS checkout {}.".format( - editable - ) + f"Package successfully installed from VCS checkout {editable}." ) ret["changes"]["editable"] = True ret["comment"] = " ".join(comments) @@ -1062,10 +1057,18 @@ def prepro(pkg): already_installed_packages = set() for line in pip_install_call.get("stdout", "").split("\n"): # Output for already installed packages: - # 'Requirement already up-to-date: jinja2 in /usr/local/lib/python2.7/dist-packages\nCleaning up...' - if line.startswith("Requirement already up-to-date: "): - package = line.split(":", 1)[1].split()[0] - already_installed_packages.add(package.lower()) + # modern pip: 'Requirement already satisfied: jinja2 in /usr/local/lib/...' + # old pip: 'Requirement already up-to-date: jinja2 in /usr/local/lib/python2.7/...' + if line.startswith( + ( + "Requirement already satisfied: ", + "Requirement already up-to-date: ", + ) + ): + pkg_str = line.split(":", 1)[1].split()[0] + # Strip version specifier to get just the package name + pkg_name = re.split(r"[=!<>~@]", pkg_str)[0] + already_installed_packages.add(__salt__["pip.normalize"](pkg_name)) for prefix, state_name in target_pkgs: # Case for packages that are not an URL @@ -1092,7 +1095,7 @@ def prepro(pkg): else: if ( prefix in pipsearch - and prefix.lower() not in already_installed_packages + and prefix not in already_installed_packages ): ver = pipsearch[prefix] ret["changes"][f"{prefix}=={ver}"] = "Installed" diff --git a/tests/pytests/unit/modules/test_pip.py b/tests/pytests/unit/modules/test_pip.py index 468b2983e1cc..14c51042c73e 100644 --- a/tests/pytests/unit/modules/test_pip.py +++ b/tests/pytests/unit/modules/test_pip.py @@ -1458,8 +1458,8 @@ def test_list_freeze_parse_command(python_binary): use_vt=False, ) assert ret == { - "SaltTesting-dev": "git+git@github.com:s0undt3ch/salt-testing.git@9ed81aa2f918d59d3706e56b18f0782d1ea43bf8", - "M2Crypto": "0.21.1", + "salttesting-dev": "git+git@github.com:s0undt3ch/salt-testing.git@9ed81aa2f918d59d3706e56b18f0782d1ea43bf8", + "m2crypto": "0.21.1", "bbfreeze-loader": "1.1.0", "bbfreeze": "1.1.0", "pip": mock_version, @@ -1503,8 +1503,8 @@ def test_list_freeze_parse_command_with_all(python_binary): use_vt=False, ) assert ret == { - "SaltTesting-dev": "git+git@github.com:s0undt3ch/salt-testing.git@9ed81aa2f918d59d3706e56b18f0782d1ea43bf8", - "M2Crypto": "0.21.1", + "salttesting-dev": "git+git@github.com:s0undt3ch/salt-testing.git@9ed81aa2f918d59d3706e56b18f0782d1ea43bf8", + "m2crypto": "0.21.1", "bbfreeze-loader": "1.1.0", "bbfreeze": "1.1.0", "pip": "9.0.1", @@ -1545,6 +1545,48 @@ def test_list_freeze_parse_command_with_prefix(python_binary): assert ret == {"bbfreeze-loader": "1.1.0", "bbfreeze": "1.1.0"} +def test_list_freeze_parse_normalizes_package_names(python_binary): + """ + list_freeze_parse must return normalized package names (lowercase, hyphens) + consistent with list_(), so that pip_list lookups work correctly regardless + of how the name appears in `pip freeze` output (underscores, mixed case, etc.). + """ + eggs = [ + "requests_oauthlib==1.3.0", + "My_Package==2.0.0", + "Pillow==10.0.0", + ] + mock = MagicMock(return_value={"retcode": 0, "stdout": "\n".join(eggs)}) + with patch.dict(pip.__salt__, {"cmd.run_all": mock}): + with patch("salt.modules.pip.version", MagicMock(return_value="6.1.1")): + ret = pip.list_freeze_parse() + assert ret == { + "requests-oauthlib": "1.3.0", + "my-package": "2.0.0", + "pillow": "10.0.0", + "pip": "6.1.1", + } + + +def test_list_freeze_parse_prefix_matches_normalized_name(python_binary): + """ + list_freeze_parse must match packages by normalized prefix even when the + freeze output uses underscores but the caller uses hyphens (or vice versa). + This ensures _check_if_installed does not produce false negatives. + """ + eggs = [ + "requests_oauthlib==1.3.0", + "requests==2.31.0", + "other_pkg==0.1.0", + ] + mock = MagicMock(return_value={"retcode": 0, "stdout": "\n".join(eggs)}) + with patch.dict(pip.__salt__, {"cmd.run_all": mock}): + with patch("salt.modules.pip.version", MagicMock(return_value="6.1.1")): + # A hyphenated prefix must match an underscore-named package + ret = pip.list_freeze_parse(prefix="requests-oauthlib") + assert ret == {"requests-oauthlib": "1.3.0"} + + def test_list_upgrades_legacy(python_binary): eggs = [ "apache-libcloud (Current: 1.1.0 Latest: 2.2.1 [wheel])", diff --git a/tests/pytests/unit/states/test_pip.py b/tests/pytests/unit/states/test_pip.py index 92061b0263b1..d7c904511567 100644 --- a/tests/pytests/unit/states/test_pip.py +++ b/tests/pytests/unit/states/test_pip.py @@ -71,3 +71,89 @@ def test_issue_64169(caplog): # Confirm that the state continued to install the package as expected. # Only check the 'pkgs' parameter of pip.install assert mock_pip_install.call_args.kwargs["pkgs"] == pkg_to_install + + +def test_already_satisfied_not_reported_as_change(): + """ + When pip outputs 'Requirement already satisfied' (modern pip >= 10) for a + package that ended up in target_pkgs, the state must NOT report it as a + change. Previously only the old 'Requirement already up-to-date' message + was checked, causing the state to always report the package as installed. + """ + pkg_name = "my-package" + pkg_version = "1.0.0" + + mock_pip_list = MagicMock( + side_effect=[ + {}, # pre-cache: empty → package goes to target_pkgs + {}, # _check_if_installed fallback: package not found + {pkg_name: pkg_version}, # post-install verification + ] + ) + mock_pip_version = MagicMock(return_value="24.0.0") + mock_pip_install = MagicMock( + return_value={ + "retcode": 0, + "stdout": f"Requirement already satisfied: {pkg_name} in /path/to/site-packages", + } + ) + + with patch.dict( + pip_state.__salt__, + { + "pip.list": mock_pip_list, + "pip.version": mock_pip_version, + "pip.install": mock_pip_install, + "pip.normalize": pip_module.normalize, + }, + ): + ret = pip_state.installed(name=pkg_name) + + assert ret["result"] is True + # The package was already satisfied — no changes should be reported + assert ( + ret["changes"] == {} + ), "Package reported as 'Requirement already satisfied' must not appear in changes" + + +def test_already_satisfied_with_version_spec_not_reported_as_change(): + """ + When pip outputs 'Requirement already satisfied: pkg==x.y.z ...' (with a + version specifier in the message), the version suffix must be stripped when + checking against already_installed_packages so the package is still + correctly excluded from changes. + """ + pkg_name = "my-package" + pkg_version = "1.0.0" + + mock_pip_list = MagicMock( + side_effect=[ + {}, # pre-cache: empty + {}, # _check_if_installed fallback + {pkg_name: pkg_version}, # post-install verification + ] + ) + mock_pip_version = MagicMock(return_value="24.0.0") + mock_pip_install = MagicMock( + return_value={ + "retcode": 0, + # pip includes the version spec in the satisfied message + "stdout": f"Requirement already satisfied: {pkg_name}=={pkg_version} in /path", + } + ) + + with patch.dict( + pip_state.__salt__, + { + "pip.list": mock_pip_list, + "pip.version": mock_pip_version, + "pip.install": mock_pip_install, + "pip.normalize": pip_module.normalize, + }, + ): + ret = pip_state.installed(name=pkg_name) + + assert ret["result"] is True + assert ( + ret["changes"] == {} + ), "Package with version spec in satisfied message must not appear in changes" From 94d335495a5e08b00ab9942e23bbeaee1475a54c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Tue, 10 Mar 2026 22:30:35 +0100 Subject: [PATCH 2/3] ref: change string formatting --- salt/states/pip_state.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/salt/states/pip_state.py b/salt/states/pip_state.py index 0d97d325f40b..2af8ff02d430 100644 --- a/salt/states/pip_state.py +++ b/salt/states/pip_state.py @@ -867,9 +867,7 @@ def prepro(pkg): ) if editable: comments.append( - 'Package will be installed in editable mode (i.e. setuptools "develop mode") from {}.'.format( - editable - ) + f'Package will be installed in editable mode (i.e. setuptools "develop mode") from {editable}.' ) ret["comment"] = " ".join(comments) return ret From 2b5c39bc42fad79c7bdba1b6c25c55e100b0c83a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20=C3=81lvaro?= Date: Wed, 1 Apr 2026 21:19:32 +0200 Subject: [PATCH 3/3] enh: add PR suggestions --- salt/modules/pip.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/salt/modules/pip.py b/salt/modules/pip.py index 5316f6cdf684..caea655524f3 100644 --- a/salt/modules/pip.py +++ b/salt/modules/pip.py @@ -1300,7 +1300,7 @@ def list_freeze_parse( normal_prefix = normalize(prefix) if prefix else None - if prefix is None or "pip".startswith(normal_prefix): + if normal_prefix is None or "pip".startswith(normal_prefix): packages["pip"] = version(bin_env, cwd) for line in freeze( @@ -1335,7 +1335,7 @@ def list_freeze_parse( continue normal_name = normalize(name) - if prefix: + if normal_prefix: if normal_name.startswith(normal_prefix): packages[normal_name] = version_ else: