pacman: implement change detection for update_cache=true; add cache_updated return value (#4337) (#4359)

* Implement change detection for update_cache=true. Add cache_updated return value.

* ...

* Make sure pacman --sync --list is called only as often as necessary.

(cherry picked from commit cf4d68ac50)

Co-authored-by: Felix Fontein <felix@fontein.de>

changelogs/fragments/4337-pacman-update_cache.yml

@@ -0,0 +1,4 @@
+minor_changes:
+  - "pacman - now implements proper change detection for ``update_cache=true``. Adds ``cache_updated`` return value to when ``update_cache=true``
+     to report this result independently of the module's overall changed return value
+     (https://github.com/ansible-collections/community.general/pull/4337)."

plugins/modules/packaging/os/pacman.py

@@ -125,6 +125,15 @@ packages:
     elements: str
     sample: [ package, other-package ]
 
+cache_updated:
+    description:
+        - The changed status of C(pacman -Sy).
+        - Useful when I(name) or I(upgrade=true) are specified next to I(update_cache=true).
+    returned: success, when I(update_cache=true)
+    type: bool
+    sample: false
+    version_added: 4.6.0
+
 stdout:
     description:
         - Output from pacman.
@@ -251,6 +260,8 @@ class Pacman(object):
 
         self.pacman_path = self.m.get_bin_path(p["executable"], True)
 
+        self._cached_database = None
+
         # Normalize for old configs
         if p["state"] == "installed":
             self.target_state = "present"
@@ -413,6 +424,7 @@ class Pacman(object):
             if rc != 0:
                 self.fail("Failed to install package(s)", cmd=cmd, stdout=stdout, stderr=stderr)
             self.add_exit_infos(stdout=stdout, stderr=stderr)
+            self._invalidate_database()
 
         if pkgs_to_install:
             _install_packages_for_real("--sync", pkgs_to_install)
@@ -467,6 +479,7 @@ class Pacman(object):
         rc, stdout, stderr = self.m.run_command(cmd, check_rc=False)
         if rc != 0:
             self.fail("failed to remove package(s)", cmd=cmd, stdout=stdout, stderr=stderr)
+        self._invalidate_database()
         self.exit_params["packages"] = removed_pkgs
         self.add_exit_infos("Removed %d package(s)" % len(removed_pkgs), stdout=stdout, stderr=stderr)
 
@@ -502,16 +515,29 @@ class Pacman(object):
             if self.m.params["upgrade_extra_args"]:
                 cmd += self.m.params["upgrade_extra_args"]
             rc, stdout, stderr = self.m.run_command(cmd, check_rc=False)
+            self._invalidate_database()
             if rc == 0:
                 self.add_exit_infos("System upgraded", stdout=stdout, stderr=stderr)
             else:
                 self.fail("Could not upgrade", cmd=cmd, stdout=stdout, stderr=stderr)
 
+    def _list_database(self):
+        """runs pacman --sync --list with some caching"""
+        if self._cached_database is None:
+            dummy, packages, dummy = self.m.run_command([self.pacman_path, '--sync', '--list'], check_rc=True)
+            self._cached_database = packages.splitlines()
+        return self._cached_database
+
+    def _invalidate_database(self):
+        """invalidates the pacman --sync --list cache"""
+        self._cached_database = None
+
     def update_package_db(self):
         """runs pacman --sync --refresh"""
         if self.m.check_mode:
             self.add_exit_infos("Would have updated the package db")
             self.changed = True
+            self.exit_params["cache_updated"] = True
             return
 
         cmd = [
@@ -523,10 +549,23 @@ class Pacman(object):
             cmd += self.m.params["update_cache_extra_args"]
         if self.m.params["force"]:
             cmd += ["--refresh"]
+        else:
+            # Dump package database to get contents before update
+            pre_state = sorted(self._list_database())
 
         rc, stdout, stderr = self.m.run_command(cmd, check_rc=False)
+        self._invalidate_database()
 
-        self.changed = True
+        if self.m.params["force"]:
+            # Always changed when force=true
+            self.exit_params["cache_updated"] = True
+        else:
+            # Dump package database to get contents after update
+            post_state = sorted(self._list_database())
+            # If contents changed, set changed=true
+            self.exit_params["cache_updated"] = pre_state != post_state
+        if self.exit_params["cache_updated"]:
+            self.changed = True
 
         if rc == 0:
             self.add_exit_infos("Updated package db", stdout=stdout, stderr=stderr)
@@ -622,9 +661,9 @@ class Pacman(object):
             installed_groups[group].add(pkgname)
 
         available_pkgs = {}
-        dummy, stdout, dummy = self.m.run_command([self.pacman_path, "--sync", "--list"], check_rc=True)
+        database = self._list_database()
         # Format of a line: "core pacman 6.0.1-2"
-        for l in stdout.splitlines():
+        for l in database:
             l = l.strip()
             if not l:
                 continue

tests/integration/targets/pacman/tasks/main.yml

@@ -10,3 +10,4 @@
     - include: 'basic.yml'
     - include: 'package_urls.yml'
     - include: 'remove_nosave.yml'
+    - include: 'update_cache.yml'

tests/integration/targets/pacman/tasks/update_cache.yml

@@ -0,0 +1,23 @@
+---
+- name: Make sure package cache is updated
+  pacman:
+    update_cache: true
+
+- name: Update package cache again (should not be changed)
+  pacman:
+    update_cache: true
+  register: update_cache_idem
+
+- name: Update package cache again with force=true (should be changed)
+  pacman:
+    update_cache: true
+    force: true
+  register: update_cache_force
+
+- name: Check conditions
+  assert:
+    that:
+      - update_cache_idem is not changed
+      - update_cache_idem.cache_updated == false
+      - update_cache_force is changed
+      - update_cache_force.cache_updated == true

tests/unit/plugins/modules/packaging/os/test_pacman.py

@@ -329,33 +329,75 @@ class TestPacman:
         assert out["changed"]
 
     @pytest.mark.parametrize(
-        "module_args,expected_call",
+        "module_args,expected_calls,changed",
         [
-            ({}, ["pacman", "--sync", "--refresh"]),
-            ({"force": True}, ["pacman", "--sync", "--refresh", "--refresh"]),
             (
-                {"update_cache_extra_args": "--some-extra args"},
+                {},
-                ["pacman", "--sync", "--refresh", "--some-extra", "args"],  # shlex test
+                [
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'a\nb\nc', ''),
+                    (["pacman", "--sync", "--refresh"], {'check_rc': False}, 0, 'stdout', 'stderr'),
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'b\na\nc', ''),
+                ],
+                False,
+            ),
+            (
+                {"force": True},
+                [
+                    (["pacman", "--sync", "--refresh", "--refresh"], {'check_rc': False}, 0, 'stdout', 'stderr'),
+                ],
+                True,
+            ),
+            (
+                {"update_cache_extra_args": "--some-extra args"},  # shlex test
+                [
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'a\nb\nc', ''),
+                    (["pacman", "--sync", "--refresh", "--some-extra", "args"], {'check_rc': False}, 0, 'stdout', 'stderr'),
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'a changed\nb\nc', ''),
+                ],
+                True,
             ),
             (
                 {"force": True, "update_cache_extra_args": "--some-extra args"},
-                ["pacman", "--sync", "--refresh", "--some-extra", "args", "--refresh"],
+                [
+                    (["pacman", "--sync", "--refresh", "--some-extra", "args", "--refresh"], {'check_rc': False}, 0, 'stdout', 'stderr'),
+                ],
+                True,
+            ),
+            (
+                # Test whether pacman --sync --list is not called more than twice
+                {"upgrade": True},
+                [
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'core foo 1.0.0-1 [installed]', ''),
+                    (["pacman", "--sync", "--refresh"], {'check_rc': False}, 0, 'stdout', 'stderr'),
+                    (["pacman", "--sync", "--list"], {'check_rc': True}, 0, 'core foo 1.0.0-1 [installed]', ''),
+                    # The following is _build_inventory:
+                    (["pacman", "--query"], {'check_rc': True}, 0, 'foo 1.0.0-1', ''),
+                    (["pacman", "--query", "--groups"], {'check_rc': True}, 0, '', ''),
+                    (["pacman", "--sync", "--groups", "--groups"], {'check_rc': True}, 0, '', ''),
+                    (["pacman", "--query", "--upgrades"], {'check_rc': False}, 0, '', ''),
+                ],
+                False,
             ),
         ],
     )
-    def test_update_db(self, mock_empty_inventory, module_args, expected_call):
+    def test_update_db(self, module_args, expected_calls, changed):
         args = {"update_cache": True}
         args.update(module_args)
         set_module_args(args)
 
-        self.mock_run_command.return_value = [0, "stdout", "stderr"]
+        self.mock_run_command.side_effect = [
+            (rc, stdout, stderr) for expected_call, kwargs, rc, stdout, stderr in expected_calls
+        ]
         with pytest.raises(AnsibleExitJson) as e:
             P = pacman.Pacman(pacman.setup_module())
             P.run()
 
-        self.mock_run_command.assert_called_with(mock.ANY, expected_call, check_rc=False)
+        self.mock_run_command.assert_has_calls([
+            mock.call(mock.ANY, expected_call, **kwargs) for expected_call, kwargs, rc, stdout, stderr in expected_calls
+        ])
         out = e.value.args[0]
-        assert out["changed"]
+        assert out["cache_updated"] == changed
+        assert out["changed"] == changed
 
     @pytest.mark.parametrize(
         "check_mode_value, run_command_data, upgrade_extra_args",