util.exe_exists and hooks._which were almost identical; merge them

certbot/hooks.py

@@ -7,6 +7,7 @@ import os
 from subprocess import Popen, PIPE
 
 from certbot import errors
+from certbot import util
 
 logger = logging.getLogger(__name__)
 
@@ -18,7 +19,7 @@ def validate_hooks(config):
 
 def _prog(shell_cmd):
     """Extract the program run by a shell command"""
-    cmd = _which(shell_cmd)
+    cmd = util.which(shell_cmd)
     return os.path.basename(cmd) if cmd else None
 
 
@@ -108,24 +109,3 @@ def execute(shell_cmd):
         logger.error('Error output from %s:\n%s', _prog(shell_cmd), err)
     return (err, out)
 
-
-def _is_exe(fpath):
-    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
-
-def _which(program):
-    """Test if program is in the path."""
-    # Borrowed from:
-    # https://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
-    # XXX May need more porting to handle .exe extensions on Windows
-
-    fpath, _fname = os.path.split(program)
-    if fpath:
-        if _is_exe(program):
-            return program
-    else:
-        for path in os.environ["PATH"].split(os.pathsep):
-            exe_file = os.path.join(path, program)
-            if _is_exe(exe_file):
-                return exe_file
-
-    return None

certbot/tests/hook_test.py

@@ -27,19 +27,8 @@ class HookTest(unittest.TestCase):
         config = mock.MagicMock(pre_hook="explodinator", post_hook="", renew_hook="")
         self.assertRaises(errors.HookCommandNotFound, hooks.validate_hooks, config)
 
-    @mock.patch('certbot.hooks._is_exe')
-    def test_which(self, mock_is_exe):
-        mock_is_exe.return_value = True
-        self.assertEqual(hooks._which("/path/to/something"), "/path/to/something")
 
-        with mock.patch.dict('os.environ', {"PATH": "/floop:/fleep"}):
-            mock_is_exe.return_value = True
-            self.assertEqual(hooks._which("pingify"), "/floop/pingify")
-            mock_is_exe.return_value = False
-            self.assertEqual(hooks._which("pingify"), None)
-        self.assertEqual(hooks._which("/path/to/something"), None)
-
-    @mock.patch('certbot.hooks._which')
+    @mock.patch('certbot.hooks.util.which')
     def test_prog(self, mockwhich):
         mockwhich.return_value = "/very/very/funky"
         self.assertEqual(hooks._prog("funky"), "funky")

certbot/tests/util_test.py

@@ -45,34 +45,19 @@ class RunScriptTest(unittest.TestCase):
         self.assertRaises(errors.SubprocessError, self._call, ["test"])
 
 
-class ExeExistsTest(unittest.TestCase):
-    """Tests for certbot.util.exe_exists."""
+class WhichTest(unittest.TestCase):
+    @mock.patch('certbot.util._is_exe')
+    def test_which(self, mock_is_exe):
+        from certbot.util import which
+        mock_is_exe.return_value = True
+        self.assertEqual(which("/path/to/something"), "/path/to/something")
 
-    @classmethod
-    def _call(cls, exe):
-        from certbot.util import exe_exists
-        return exe_exists(exe)
-
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_full_path(self, mock_access, mock_isfile):
-        mock_access.return_value = True
-        mock_isfile.return_value = True
-        self.assertTrue(self._call("/path/to/exe"))
-
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_on_path(self, mock_access, mock_isfile):
-        mock_access.return_value = True
-        mock_isfile.return_value = True
-        self.assertTrue(self._call("exe"))
-
-    @mock.patch("certbot.util.os.path.isfile")
-    @mock.patch("certbot.util.os.access")
-    def test_not_found(self, mock_access, mock_isfile):
-        mock_access.return_value = False
-        mock_isfile.return_value = True
-        self.assertFalse(self._call("exe"))
+        with mock.patch.dict('os.environ', {"PATH": "/floop:/fleep"}):
+            mock_is_exe.return_value = True
+            self.assertEqual(which("pingify"), "/floop/pingify")
+            mock_is_exe.return_value = False
+            self.assertEqual(which("pingify"), None)
+        self.assertEqual(which("/path/to/something"), None)
 
 
 class MakeOrVerifyDirTest(unittest.TestCase):

certbot/util.py

@@ -66,28 +66,36 @@ def run_script(params):
     return stdout, stderr
 
 
-def exe_exists(exe):
+def _is_exe(fpath):
+    return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
+
+def which(program):
     """Determine whether path/name refers to an executable.
 
-    :param str exe: Executable path or name
+    :param str program: Executable path or name
 
-    :returns: If exe is a valid executable
-    :rtype: bool
+    :returns: Path to executable if it exists, or None
+    :rtype: str or None
 
     """
-    def is_exe(path):
-        """Determine if path is an exe."""
-        return os.path.isfile(path) and os.access(path, os.X_OK)
+    # Borrowed from:
+    # https://stackoverflow.com/questions/377017/test-if-executable-exists-in-python
+    # XXX May need more porting to handle .exe extensions on Windows
 
-    path, _ = os.path.split(exe)
-    if path:
-        return is_exe(exe)
+    fpath, _fname = os.path.split(program)
+    if fpath:
+        if _is_exe(program):
+            return program
     else:
         for path in os.environ["PATH"].split(os.pathsep):
-            if is_exe(os.path.join(path, exe)):
-                return True
+            exe_file = os.path.join(path, program)
+            if _is_exe(exe_file):
+                return exe_file
 
-    return False
+    return None
+
+# Either name makes sense, and we have a lot of call sites!
+exe_exists = which
 
 
 def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False):