actually check we can run scm command for roles (#43315)

* actually check we can run scm command for roles
* a better error message than file not found
* more narrow exception hanlding
* refactor common functions for more extended use and further 'basic.py' separation

lib/ansible/module_utils/basic.py

@@ -160,6 +160,8 @@ from ansible.module_utils.common._collections_compat import (
     Sequence, MutableSequence,
     Set, MutableSet,
 )
+from ansible.module_utils.common.process import get_bin_path
+from ansible.module_utils.common.file import is_executable
 from ansible.module_utils.pycompat24 import get_exception, literal_eval
 from ansible.module_utils.six import (
     PY2,
@@ -670,20 +672,6 @@ def human_to_bytes(number, default_unit=None, isbits=False):
     return int(round(num * limit))
 
 
-def is_executable(path):
-    '''is the given path executable?
-
-    Limitations:
-    * Does not account for FSACLs.
-    * Most times we really want to know "Can the current user execute this
-      file"  This function does not tell us that, only if an execute bit is set.
-    '''
-    # These are all bitfields so first bitwise-or all the permissions we're
-    # looking for, then bitwise-and with the file's mode to determine if any
-    # execute bits are set.
-    return ((stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) & os.stat(path)[stat.ST_MODE])
-
-
 def _load_params():
     ''' read the modules parameters and store them globally.
 
@@ -2287,28 +2275,13 @@ class AnsibleModule(object):
            - opt_dirs:  optional list of directories to search in addition to PATH
         if found return full path; otherwise return None
         '''
-        opt_dirs = [] if opt_dirs is None else opt_dirs
 
-        sbin_paths = ['/sbin', '/usr/sbin', '/usr/local/sbin']
-        paths = []
-        for d in opt_dirs:
-            if d is not None and os.path.exists(d):
-                paths.append(d)
-        paths += os.environ.get('PATH', '').split(os.pathsep)
         bin_path = None
-        # mangle PATH to include /sbin dirs
+        try:
-        for p in sbin_paths:
+            bin_path = get_bin_path(arg, required, opt_dirs)
-            if p not in paths and os.path.exists(p):
+        except ValueError as e:
-                paths.append(p)
+            self.fail_json(msg=to_text(e))
-        for d in paths:
+
-            if not d:
-                continue
-            path = os.path.join(d, arg)
-            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:
-            self.fail_json(msg='Failed to find required executable %s in paths: %s' % (arg, os.pathsep.join(paths)))
         return bin_path
 
     def boolean(self, arg):

lib/ansible/module_utils/common/file.py

@@ -32,6 +32,20 @@ class LockTimeout(Exception):
     pass
 
 
+def is_executable(path):
+    '''is the given path executable?
+
+    Limitations:
+    * Does not account for FSACLs.
+    * Most times we really want to know "Can the current user execute this
+      file"  This function does not tell us that, only if an execute bit is set.
+    '''
+    # These are all bitfields so first bitwise-or all the permissions we're
+    # looking for, then bitwise-and with the file's mode to determine if any
+    # execute bits are set.
+    return ((stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) & os.stat(path)[stat.ST_MODE])
+
+
 class FileLock:
     '''
     Currently FileLock is implemented via fcntl.flock on a lock file, however this

lib/ansible/module_utils/common/process.py

@@ -0,0 +1,43 @@
+# Copyright (c) 2018, Ansible Project
+# Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause)
+
+from __future__ import (absolute_import, division, print_function)
+__metaclass__ = type
+
+import os
+
+from ansible.module_utils.common.file import is_executable
+
+
+def get_bin_path(arg, required=False, opt_dirs=None):
+    '''
+    find system executable in PATH.
+    Optional arguments:
+       - required:  if executable is not found and required is true it produces an Exception
+       - opt_dirs:  optional list of directories to search in addition to PATH
+    if found return full path; otherwise return None
+    '''
+    opt_dirs = [] if opt_dirs is None else opt_dirs
+
+    sbin_paths = ['/sbin', '/usr/sbin', '/usr/local/sbin']
+    paths = []
+    for d in opt_dirs:
+        if d is not None and os.path.exists(d):
+            paths.append(d)
+    paths += os.environ.get('PATH', '').split(os.pathsep)
+    bin_path = None
+    # mangle PATH to include /sbin dirs
+    for p in sbin_paths:
+        if p not in paths and os.path.exists(p):
+            paths.append(p)
+    for d in paths:
+        if not d:
+            continue
+        path = os.path.join(d, arg)
+        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:
+        raise ValueError('Failed to find required executable %s in paths: %s' % (arg, os.pathsep.join(paths)))
+
+    return bin_path

lib/ansible/playbook/role/requirement.py

@@ -28,6 +28,7 @@ from subprocess import Popen, PIPE
 from ansible import constants as C
 from ansible.errors import AnsibleError
 from ansible.module_utils._text import to_native
+from ansible.module_utils.common.process import get_bin_path
 from ansible.module_utils.six import string_types
 from ansible.playbook.role.definition import RoleDefinition
 
@@ -203,12 +204,17 @@ class RoleRequirement(RoleDefinition):
         if scm not in ['hg', 'git']:
             raise AnsibleError("- scm %s is not currently supported" % scm)
 
+        try:
+            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))
+
         tempdir = tempfile.mkdtemp(dir=C.DEFAULT_LOCAL_TMP)
-        clone_cmd = [scm, 'clone', src, name]
+        clone_cmd = [scm_path, 'clone', src, name]
         run_scm_cmd(clone_cmd, tempdir)
 
         if scm == 'git' and version:
-            checkout_cmd = [scm, 'checkout', version]
+            checkout_cmd = [scm_path, 'checkout', version]
             run_scm_cmd(checkout_cmd, os.path.join(tempdir, name))
 
         temp_file = tempfile.NamedTemporaryFile(delete=False, suffix='.tar', dir=C.DEFAULT_LOCAL_TMP)
@@ -218,12 +224,12 @@ class RoleRequirement(RoleDefinition):
             with tarfile.open(temp_file.name, "w") as tar:
                 tar.add(os.path.join(tempdir, name), arcname=name)
         elif scm == 'hg':
-            archive_cmd = ['hg', 'archive', '--prefix', "%s/" % name]
+            archive_cmd = [scm_path, 'archive', '--prefix', "%s/" % name]
             if version:
                 archive_cmd.extend(['-r', version])
             archive_cmd.append(temp_file.name)
         elif scm == 'git':
-            archive_cmd = ['git', 'archive', '--prefix=%s/' % name, '--output=%s' % temp_file.name]
+            archive_cmd = [scm_path, 'archive', '--prefix=%s/' % name, '--output=%s' % temp_file.name]
             if version:
                 archive_cmd.append(version)
             else: