Prefer vim to vi for default editor (#4230)

* vim > vi
* Allow which to accept multiple args
* Update __init__ to use which with multiple args
* Fix doc tests

7 files changed, 97 insertions, 85 deletions

diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py
index 057c54d665..214bb09878 100644
--- a/lib/spack/spack/__init__.py
+++ b/lib/spack/spack/__init__.py
@@ -213,7 +213,22 @@ from spack.util.executable import *
 __all__ += spack.util.executable.__all__
 
 # User's editor from the environment
-editor = Executable(os.environ.get("EDITOR", "vi"))
+
+# Default editors to use:
+_default_editors = ['vim', 'vi', 'emacs', 'nano']
+
+# The EDITOR environment variable has the highest precedence
+if os.environ.get('EDITOR'):
+    _default_editors.insert(0, os.environ.get('EDITOR'))
+
+editor = which(*_default_editors)
+
+if not editor:
+    default = default_editors[0]
+    msg  = 'Default text editor, {0}, not found.\n'.format(default)
+    msg += 'Please set the EDITOR environment variable to your preferred '
+    msg += 'text editor, or install {0}.'.format(default)
+    raise EnvironmentError(msg)
 
 from spack.package import \
     install_dependency_symlinks, flatten_dependencies, \
diff --git a/lib/spack/spack/abi.py b/lib/spack/spack/abi.py
index ad3cae6ee2..401b99cf71 100644
--- a/lib/spack/spack/abi.py
+++ b/lib/spack/spack/abi.py
@@ -69,8 +69,7 @@ class ABI(object):
             if Clang.default_version(rungcc.exe[0]) != 'unknown':
                 return None
 
-            output = rungcc("--print-file-name=%s" % libname,
-                            return_output=True)
+            output = rungcc("--print-file-name=%s" % libname, output=str)
         except ProcessError:
             return None
         if not output:
diff --git a/lib/spack/spack/package_test.py b/lib/spack/spack/package_test.py
index 54f424d45e..fa424287f3 100644
--- a/lib/spack/spack/package_test.py
+++ b/lib/spack/spack/package_test.py
@@ -39,7 +39,7 @@ def compile_c_and_execute(source_file, include_flags, link_flags):
        *link_flags)
 
     check = Executable('./check')
-    return check(return_output=True)
+    return check(output=str)
 
 
 def compare_output(current_output, blessed_output):
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index c7e445971b..0aec84b1d0 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -46,9 +46,16 @@ class Executable(object):
             raise ProcessError("Cannot construct executable for '%s'" % name)
 
     def add_default_arg(self, arg):
+        """Add a default argument to the command."""
         self.exe.append(arg)
 
     def add_default_env(self, key, value):
+        """Set an environment variable when the command is run.
+
+        Parameters:
+            key: The environment variable to set
+            value: The value to set it to
+        """
         self.default_env[key] = value
 
     @property
@@ -81,55 +88,34 @@ class Executable(object):
     def __call__(self, *args, **kwargs):
         """Run this executable in a subprocess.
 
-        Arguments
-          args
-            command line arguments to the executable to run.
-
-        Optional arguments
-
-          fail_on_error
-
-            Raise an exception if the subprocess returns an
-            error. Default is True.  When not set, the return code is
-            available as `exe.returncode`.
-
-          ignore_errors
-
-            An optional list/tuple of error codes that can be
-            *ignored*.  i.e., if these codes are returned, this will
-            not raise an exception when `fail_on_error` is `True`.
-
-          output, error
-
-            These arguments allow you to specify new stdout and stderr
-            values.  They default to `None`, which means the
-            subprocess will inherit the parent's file descriptors.
-
-            You can set these to:
-            - python streams, e.g. open Python file objects, or os.devnull;
-            - filenames, which will be automatically opened for writing; or
-            - `str`, as in the Python string type. If you set these to `str`,
-               output and error will be written to pipes and returned as
-               a string.  If both `output` and `error` are set to `str`,
-               then one string is returned containing output concatenated
-               with error.
-
-          input
-
-            Same as output, error, but `str` is not an allowed value.
-
-        Deprecated arguments
-
-          return_output[=False]
-
-            Setting this to True is the same as setting output=str.
-            This argument may be removed in future Spack versions.
-
+        Parameters:
+            *args (str): Command-line arguments to the executable to run
+
+        Keyword Arguments:
+            env (dict): The environment to run the executable with
+            fail_on_error (bool): Raise an exception if the subprocess returns
+                an error. Default is True. The return code is available as
+                ``exe.returncode``
+            ignore_errors (int or list): A list of error codes to ignore.
+                If these codes are returned, this process will not raise
+                an exception even if ``fail_on_error`` is set to ``True``
+            input: Where to read stdin from
+            output: Where to send stdout
+            error: Where to send stderr
+
+        Accepted values for input, output, and error:
+
+        * python streams, e.g. open Python file objects, or ``os.devnull``
+        * filenames, which will be automatically opened for writing
+        * ``str``, as in the Python string type. If you set these to ``str``,
+          output and error will be written to pipes and returned as a string.
+          If both ``output`` and ``error`` are set to ``str``, then one string
+          is returned containing output concatenated with error. Not valid
+          for ``input``
+
+        By default, the subprocess inherits the parent's file descriptors.
         """
-        fail_on_error = kwargs.pop("fail_on_error", True)
-        ignore_errors = kwargs.pop("ignore_errors", ())
-
-        # environment
+        # Environment
         env_arg = kwargs.get('env', None)
         if env_arg is None:
             env = os.environ.copy()
@@ -138,19 +124,19 @@ class Executable(object):
             env = self.default_env.copy()
             env.update(env_arg)
 
-        # TODO: This is deprecated.  Remove in a future version.
-        return_output = kwargs.pop("return_output", False)
+        fail_on_error = kwargs.pop('fail_on_error', True)
+        ignore_errors = kwargs.pop('ignore_errors', ())
 
-        # Default values of None says to keep parent's file descriptors.
-        if return_output:
-            output = str
-        else:
-            output = kwargs.pop("output", None)
+        # If they just want to ignore one error code, make it a tuple.
+        if isinstance(ignore_errors, int):
+            ignore_errors = (ignore_errors, )
+
+        input  = kwargs.pop('input',  None)
+        output = kwargs.pop('output', None)
+        error  = kwargs.pop('error',  None)
 
-        error = kwargs.pop("error", None)
-        input = kwargs.pop("input", None)
         if input is str:
-            raise ValueError("Cannot use `str` as input stream.")
+            raise ValueError('Cannot use `str` as input stream.')
 
         def streamify(arg, mode):
             if isinstance(arg, string_types):
@@ -161,12 +147,8 @@ class Executable(object):
                 return arg, False
 
         ostream, close_ostream = streamify(output, 'w')
-        estream, close_estream = streamify(error, 'w')
-        istream, close_istream = streamify(input, 'r')
-
-        # if they just want to ignore one error code, make it a tuple.
-        if isinstance(ignore_errors, int):
-            ignore_errors = (ignore_errors, )
+        estream, close_estream = streamify(error,  'w')
+        istream, close_istream = streamify(input,  'r')
 
         quoted_args = [arg for arg in args if re.search(r'^"|^\'|"$|\'$', arg)]
         if quoted_args:
@@ -196,7 +178,7 @@ class Executable(object):
 
             rc = self.returncode = proc.returncode
             if fail_on_error and rc != 0 and (rc not in ignore_errors):
-                raise ProcessError("Command exited with status %d:" %
+                raise ProcessError('Command exited with status %d:' %
                                    proc.returncode, cmd_line)
 
             if output is str or error is str:
@@ -209,12 +191,12 @@ class Executable(object):
 
         except OSError as e:
             raise ProcessError(
-                "%s: %s" % (self.exe[0], e.strerror), "Command: " + cmd_line)
+                '%s: %s' % (self.exe[0], e.strerror), 'Command: ' + cmd_line)
 
         except subprocess.CalledProcessError as e:
             if fail_on_error:
                 raise ProcessError(
-                    str(e), "\nExit status %d when invoking command: %s" %
+                    str(e), '\nExit status %d when invoking command: %s' %
                     (proc.returncode, cmd_line))
 
         finally:
@@ -235,27 +217,43 @@ class Executable(object):
         return hash((type(self), ) + tuple(self.exe))
 
     def __repr__(self):
-        return "<exe: %s>" % self.exe
+        return '<exe: %s>' % self.exe
 
     def __str__(self):
         return ' '.join(self.exe)
 
 
-def which(name, **kwargs):
-    """Finds an executable in the path like command-line which."""
-    path = kwargs.get('path', os.environ.get('PATH', '').split(os.pathsep))
+def which(*args, **kwargs):
+    """Finds an executable in the path like command-line which.
+
+    If given multiple executables, returns the first one that is found.
+    If no executables are found, returns None.
+
+    Parameters:
+        *args (str): One or more executables to search for
+
+    Keyword Arguments:
+        path (:func:`list` or str): The path to search. Defaults to ``PATH``
+        required (bool): If set to True, raise an error if executable not found
+
+    Returns:
+        Executable: The first executable that is found in the path
+    """
+    path = kwargs.get('path', os.environ.get('PATH'))
     required = kwargs.get('required', False)
 
-    if not path:
-        path = []
+    if isinstance(path, string_types):
+        path = path.split(os.pathsep)
 
-    for dir in path:
-        exe = os.path.join(dir, name)
-        if os.path.isfile(exe) and os.access(exe, os.X_OK):
-            return Executable(exe)
+    for name in args:
+        for directory in path:
+            exe = os.path.join(directory, name)
+            if os.path.isfile(exe) and os.access(exe, os.X_OK):
+                return Executable(exe)
 
     if required:
-        tty.die("spack requires %s.  Make sure it is in your path." % name)
+        tty.die("spack requires '%s'. Make sure it is in your path." % args[0])
+
     return None
 
 
diff --git a/var/spack/repos/builtin/packages/hdf5-blosc/package.py b/var/spack/repos/builtin/packages/hdf5-blosc/package.py
index 053a4d4852..288f958134 100644
--- a/var/spack/repos/builtin/packages/hdf5-blosc/package.py
+++ b/var/spack/repos/builtin/packages/hdf5-blosc/package.py
@@ -184,7 +184,7 @@ Done.
                "-L%s" % spec["hdf5"].prefix.lib, "-lhdf5")
             try:
                 check = Executable("./check")
-                output = check(return_output=True)
+                output = check(output=str)
             except:
                 output = ""
             success = output == expected
diff --git a/var/spack/repos/builtin/packages/hdf5/package.py b/var/spack/repos/builtin/packages/hdf5/package.py
index 50ad30222f..2f2f016177 100644
--- a/var/spack/repos/builtin/packages/hdf5/package.py
+++ b/var/spack/repos/builtin/packages/hdf5/package.py
@@ -263,7 +263,7 @@ HDF5 version {version} {version}
                  spec['hdf5'].libs.ld_flags.split()))
             try:
                 check = Executable('./check')
-                output = check(return_output=True)
+                output = check(output=str)
             except:
                 output = ""
             success = output == expected
diff --git a/var/spack/repos/builtin/packages/spark/package.py b/var/spack/repos/builtin/packages/spark/package.py
index 3030fb1310..52eb5d7289 100644
--- a/var/spack/repos/builtin/packages/spark/package.py
+++ b/var/spack/repos/builtin/packages/spark/package.py
@@ -72,7 +72,7 @@ class Spark(Package):
         # spack_env.set('JAVA_HOME', self.spec['jdk'].prefix)
 
         hadoop = self.spec['hadoop'].command
-        hadoop_classpath = hadoop('classpath', return_output=True)
+        hadoop_classpath = hadoop('classpath', output=str)
 
         # Remove whitespaces, as they can compromise syntax in
         # module files