package_facts: fix warning logic (#83520)

* package_facts: fix warning logic

* Refactor so that warnings can work

changelogs/fragments/package_facts_warnings.yml

@@ -0,0 +1,3 @@
+bugfixes:
+  - package_facts - returns the correct warning when package listing fails.
+  - package_facts - no longer fails silently when the selected package manager is unable to list packages.

lib/ansible/module_utils/facts/packages.py

@@ -3,24 +3,29 @@
 
 from __future__ import annotations
 
+import ansible.module_utils.compat.typing as t
+
 from abc import ABCMeta, abstractmethod
 
 from ansible.module_utils.six import with_metaclass
+from ansible.module_utils.basic import missing_required_lib
 from ansible.module_utils.common.process import get_bin_path
+from ansible.module_utils.common.respawn import has_respawned, probe_interpreters_for_module, respawn_module
 from ansible.module_utils.common._utils import get_all_subclasses
 
 
 def get_all_pkg_managers():
 
-    return {obj.__name__.lower(): obj for obj in get_all_subclasses(PkgMgr) if obj not in (CLIMgr, LibMgr)}
+    return {obj.__name__.lower(): obj for obj in get_all_subclasses(PkgMgr) if obj not in (CLIMgr, LibMgr, RespawningLibMgr)}
 
 
 class PkgMgr(with_metaclass(ABCMeta, object)):  # type: ignore[misc]
 
     @abstractmethod
-    def is_available(self):
+    def is_available(self, handle_exceptions):
         # This method is supposed to return True/False if the package manager is currently installed/usable
         # It can also 'prep' the required systems in the process of detecting availability
+        # If handle_exceptions is false it should raise exceptions related to manager discovery instead of handling them.
         pass
 
     @abstractmethod
@@ -58,16 +63,50 @@ class LibMgr(PkgMgr):
         self._lib = None
         super(LibMgr, self).__init__()
 
-    def is_available(self):
+    def is_available(self, handle_exceptions=True):
         found = False
         try:
             self._lib = __import__(self.LIB)
             found = True
         except ImportError:
-            pass
+            if not handle_exceptions:
+                raise Exception(missing_required_lib(self.LIB))
         return found
 
 
+class RespawningLibMgr(LibMgr):
+
+    CLI_BINARIES = []   # type: t.List[str]
+    INTERPRETERS = ['/usr/bin/python3']
+
+    def is_available(self, handle_exceptions=True):
+        if super(RespawningLibMgr, self).is_available():
+            return True
+
+        for binary in self.CLI_BINARIES:
+            try:
+                bin_path = get_bin_path(binary)
+            except ValueError:
+                # Not an interesting exception to raise, just a speculative probe
+                continue
+            else:
+                # It looks like this package manager is installed
+                if not has_respawned():
+                    # See if respawning will help
+                    interpreter_path = probe_interpreters_for_module(self.INTERPRETERS, self.LIB)
+                    if interpreter_path:
+                        respawn_module(interpreter_path)
+                        # The module will exit when the respawned copy completes
+
+                if not handle_exceptions:
+                    raise Exception(f'Found executable at {bin_path}. {missing_required_lib(self.LIB)}')
+
+        if not handle_exceptions:
+            raise Exception(missing_required_lib(self.LIB))
+
+        return False
+
+
 class CLIMgr(PkgMgr):
 
     CLI = None  # type: str | None
@@ -77,9 +116,12 @@ class CLIMgr(PkgMgr):
         self._cli = None
         super(CLIMgr, self).__init__()
 
-    def is_available(self):
+    def is_available(self, handle_exceptions=True):
+        found = False
         try:
             self._cli = get_bin_path(self.CLI)
+            found = True
         except ValueError:
-            return False
+            if not handle_exceptions:
-        return True
+                raise
+        return found

lib/ansible/modules/package_facts.py

@@ -19,7 +19,7 @@ options:
       - The V(portage) and V(pkg) options were added in version 2.8.
       - The V(apk) option was added in version 2.11.
       - The V(pkg_info)' option was added in version 2.13.
-      - Aliases were added in 2.18, to support using C(auto={{ansible_facts['pkg_mgr']}})
+      - Aliases were added in 2.18, to support using C(manager={{ansible_facts['pkg_mgr']}})
     default: ['auto']
     choices:
         auto: Depending on O(strategy), will match the first or all package managers provided, in order
@@ -253,11 +253,9 @@ ansible_facts:
 import re
 
 from ansible.module_utils.common.text.converters import to_native, to_text
-from ansible.module_utils.basic import AnsibleModule, missing_required_lib
+from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.common.locale import get_best_parsable_locale
-from ansible.module_utils.common.process import get_bin_path
+from ansible.module_utils.facts.packages import CLIMgr, RespawningLibMgr, get_all_pkg_managers
-from ansible.module_utils.common.respawn import has_respawned, probe_interpreters_for_module, respawn_module
-from ansible.module_utils.facts.packages import LibMgr, CLIMgr, get_all_pkg_managers
 
 
 ALIASES = {
@@ -267,9 +265,14 @@ ALIASES = {
 }
 
 
-class RPM(LibMgr):
+class RPM(RespawningLibMgr):
 
     LIB = 'rpm'
+    CLI_BINARIES = ['rpm']
+    INTERPRETERS = [
+        '/usr/libexec/platform-python',
+        '/usr/bin/python3',
+    ]
 
     def list_installed(self):
         return self._lib.TransactionSet().dbMatch()
@@ -281,34 +284,11 @@ class RPM(LibMgr):
                     epoch=package[self._lib.RPMTAG_EPOCH],
                     arch=package[self._lib.RPMTAG_ARCH],)
 
-    def is_available(self):
-        ''' we expect the python bindings installed, but this gives warning if they are missing and we have rpm cli'''
-        we_have_lib = super(RPM, self).is_available()
-
-        try:
-            get_bin_path('rpm')
-
-            if not we_have_lib and not has_respawned():
-                # try to locate an interpreter with the necessary lib
-                interpreters = ['/usr/libexec/platform-python',
-                                '/usr/bin/python3',
-                                '/usr/bin/python2']
-                interpreter_path = probe_interpreters_for_module(interpreters, self.LIB)
-                if interpreter_path:
-                    respawn_module(interpreter_path)
-                    # end of the line for this process; this module will exit when the respawned copy completes
-
-            if not we_have_lib:
-                module.warn('Found "rpm" but %s' % (missing_required_lib(self.LIB)))
-        except ValueError:
-            pass
-
-        return we_have_lib
-
 
-class APT(LibMgr):
+class APT(RespawningLibMgr):
 
     LIB = 'apt'
+    CLI_BINARIES = ['apt', 'apt-get', 'aptitude']
 
     def __init__(self):
         self._cache = None
@@ -322,30 +302,6 @@ class APT(LibMgr):
         self._cache = self._lib.Cache()
         return self._cache
 
-    def is_available(self):
-        ''' we expect the python bindings installed, but if there is apt/apt-get give warning about missing bindings'''
-        we_have_lib = super(APT, self).is_available()
-        if not we_have_lib:
-            for exe in ('apt', 'apt-get', 'aptitude'):
-                try:
-                    get_bin_path(exe)
-                except ValueError:
-                    continue
-                else:
-                    if not has_respawned():
-                        # try to locate an interpreter with the necessary lib
-                        interpreters = ['/usr/bin/python3',
-                                        '/usr/bin/python2']
-                        interpreter_path = probe_interpreters_for_module(interpreters, self.LIB)
-                        if interpreter_path:
-                            respawn_module(interpreter_path)
-                            # end of the line for this process; this module will exit here when respawned copy completes
-
-                    module.warn('Found "%s" but %s' % (exe, missing_required_lib('apt')))
-                    break
-
-        return we_have_lib
-
     def list_installed(self):
         # Store the cache to avoid running pkg_cache() for each item in the comprehension, which is very slow
         cache = self.pkg_cache
@@ -551,22 +507,18 @@ def main():
             continue
 
         seen.add(pkgmgr)
-        try:
+
-            try:
-                # manager throws exception on init (calls self.test) if not usable.
         manager = PKG_MANAGERS[pkgmgr]()
-                if manager.is_available():
+        try:
+            if manager.is_available(handle_exceptions=False):
                 found += 1
+                try:
                     packages.update(manager.get_packages())
-
                 except Exception as e:
-                if pkgmgr in module.params['manager']:
+                    module.warn('Failed to retrieve packages with %s: %s' % (pkgmgr, to_text(e)))
-                    module.warn('Requested package manager %s was not usable by this module: %s' % (pkgmgr, to_text(e)))
-                continue
-
         except Exception as e:
             if pkgmgr in module.params['manager']:
-                module.warn('Failed to retrieve packages with %s: %s' % (pkgmgr, to_text(e)))
+                module.warn('Requested package manager %s was not usable by this module: %s' % (pkgmgr, to_text(e)))
 
     if found == 0:
         msg = ('Could not detect a supported package manager from the following list: %s, '

test/integration/targets/package_facts/aliases

@@ -1,2 +1,3 @@
+destructive
 shippable/posix/group2
 skip/macos

test/integration/targets/package_facts/files/apk

@@ -0,0 +1,3 @@
+#!/bin/sh
+
+exit 1

test/integration/targets/package_facts/runme.sh

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+
+set -eux
+
+ansible-playbook -i ../../inventory runme.yml -v "$@"
+
+ansible-playbook -i ../../inventory test_warning_unusable.yml -v "$@" 2>&1 | tee output.log
+if ! grep -q "Conditional result was False" output.log; then
+    grep "Requested package manager apk was not usable by this module" output.log
+fi
+
+ansible-playbook -i ../../inventory test_warning_failed.yml -v "$@" 2>&1 | tee output.log
+if ! grep -q "Conditional result was False" output.log; then
+    grep "Failed to retrieve packages with apk: Unable to list packages" output.log
+fi

test/integration/targets/package_facts/runme.yml

@@ -0,0 +1,4 @@
+- hosts: all
+  gather_facts: true
+  roles:
+    - { role: ../package_facts }

test/integration/targets/package_facts/test_warning_failed.yml

@@ -0,0 +1,26 @@
+- hosts: all
+  tasks:
+    - name: Check for apk
+      ansible.builtin.command: apk info
+      ignore_errors: true
+      register: apk_exists
+
+    - when: apk_exists is failed
+      block:
+        - name: Create a mock apk
+          ansible.builtin.copy:
+            dest: /usr/bin/apk
+            src: apk
+            mode: "0755"
+          become: true
+
+        - name: Elicit a warning about failing to list packages
+          ansible.builtin.package_facts:
+            manager: apk
+          failed_when: false
+
+        - name: Remove the mock
+          ansible.builtin.file:
+            dest: /usr/bin/apk
+            state: absent
+          become: true

test/integration/targets/package_facts/test_warning_unusable.yml

@@ -0,0 +1,12 @@
+- hosts: all
+  tasks:
+    - name: Check for apk
+      ansible.builtin.command: apk info
+      ignore_errors: true
+      register: apk_exists
+
+    - name: Elicit a warning about the missing binary
+      ansible.builtin.package_facts:
+        manager: apk
+      when: apk_exists is failed
+      failed_when: false