Re-implemented salt.utils.path.which to properly define executable semantics on both the windows and posix platforms.

salt/utils/path.py

@@ -20,7 +20,6 @@
 import salt.utils.platform
 import salt.utils.stringutils
 from salt.exceptions import CommandNotFoundError
-from salt.utils.decorators import memoize as real_memoize
 from salt.utils.decorators.jinja import jinja_filter
 
 # Import 3rd-party libs
@@ -191,70 +190,110 @@ def which(exe=None):
     '''
     Python clone of /usr/bin/which
     '''
-    def _is_executable_file_or_link(exe):
-        # check for os.X_OK doesn't suffice because directory may executable
-        return (os.access(exe, os.X_OK) and
-                (os.path.isfile(exe) or os.path.islink(exe)))
-
-    if exe:
-        if _is_executable_file_or_link(exe):
-            # executable in cwd or fullpath
-            return exe
-
-        ext_list = salt.utils.stringutils.to_str(
-            os.environ.get('PATHEXT', str('.EXE'))
-        ).split(str(';'))
-
-        @real_memoize
-        def _exe_has_ext():
-            '''
-            Do a case insensitive test if exe has a file extension match in
-            PATHEXT
-            '''
-            for ext in ext_list:
-                try:
-                    pattern = r'.*\.{0}$'.format(
-                        salt.utils.stringutils.to_unicode(ext).lstrip('.')
-                    )
-                    re.match(
-                        pattern,
-                        salt.utils.stringutils.to_unicode(exe),
-                        re.I).groups()
-                    return True
-                except AttributeError:
-                    continue
-            return False
-
-        # Enhance POSIX path for the reliability at some environments, when $PATH is changing
-        # This also keeps order, where 'first came, first win' for cases to find optional alternatives
-        system_path = salt.utils.stringutils.to_unicode(os.environ.get('PATH', ''))
-        search_path = system_path.split(os.pathsep)
-        if not salt.utils.platform.is_windows():
-            search_path.extend([
-                x for x in ('/bin', '/sbin', '/usr/bin',
-                            '/usr/sbin', '/usr/local/bin')
-                if x not in search_path
-            ])
-
-        for path in search_path:
-            full_path = join(path, exe)
-            if _is_executable_file_or_link(full_path):
-                return full_path
-            elif salt.utils.platform.is_windows() and not _exe_has_ext():
-                # On Windows, check for any extensions in PATHEXT.
-                # Allows both 'cmd' and 'cmd.exe' to be matched.
-                for ext in ext_list:
-                    # Windows filesystem is case insensitive so we
-                    # safely rely on that behavior
-                    if _is_executable_file_or_link(full_path + ext):
-                        return full_path + ext
-        log.trace(
-            '\'%s\' could not be found in the following search path: \'%s\'',
-            exe, search_path
-        )
-    else:
+
+    if not exe:
         log.error('No executable was passed to be searched by salt.utils.path.which()')
+        return None
+
+    ## define some utilities (we use closures here because our predecessor used them)
+    def is_executable_common(path):
+        '''
+        This returns truth if posixy semantics (which python simulates on
+        windows) states that this is executable.
+        '''
+        return os.path.isfile(path) and os.access(path, os.X_OK)
+
+    def resolve(path):
+        '''
+        This will take a path and recursively follow the link until we get to a
+        real file.
+        '''
+        while os.path.islink(path):
+            res = os.readlink(path)
+
+            # if the link points to a relative target, then convert it to an
+            # absolute path relative to the original path
+            if not os.path.isabs(res):
+                directory, _ = os.path.split(path)
+                res = join(directory, res)
+            path = res
+        return path
+
+    # windows-only
+    def has_executable_ext(path, ext_membership):
+        '''
+        Extract the extension from the specified path, lowercase it so we
+        can be insensitive, and then check it against the available exts.
+        '''
+        p, ext = os.path.splitext(path)
+        return ext.lower() in ext_membership
+
+    ## prepare related variables from the environment
+    res = salt.utils.stringutils.to_unicode(os.environ.get('PATH', ''))
+    system_path = res.split(os.pathsep)
+
+    # add some reasonable defaults in case someone's PATH is busted
+    if not salt.utils.platform.is_windows():
+        res = set(system_path)
+        extended_path = ['/sbin', '/bin', '/usr/sbin', '/usr/bin', '/usr/local/sbin', '/usr/local/bin']
+        system_path.extend([p for p in extended_path if p not in res])
+
+    ## now to define the semantics of what's considered executable on a given platform
+    if salt.utils.platform.is_windows():
+        # executable semantics on windows requires us to search PATHEXT
+        res = salt.utils.stringutils.to_str(os.environ.get('PATHEXT', str('.EXE')))
+
+        # generate two variables, one of them for O(n) searches (but ordered)
+        # and another for O(1) searches. the previous guy was trying to use
+        # memoization with a function that has no arguments, this provides
+        # the exact same benefit
+        pathext = res.split(os.pathsep)
+        res = {ext.lower() for ext in pathext}
+
+        # check if our caller already specified a valid extension as then we don't need to match it
+        _, ext = os.path.splitext(exe)
+        if ext.lower() in res:
+            pathext = ['']
+
+            is_executable = is_executable_common
+
+        # The specified extension isn't valid, so we just assume it's part of the
+        # filename and proceed to walk the pathext list
+        else:
+            is_executable = lambda path, membership=res: is_executable_common(path) and has_executable_ext(path, membership)
+
+    else:
+        # in posix, there's no such thing as file extensions..only zuul
+        pathext = ['']
+
+        # executable semantics are pretty simple on reasonable platforms...
+        is_executable = is_executable_common
+
+    ## search for the executable
+
+    # check to see if the full path was specified as then we don't need
+    # to actually walk the system_path for any reason
+    if is_executable(exe):
+        return exe
 
+    # now to search through our system_path
+    for path in system_path:
+        p = join(path, exe)
+
+        # iterate through all extensions to see which one is executable
+        for ext in pathext:
+            pext = p + ext
+            rp = resolve(pext)
+            if is_executable(rp):
+                return p + ext
+            continue
+        continue
+
+    ## if something was executable, we should've found it already...
+    log.trace(
+        '\'%s\' could not be found in the following search path: \'%s\'',
+        exe, system_path
+    )
     return None
 
 

tests/unit/utils/test_path.py

@@ -186,92 +186,108 @@ class TestWhich(TestCase):
     # The mock patch below will make sure that ALL calls to the which function
     # returns None
     def test_missing_binary_in_linux(self):
-        with patch('salt.utils.path.which', lambda exe: None):
-            self.assertTrue(
-                salt.utils.path.which('this-binary-does-not-exist') is None
-            )
+        # salt.utils.path.which uses platform.is_windows to determine the platform, so we're using linux here
+        with patch('salt.utils.platform.is_windows', lambda: False):
+            with patch('salt.utils.path.which', lambda exe: None):
+                self.assertTrue(
+                    salt.utils.path.which('this-binary-does-not-exist') is None
+                )
 
     # The mock patch below will make sure that ALL calls to the which function
     # return whatever is sent to it
     def test_existing_binary_in_linux(self):
-        with patch('salt.utils.path.which', lambda exe: exe):
-            self.assertTrue(salt.utils.path.which('this-binary-exists-under-linux'))
+        # salt.utils.path.which uses platform.is_windows to determine the platform, so we're using linux here
+        with patch('salt.utils.platform.is_windows', lambda: False):
+            with patch('salt.utils.path.which', lambda exe: exe):
+                self.assertTrue(salt.utils.path.which('this-binary-exists-under-linux'))
 
     def test_existing_binary_in_windows(self):
-        with patch('os.access') as osaccess:
+        with patch('os.path.isfile') as isfile:
             # We define the side_effect attribute on the mocked object in order to
-            # specify which calls return which values. First call to os.access
+            # specify which calls return which values. First call to os.path.isfile
             # returns X, the second Y, the third Z, etc...
-            osaccess.side_effect = [
-                # The first os.access should return False(the abspath one)
-                False,
-                # The second, iterating through $PATH, should also return False,
-                # still checking for Linux
+            isfile.side_effect = [
+                # The first os.path.isfile should return False due to checking the explicit path (first is_executable)
                 False,
                 # We will now also return False once so we get a .EXE back from
                 # the function, see PATHEXT below.
                 False,
                 # Lastly return True, this is the windows check.
                 True
             ]
-            # Let's patch os.environ to provide a custom PATH variable
-            with patch.dict(os.environ, {'PATH': os.sep + 'bin',
-                                         'PATHEXT': '.COM;.EXE;.BAT;.CMD'}):
-                # Let's also patch is_windows to return True
-                with patch('salt.utils.platform.is_windows', lambda: True):
-                    with patch('os.path.isfile', lambda x: True):
-                        self.assertEqual(
-                            salt.utils.path.which('this-binary-exists-under-windows'),
-                            os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.EXE')
-                        )
+
+            # Patch os.access so that it always returns True
+            with patch('os.access', lambda path, mode: True):
+                # Disable os.path.islink
+                with patch('os.path.islink', lambda path: False):
+                    # we're using ';' as os.pathsep in this test
+                    with patch('os.pathsep', ';'):
+                        # Let's patch os.environ to provide a custom PATH variable
+                        with patch.dict(os.environ, {'PATH': os.sep + 'bin',
+                                                     'PATHEXT': '.COM;.EXE;.BAT;.CMD'}):
+                            # Let's also patch is_windows to return True
+                            with patch('salt.utils.platform.is_windows', lambda: True):
+                                self.assertEqual(
+                                    salt.utils.path.which('this-binary-exists-under-windows'),
+                                    os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.EXE')
+                                )
 
     def test_missing_binary_in_windows(self):
         with patch('os.access') as osaccess:
             osaccess.side_effect = [
-                # The first os.access should return False(the abspath one)
+                # The first os.access should return False due to checking the explicit path (first is_executable)
                 False,
                 # The second, iterating through $PATH, should also return False,
                 # still checking for Linux
                 # which() will add 4 extra paths to the given one, os.access will
                 # be called 5 times
                 False, False, False, False, False
             ]
-            # Let's patch os.environ to provide a custom PATH variable
-            with patch.dict(os.environ, {'PATH': os.sep + 'bin'}):
-                # Let's also patch is_widows to return True
-                with patch('salt.utils.platform.is_windows', lambda: True):
-                    self.assertEqual(
-                        # Since we're passing the .exe suffix, the last True above
-                        # will not matter. The result will be None
-                        salt.utils.path.which('this-binary-is-missing-in-windows.exe'),
-                        None
-                    )
+            # we're using ';' as os.pathsep in this test
+            with patch('os.pathsep', ';'):
+                # Let's patch os.environ to provide a custom PATH variable
+                with patch.dict(os.environ, {'PATH': os.sep + 'bin'}):
+                    # Let's also patch is_widows to return True
+                    with patch('salt.utils.platform.is_windows', lambda: True):
+                        self.assertEqual(
+                            # Since we're passing the .exe suffix, the last True above
+                            # will not matter. The result will be None
+                            salt.utils.path.which('this-binary-is-missing-in-windows.exe'),
+                            None
+                        )
 
     def test_existing_binary_in_windows_pathext(self):
-        with patch('os.access') as osaccess:
+        with patch('os.path.isfile') as isfile:
             # We define the side_effect attribute on the mocked object in order to
-            # specify which calls return which values. First call to os.access
+            # specify which calls return which values. First call to os.path.isfile
             # returns X, the second Y, the third Z, etc...
-            osaccess.side_effect = [
-                # The first os.access should return False(the abspath one)
-                False,
-                # The second, iterating through $PATH, should also return False,
-                # still checking for Linux
+            isfile.side_effect = [
+                # The first os.path.isfile should return False due to checking the explicit path (first is_executable)
                 False,
                 # We will now also return False 3 times so we get a .CMD back from
                 # the function, see PATHEXT below.
                 # Lastly return True, this is the windows check.
                 False, False, False,
                 True
             ]
-            # Let's patch os.environ to provide a custom PATH variable
-            with patch.dict(os.environ, {'PATH': os.sep + 'bin',
-                                         'PATHEXT': '.COM;.EXE;.BAT;.CMD;.VBS;'
-                                         '.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.PY'}):
-                # Let's also patch is_windows to return True
-                with patch('salt.utils.platform.is_windows', lambda: True):
-                    with patch('os.path.isfile', lambda x: True):
-                        self.assertEqual(
-                            salt.utils.path.which('this-binary-exists-under-windows'),
-                            os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.CMD')
-                        )
+
+            # Patch os.access so that it always returns True
+            with patch('os.access', lambda path, mode: True):
+
+                # Disable os.path.islink
+                with patch('os.path.islink', lambda path: False):
+
+                    # we're using ';' as os.pathsep in this test
+                    with patch('os.pathsep', ';'):
+
+                        # Let's patch os.environ to provide a custom PATH variable
+                        with patch.dict(os.environ, {'PATH': os.sep + 'bin',
+                                                     'PATHEXT': '.COM;.EXE;.BAT;.CMD;.VBS;'
+                                                     '.VBE;.JS;.JSE;.WSF;.WSH;.MSC;.PY'}):
+
+                            # Let's also patch is_windows to return True
+                            with patch('salt.utils.platform.is_windows', lambda: True):
+                                self.assertEqual(
+                                    salt.utils.path.which('this-binary-exists-under-windows'),
+                                    os.path.join(os.sep + 'bin', 'this-binary-exists-under-windows.CMD')
+                                )