fix(pip): normalize names in list_freeze_parse and detect modern pip …

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.

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 normal_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
 
-        if prefix:
-            if name.lower().startswith(prefix.lower()):
-                packages[name] = version_
+        normal_name = normalize(name)
+        if normal_prefix:
+            if normal_name.startswith(normal_prefix):
+                packages[normal_name] = version_
         else:
-            packages[name] = version_
+            packages[normal_name] = version_
 
     return packages
 

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,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
@@ -1039,18 +1036,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 +1055,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 +1093,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"

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])",

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"