Make get_bin_path() always raise an exception (ansible#56813)

This makes it behave in a more idiomatic way

* Fix bug in Darwin facts for free memory
    If the vm_stat command is not found, fact gathering would fail with an unhelpful 
    error message. Handle this gracefully and return a default value for free memory.

* Add unit tests

changelogs/fragments/change-get_bin_path-always-raise-exception.yaml

@@ -0,0 +1,2 @@
+minor_changes:
+  - get_bin_path() - change the interface to always raise ``ValueError`` if the command is not found (https://github.com/ansible/ansible/pull/56813)

lib/ansible/module_utils/basic.py

@@ -1973,9 +1973,12 @@ def get_bin_path(self, arg, required=False, opt_dirs=None):
 
         bin_path = None
         try:
-            bin_path = get_bin_path(arg, required, opt_dirs)
+            bin_path = get_bin_path(arg=arg, opt_dirs=opt_dirs)
         except ValueError as e:
-            self.fail_json(msg=to_text(e))
+            if required:
+                self.fail_json(msg=to_text(e))
+            else:
+                return bin_path
 
         return bin_path
 

lib/ansible/module_utils/common/process.py

@@ -9,13 +9,14 @@
 from ansible.module_utils.common.file import is_executable
 
 
-def get_bin_path(arg, required=False, opt_dirs=None):
+def get_bin_path(arg, opt_dirs=None, required=None):
     '''
-    find system executable in PATH.
+    Find system executable in PATH. Raises ValueError if executable is not found.
     Optional arguments:
-       - required:  if executable is not found and required is true it produces an Exception
+       - required:  [Deprecated] Prior to 2.10, if executable is not found and required is true it raises an Exception.
+                    In 2.10 and later, an Exception is always raised. This parameter will be removed in 2.14.
        - opt_dirs:  optional list of directories to search in addition to PATH
-    if found return full path; otherwise return None
+    If found return full path, otherwise raise ValueError.
     '''
     opt_dirs = [] if opt_dirs is None else opt_dirs
 
@@ -37,7 +38,7 @@ def get_bin_path(arg, required=False, opt_dirs=None):
         if os.path.exists(path) and not os.path.isdir(path) and is_executable(path):
             bin_path = path
             break
-    if required and bin_path is None:
+    if bin_path is None:
         raise ValueError('Failed to find required executable %s in paths: %s' % (arg, os.pathsep.join(paths)))
 
     return bin_path

lib/ansible/module_utils/facts/hardware/darwin.py

@@ -85,12 +85,17 @@ def get_cpu_facts(self):
 
     def get_memory_facts(self):
         memory_facts = {
-            'memtotal_mb': int(self.sysctl['hw.memsize']) // 1024 // 1024
+            'memtotal_mb': int(self.sysctl['hw.memsize']) // 1024 // 1024,
+            'memfree_mb': 0,
         }
 
         total_used = 0
         page_size = 4096
-        vm_stat_command = get_bin_path('vm_stat')
+        try:
+            vm_stat_command = get_bin_path('vm_stat')
+        except ValueError:
+            return memory_facts
+
         rc, out, err = self.module.run_command(vm_stat_command)
         if rc == 0:
             # Free = Total - (Wired + active + inactive)
@@ -104,7 +109,7 @@ def get_memory_facts(self):
             for k, v in memory_stats.items():
                 try:
                     memory_stats[k] = int(v)
-                except ValueError as ve:
+                except ValueError:
                     # Most values convert cleanly to integer values but if the field does
                     # not convert to an integer, just leave it alone.
                     pass
@@ -116,7 +121,7 @@ def get_memory_facts(self):
             if memory_stats.get('Pages inactive'):
                 total_used += memory_stats['Pages inactive'] * page_size
 
-        memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024)
+            memory_facts['memfree_mb'] = memory_facts['memtotal_mb'] - (total_used // 1024 // 1024)
 
         return memory_facts
 

lib/ansible/module_utils/facts/network/iscsi.py

@@ -80,22 +80,30 @@ def collect(self, module=None, collected_facts=None):
                     iscsi_facts['iscsi_iqn'] = line.split('=', 1)[1]
                     break
         elif sys.platform.startswith('aix'):
-            cmd = get_bin_path('lsattr')
-            if cmd:
-                cmd += " -E -l iscsi0"
-                rc, out, err = module.run_command(cmd)
-                if rc == 0 and out:
-                    line = self.findstr(out, 'initiator_name')
-                    iscsi_facts['iscsi_iqn'] = line.split()[1].rstrip()
+            try:
+                cmd = get_bin_path('lsattr')
+            except ValueError:
+                return iscsi_facts
+
+            cmd += " -E -l iscsi0"
+            rc, out, err = module.run_command(cmd)
+            if rc == 0 and out:
+                line = self.findstr(out, 'initiator_name')
+                iscsi_facts['iscsi_iqn'] = line.split()[1].rstrip()
+
         elif sys.platform.startswith('hp-ux'):
             # try to find it in the default PATH and opt_dirs
-            cmd = get_bin_path('iscsiutil', opt_dirs=['/opt/iscsi/bin'])
-            if cmd:
-                cmd += " -l"
-                rc, out, err = module.run_command(cmd)
-                if out:
-                    line = self.findstr(out, 'Initiator Name')
-                    iscsi_facts['iscsi_iqn'] = line.split(":", 1)[1].rstrip()
+            try:
+                cmd = get_bin_path('iscsiutil', opt_dirs=['/opt/iscsi/bin'])
+            except ValueError:
+                return iscsi_facts
+
+            cmd += " -l"
+            rc, out, err = module.run_command(cmd)
+            if out:
+                line = self.findstr(out, 'Initiator Name')
+                iscsi_facts['iscsi_iqn'] = line.split(":", 1)[1].rstrip()
+
         return iscsi_facts
 
     def findstr(self, text, match):

lib/ansible/module_utils/facts/packages.py

@@ -79,5 +79,8 @@ def __init__(self):
         super(CLIMgr, self).__init__()
 
     def is_available(self):
-        self._cli = get_bin_path(self.CLI, False)
-        return bool(self._cli)
+        try:
+            self._cli = get_bin_path(self.CLI)
+        except ValueError:
+            return False
+        return True

lib/ansible/modules/files/copy.py

@@ -291,7 +291,7 @@ def __init__(self, results):
 # basic::AnsibleModule() until then but if so, make it a private function so that we don't have to
 # keep it for backwards compatibility later.
 def clear_facls(path):
-    setfacl = get_bin_path('setfacl', True)
+    setfacl = get_bin_path('setfacl')
     # FIXME "setfacl -b" is available on Linux and FreeBSD. There is "setfacl -D e" on z/OS. Others?
     acl_command = [setfacl, '-b', path]
     b_acl_command = [to_bytes(x) for x in acl_command]

lib/ansible/modules/packaging/os/package_facts.py

@@ -229,8 +229,14 @@ def get_package_details(self, package):
     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()
-        if not we_have_lib and get_bin_path('rpm'):
-            module.warn('Found "rpm" but %s' % (missing_required_lib('rpm')))
+
+        try:
+            get_bin_path('rpm')
+            if not we_have_lib:
+                module.warn('Found "rpm" but %s' % (missing_required_lib('rpm')))
+        except ValueError:
+            pass
+
         return we_have_lib
 
 
@@ -255,7 +261,11 @@ def is_available(self):
         we_have_lib = super(APT, self).is_available()
         if not we_have_lib:
             for exe in ('apt', 'apt-get', 'aptitude'):
-                if get_bin_path(exe):
+                try:
+                    get_bin_path(exe)
+                except ValueError:
+                    continue
+                else:
                     module.warn('Found "%s" but %s' % (exe, missing_required_lib('apt')))
                     break
         return we_have_lib

lib/ansible/playbook/role/requirement.py

@@ -155,7 +155,7 @@ def run_scm_cmd(cmd, tempdir):
             raise AnsibleError("- scm %s is not currently supported" % scm)
 
         try:
-            scm_path = get_bin_path(scm, required=True)
+            scm_path = get_bin_path(scm)
         except (ValueError, OSError, IOError):
             raise AnsibleError("could not find/use %s, it is required to continue with installing %s" % (scm, src))
 

lib/ansible/plugins/connection/chroot.py

@@ -101,10 +101,10 @@ def _connect(self):
         if os.path.isabs(self.get_option('chroot_exe')):
             self.chroot_cmd = self.get_option('chroot_exe')
         else:
-            self.chroot_cmd = get_bin_path(self.get_option('chroot_exe'))
-
-        if not self.chroot_cmd:
-            raise AnsibleError("chroot command (%s) not found in PATH" % to_native(self.get_option('chroot_exe')))
+            try:
+                self.chroot_cmd = get_bin_path(self.get_option('chroot_exe'))
+            except ValueError as e:
+                raise AnsibleError(to_native(e))
 
         super(Connection, self)._connect()
         if not self._connected:

lib/ansible/plugins/inventory/docker_machine.py

@@ -108,9 +108,9 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable):
     def _run_command(self, args):
         if not self.DOCKER_MACHINE_PATH:
             try:
-                self.DOCKER_MACHINE_PATH = get_bin_path('docker-machine', required=True)
+                self.DOCKER_MACHINE_PATH = get_bin_path('docker-machine')
             except ValueError as e:
-                raise AnsibleError('Unable to locate the docker-machine binary.', orig_exc=e)
+                raise AnsibleError(to_native(e))
 
         command = [self.DOCKER_MACHINE_PATH]
         command.extend(args)

lib/ansible/plugins/inventory/nmap.py

@@ -86,12 +86,9 @@ def verify_file(self, path):
     def parse(self, inventory, loader, path, cache=False):
 
         try:
-            self._nmap = get_bin_path('nmap', True)
+            self._nmap = get_bin_path('nmap')
         except ValueError as e:
-            raise AnsibleParserError(e)
-
-        if self._nmap is None:
-            raise AnsibleParserError('nmap inventory plugin requires the nmap cli tool to work')
+            raise AnsibleParserError('nmap inventory plugin requires the nmap cli tool to work: {0}'.format(to_native(e)))
 
         super(InventoryModule, self).parse(inventory, loader, path, cache=cache)
 

lib/ansible/plugins/inventory/virtualbox.py

@@ -229,7 +229,7 @@ def verify_file(self, path):
     def parse(self, inventory, loader, path, cache=True):
 
         try:
-            self._vbox_path = get_bin_path(self.VBOX, True)
+            self._vbox_path = get_bin_path(self.VBOX)
         except ValueError as e:
             raise AnsibleParserError(e)
 

test/units/module_utils/common/process/test_get_bin_path.py

@@ -0,0 +1,33 @@
+# -*- coding: utf-8 -*-
+# Copyright (c) 2020 Ansible Project
+# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
+
+from __future__ import absolute_import, division, print_function
+__metaclass__ = type
+
+import pytest
+
+from ansible.module_utils.common.process import get_bin_path
+
+
+def test_get_bin_path(mocker):
+    path = '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin'
+    mocker.patch.dict('os.environ', {'PATH': path})
+    mocker.patch('os.pathsep', ':')
+
+    mocker.patch('os.path.exists', side_effect=[False, True])
+    mocker.patch('os.path.isdir', return_value=False)
+    mocker.patch('ansible.module_utils.common.process.is_executable', return_value=True)
+
+    assert '/usr/local/bin/notacommand' == get_bin_path('notacommand')
+
+
+def test_get_path_path_raise_valueerror(mocker):
+    mocker.patch.dict('os.environ', {'PATH': ''})
+
+    mocker.patch('os.path.exists', return_value=False)
+    mocker.patch('os.path.isdir', return_value=False)
+    mocker.patch('ansible.module_utils.common.process.is_executable', return_value=True)
+
+    with pytest.raises(ValueError, match='Failed to find required executable notacommand'):
+        get_bin_path('notacommand')