spack edit: use execv instead of Executable (#11245)

- `spack edit` previously used `spack.util.executable` `Executable` objects,
  and didn't `exec` the editor like you'd expect it to

- This meant that Spack was still running while your editor was, and
 stdout/stdin were being set up in weird ways

- e.g. on macOS, if you call `spack edit` with `EDITOR` set to the
  builtin `emacs` command, then type `Ctrl-g`, the whole thing dies with
  a `==> Error: Keyboard interrupt`

- Fix all this by changing spack.util.editor to use `os.execv` instead of
  Spack's `Executable` object

3 files changed, 262 insertions, 37 deletions

diff --git a/lib/spack/spack/test/util/editor.py b/lib/spack/spack/test/util/editor.py
new file mode 100644
index 0000000000..2bb30cd1a2
--- /dev/null
+++ b/lib/spack/spack/test/util/editor.py
@@ -0,0 +1,129 @@
+# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import os
+import pytest
+
+from llnl.util.filesystem import set_executable
+
+import spack.util.editor as ed
+
+
+pytestmark = pytest.mark.usefixtures('working_env')
+
+
+def _make_exe(tmpdir_factory, name, contents=None):
+    path = str(tmpdir_factory.mktemp('%s_exe' % name).join(name))
+    if contents is not None:
+        with open(path, 'w') as f:
+            f.write('#!/bin/sh\n%s\n' % contents)
+        set_executable(path)
+    return path
+
+
+@pytest.fixture(scope='session')
+def good_exe(tmpdir_factory):
+    return _make_exe(tmpdir_factory, 'good', 'exit 0')
+
+
+@pytest.fixture(scope='session')
+def bad_exe(tmpdir_factory):
+    return _make_exe(tmpdir_factory, 'bad', 'exit 1')
+
+
+@pytest.fixture(scope='session')
+def nosuch_exe(tmpdir_factory):
+    return _make_exe(tmpdir_factory, 'nosuch')
+
+
+@pytest.fixture(scope='session')
+def vim_exe(tmpdir_factory):
+    return _make_exe(tmpdir_factory, 'vim', 'exit 0')
+
+
+def test_find_exe_from_env_var(good_exe):
+    os.environ['EDITOR'] = good_exe
+    assert ed._find_exe_from_env_var('EDITOR') == (good_exe, [good_exe])
+
+
+def test_find_exe_from_env_var_with_args(good_exe):
+    os.environ['EDITOR'] = good_exe + ' a b c'
+    assert ed._find_exe_from_env_var('EDITOR') == (
+        good_exe, [good_exe, 'a', 'b', 'c'])
+
+
+def test_find_exe_from_env_var_bad_path(nosuch_exe):
+    os.environ['EDITOR'] = nosuch_exe
+    assert ed._find_exe_from_env_var('FOO') == (None, [])
+
+
+def test_find_exe_from_env_var_no_editor():
+    if 'FOO' in os.environ:
+        os.environ.unset('FOO')
+    assert ed._find_exe_from_env_var('FOO') == (None, [])
+
+
+def test_editor_visual(good_exe):
+    os.environ['VISUAL'] = good_exe
+
+    def assert_exec(exe, args):
+        assert exe == good_exe
+        assert args == [good_exe, '/path/to/file']
+
+    ed.editor('/path/to/file', _exec_func=assert_exec)
+
+
+def test_editor_visual_bad(good_exe, bad_exe):
+    os.environ['VISUAL'] = bad_exe
+    os.environ['EDITOR'] = good_exe
+
+    def assert_exec(exe, args):
+        if exe == bad_exe:
+            raise OSError()
+
+        assert exe == good_exe
+        assert args == [good_exe, '/path/to/file']
+
+    ed.editor('/path/to/file', _exec_func=assert_exec)
+
+
+def test_editor_no_visual(good_exe):
+    if 'VISUAL' in os.environ:
+        del os.environ['VISUAL']
+    os.environ['EDITOR'] = good_exe
+
+    def assert_exec(exe, args):
+        assert exe == good_exe
+        assert args == [good_exe, '/path/to/file']
+
+    ed.editor('/path/to/file', _exec_func=assert_exec)
+
+
+def test_editor_no_visual_with_args(good_exe):
+    if 'VISUAL' in os.environ:
+        del os.environ['VISUAL']
+
+    # editor has extra args in the var (e.g., emacs -nw)
+    os.environ['EDITOR'] = good_exe + ' -nw --foo'
+
+    def assert_exec(exe, args):
+        assert exe == good_exe
+        assert args == [good_exe, '-nw', '--foo', '/path/to/file']
+
+    ed.editor('/path/to/file', _exec_func=assert_exec)
+
+
+def test_editor_both_bad(nosuch_exe, vim_exe):
+    os.environ['VISUAL'] = nosuch_exe
+    os.environ['EDITOR'] = nosuch_exe
+
+    os.environ['PATH'] = '%s:%s' % (
+        os.path.dirname(vim_exe), os.environ['PATH'])
+
+    def assert_exec(exe, args):
+        assert exe == vim_exe
+        assert args == [vim_exe, '/path/to/file']
+
+    ed.editor('/path/to/file', _exec_func=assert_exec)
diff --git a/lib/spack/spack/util/editor.py b/lib/spack/spack/util/editor.py
index 8d9ee2f04b..22db247c6a 100644
--- a/lib/spack/spack/util/editor.py
+++ b/lib/spack/spack/util/editor.py
@@ -12,30 +12,120 @@ specified editor fails (e.g. no DISPLAY for a graphical editor). If
 neither variable is set, we fall back to one of several common editors,
 raising an EnvironmentError if we are unable to find one.
 """
-import copy
 import os
+import shlex
 
-from spack.util.executable import Executable, which
+import llnl.util.tty as tty
 
-_visual_exe\
-    = Executable(os.environ['VISUAL']) if 'VISUAL' in os.environ else None
-_editor_exe\
-    = Executable(os.environ['EDITOR']) \
-    if 'EDITOR' in os.environ else which('vim', 'vi', 'emacs', 'nano')
+import spack.config
+from spack.util.executable import which_string
+
+
+#: editors to try if VISUAL and EDITOR are not set
+_default_editors = ['vim', 'vi', 'emacs', 'nano']
+
+
+def _find_exe_from_env_var(var):
+    """Find an executable from an environment variable.
+
+    Args:
+        var (str): environment variable name
+
+    Returns:
+        (str or None, list): executable string (or None if not found) and
+            arguments parsed from the env var
+    """
+    # try to get the environment variable
+    exe = os.environ.get(var)
+    if not exe:
+        return None, []
+
+    # split env var into executable and args if needed
+    args = shlex.split(exe)
+    if not args:
+        return None, []
+
+    exe = which_string(args[0])
+    args = [exe] + args[1:]
+    return exe, args
 
 
-# Invoke the user's editor.
 def editor(*args, **kwargs):
-    if _visual_exe:
-        visual_kwargs = copy.copy(kwargs)
-        visual_kwargs['fail_on_error'] = False
-        _visual_exe(*args, **visual_kwargs)
-        if _visual_exe.returncode == 0:
-            return  # Otherwise, fall back to EDITOR.
-
-    if _editor_exe:
-        _editor_exe(*args, **kwargs)
-    else:
-        raise EnvironmentError(
-            'No text editor found! Please set the VISUAL and/or EDITOR '
-            'environment variable(s) to your preferred text editor.')
+    """Invoke the user's editor.
+
+    This will try to execute the following, in order:
+
+      1. $VISUAL <args>    # the "visual" editor (per POSIX)
+      2. $EDITOR <args>    # the regular editor (per POSIX)
+      3. some default editor (see ``_default_editors``) with <args>
+
+    If an environment variable isn't defined, it is skipped.  If it
+    points to something that can't be executed, we'll print a
+    warning. And if we can't find anything that can be executed after
+    searching the full list above, we'll raise an error.
+
+    Arguments:
+        args (list of str): args to pass to editor
+
+    Optional Arguments:
+        _exec_func (function): invoke this function instead of ``os.execv()``
+
+    """
+    # allow this to be customized for testing
+    _exec_func = kwargs.get('_exec_func', os.execv)
+
+    def try_exec(exe, args, var=None):
+        """Try to execute an editor with execv, and warn if it fails.
+
+        Returns: (bool) False if the editor failed, ideally does not
+            return if ``execv`` succeeds, and ``True`` if the
+            ``_exec_func`` does return successfully.
+        """
+        try:
+            _exec_func(exe, args)
+            return True
+
+        except OSError as e:
+            if spack.config.get('config:debug'):
+                raise
+
+            # Show variable we were trying to use, if it's from one
+            if var:
+                exe = '$%s (%s)' % (var, exe)
+            tty.warn('Could not execute %s due to error:' % exe, str(e))
+            return False
+
+    def try_env_var(var):
+        """Find an editor from an environment variable and try to exec it.
+
+        This will warn if the variable points to something is not
+        executable, or if there is an error when trying to exec it.
+        """
+        if var not in os.environ:
+            return False
+
+        exe, editor_args = _find_exe_from_env_var(var)
+        if not exe:
+            tty.warn('$%s is not an executable:' % var, os.environ[var])
+            return False
+
+        full_args = editor_args + list(args)
+        return try_exec(exe, full_args, var)
+
+    # try standard environment variables
+    if try_env_var('VISUAL'):
+        return
+    if try_env_var('EDITOR'):
+        return
+
+    # nothing worked -- try the first default we can find don't bother
+    # trying them all -- if we get here and one fails, something is
+    # probably much more deeply wrong with the environment.
+    exe = which_string(*_default_editors)
+    if try_exec(exe, [exe] + list(args)):
+        return
+
+    # Fail if nothing could be found
+    raise EnvironmentError(
+        'No text editor found! Please set the VISUAL and/or EDITOR '
+        'environment variable(s) to your preferred text editor.')
diff --git a/lib/spack/spack/util/executable.py b/lib/spack/spack/util/executable.py
index 603fb5c2ac..de18d24922 100644
--- a/lib/spack/spack/util/executable.py
+++ b/lib/spack/spack/util/executable.py
@@ -224,6 +224,26 @@ class Executable(object):
         return ' '.join(self.exe)
 
 
+def which_string(*args, **kwargs):
+    """Like ``which()``, but return a string instead of an ``Executable``."""
+    path = kwargs.get('path', os.environ.get('PATH', ''))
+    required = kwargs.get('required', False)
+
+    if isinstance(path, string_types):
+        path = path.split(os.pathsep)
+
+    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 exe
+
+    if required:
+        tty.die("spack requires '%s'. Make sure it is in your path." % args[0])
+
+    return None
+
+
 def which(*args, **kwargs):
     """Finds an executable in the path like command-line which.
 
@@ -240,22 +260,8 @@ def which(*args, **kwargs):
     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 isinstance(path, string_types):
-        path = path.split(os.pathsep)
-
-    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." % args[0])
-
-    return None
+    exe = which_string(*args, **kwargs)
+    return Executable(exe) if exe else None
 
 
 class ProcessError(spack.error.SpackError):