Revert addition of SPACK_EDITOR pending review.

This reverts commit d8a26905ee.
This reverts commit 1ee049ccc3.

These were spuriously pushed to `develop`.

lib/spack/docs/getting_started.rst

@@ -368,8 +368,7 @@ Manual compiler configuration
 
 If auto-detection fails, you can manually configure a compiler by
 editing your ``~/.spack/<platform>/compilers.yaml`` file.  You can do this by running
-``spack config edit compilers``, which will open the file in
+``spack config edit compilers``, which will open the file in your ``$EDITOR``.
-:ref:`your favorite editor <controlling-the-editor>`.
 
 Each compiler configuration in the file looks like this:
 

lib/spack/docs/packaging_guide.rst

@@ -229,8 +229,7 @@ always choose to download just one tarball initially, and run
 
 Spack automatically creates a directory in the appropriate repository,
 generates a boilerplate template for your package, and opens up the new
-``package.py`` in your favorite ``$EDITOR`` (see :ref:`controlling-the-editor`
+``package.py`` in your favorite ``$EDITOR``:
-for details):
 
 .. code-block:: python
    :linenos:
@@ -336,31 +335,6 @@ The rest of the tasks you need to do are as follows:
    :ref:`installation_process` is
    covered in detail later.
 
-
-.. _controlling-the-editor:
-
-^^^^^^^^^^^^^^^^^^^^^^^^^
-Controlling the editor
-^^^^^^^^^^^^^^^^^^^^^^^^^
-
-When Spack needs to open an editor for you (e.g., for commands like
-:ref:`cmd-spack-create` or :ref:`spack edit`, it looks at several environment variables
-to figure out what to use. The order of precedence is:
-
-* ``SPACK_EDITOR``: highest precedence, in case you want something specific for Spack;
-* ``VISUAL``: standard environment variable for full-screen editors like ``vim`` or ``emacs``;
-* ``EDITOR``: older environment variable for your editor.
-
-You can set any of these to the command you want to run, e.g., in ``bash`` you might run
-one of these::
-
-  export VISUAL=vim
-  export EDITOR="emacs -nw"
-  export SPACK_EDITOR=nano
-
-If Spack finds none of these variables set, it will look for ``vim``, ``vi``, ``emacs``,
-``nano``, and ``notepad``, in that order.
-
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 Non-downloadable software
 ^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -1813,7 +1787,7 @@ for instructions on setting up a mirror.
 After running ``spack install pgi``, the first thing that will happen is
 Spack will create a global license file located at
 ``$SPACK_ROOT/etc/spack/licenses/pgi/license.dat``. It will then open up the
-file using :ref:`your favorite editor <controlling-the-editor>`. It will look like
+file using the editor set in ``$EDITOR``, or vi if unset. It will look like
 this:
 
 .. code-block:: sh

lib/spack/spack/hooks/licensing.py

@@ -9,7 +9,8 @@
 from llnl.util.filesystem import mkdirp
 from llnl.util.symlink import symlink
 
-import spack.util.editor as ed
+from spack.util.editor import editor
+from spack.util.executable import Executable, which
 
 
 def pre_install(spec):
@@ -37,9 +38,29 @@ def set_up_license(pkg):
         if not os.path.exists(license_path):
             # Create a new license file
             write_license_file(pkg, license_path)
+            # Open up file in user's favorite $EDITOR for editing
+            editor_exe = None
+            if "VISUAL" in os.environ:
+                editor_exe = Executable(os.environ["VISUAL"])
+                # gvim runs in the background by default so we force it to run
+                # in the foreground to make sure the license file is updated
+                # before we try to install
+                if "gvim" in os.environ["VISUAL"]:
+                    editor_exe.add_default_arg("-f")
+            elif "EDITOR" in os.environ:
+                editor_exe = Executable(os.environ["EDITOR"])
+            else:
+                editor_exe = which("vim", "vi", "emacs", "nano")
+            if editor_exe is None:
+                raise EnvironmentError(
+                    "No text editor found! Please set the VISUAL and/or EDITOR"
+                    " environment variable(s) to your preferred text editor."
+                )
 
-            # use spack.util.executable so the editor does not hang on return here
+            def editor_wrapper(exe, args):
-            ed.editor(license_path, exec_fn=ed.executable)
+                editor_exe(license_path)
+
+            editor(license_path, _exec_func=editor_wrapper)
         else:
             # Use already existing license file
             tty.msg("Found already existing license %s" % license_path)

lib/spack/spack/test/util/editor.py

@@ -18,30 +18,6 @@
 ]
 
 
-# env vars that control the editor
-EDITOR_VARS = ["SPACK_EDITOR", "VISUAL", "EDITOR"]
-
-
-@pytest.fixture(scope="module", autouse=True)
-def clean_env_vars():
-    """Unset all editor env vars before tests."""
-    for var in EDITOR_VARS:
-        if var in os.environ:
-            del os.environ[var]
-
-
-@pytest.fixture(autouse=True)
-def working_editor_test_env(working_env):
-    """Don't leak environent variables between functions here."""
-    pass
-
-
-# parameterized fixture for editor var names
-@pytest.fixture(params=EDITOR_VARS)
-def editor_var(request):
-    return request.param
-
-
 def _make_exe(tmpdir_factory, name, contents=None):
     if sys.platform == "win32":
         name += ".exe"
@@ -73,11 +49,6 @@ def vim_exe(tmpdir_factory):
     return _make_exe(tmpdir_factory, "vim", "exit 0")
 
 
-@pytest.fixture(scope="session")
-def gvim_exe(tmpdir_factory):
-    return _make_exe(tmpdir_factory, "gvim", "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])
@@ -93,56 +64,20 @@ def test_find_exe_from_env_var_bad_path(nosuch_exe):
     assert ed._find_exe_from_env_var("FOO") == (None, [])
 
 
-def test_editor_gvim_special_case(gvim_exe):
-    os.environ["EDITOR"] = gvim_exe
-
-    def assert_exec(exe, args):
-        assert exe == gvim_exe
-        assert args == [gvim_exe, "-f", "/path/to/file"]
-        return 0
-
-    assert ed.editor("/path/to/file", exec_fn=assert_exec)
-
-
-def test_editor_precedence(good_exe, gvim_exe, vim_exe, bad_exe):
-    """Ensure we prefer editor variables in order of precedence."""
-    os.environ["SPACK_EDITOR"] = good_exe
-    os.environ["VISUAL"] = gvim_exe
-    os.environ["EDITOR"] = vim_exe
-    correct_exe = good_exe
-
-    def assert_callback(exe, args):
-        result = ed.executable(exe, args)
-        if result == 0:
-            assert exe == correct_exe
-        return result
-
-    ed.editor(exec_fn=assert_callback)
-
-    os.environ["SPACK_EDITOR"] = bad_exe
-    correct_exe = gvim_exe
-    ed.editor(exec_fn=assert_callback)
-
-    os.environ["VISUAL"] = bad_exe
-    correct_exe = vim_exe
-    ed.editor(exec_fn=assert_callback)
-
-
 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(editor_var, good_exe):
+def test_editor_visual(good_exe):
-    os.environ[editor_var] = good_exe
+    os.environ["VISUAL"] = good_exe
 
     def assert_exec(exe, args):
         assert exe == good_exe
         assert args == [good_exe, "/path/to/file"]
-        return 0
 
-    ed.editor("/path/to/file", exec_fn=assert_exec)
+    ed.editor("/path/to/file", _exec_func=assert_exec)
 
 
 def test_editor_visual_bad(good_exe, bad_exe):
@@ -155,32 +90,34 @@ def assert_exec(exe, args):
 
         assert exe == good_exe
         assert args == [good_exe, "/path/to/file"]
-        return 0
 
-    ed.editor("/path/to/file", exec_fn=assert_exec)
+    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"]
-        return 0
 
-    ed.editor("/path/to/file", exec_fn=assert_exec)
+    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"]
-        return 0
 
-    ed.editor("/path/to/file", exec_fn=assert_exec)
+    ed.editor("/path/to/file", _exec_func=assert_exec)
 
 
 def test_editor_both_bad(nosuch_exe, vim_exe):
@@ -192,32 +129,19 @@ def test_editor_both_bad(nosuch_exe, vim_exe):
     def assert_exec(exe, args):
         assert exe == vim_exe
         assert args == [vim_exe, "/path/to/file"]
-        return 0
 
-    ed.editor("/path/to/file", exec_fn=assert_exec)
+    ed.editor("/path/to/file", _exec_func=assert_exec)
 
 
 def test_no_editor():
+    if "VISUAL" in os.environ:
+        del os.environ["VISUAL"]
+    if "EDITOR" in os.environ:
+        del os.environ["EDITOR"]
     os.environ["PATH"] = ""
 
     def assert_exec(exe, args):
         assert False
 
     with pytest.raises(EnvironmentError, match=r"No text editor found.*"):
-        ed.editor("/path/to/file", exec_fn=assert_exec)
+        ed.editor("/path/to/file", _exec_func=assert_exec)
-
-    def assert_exec(exe, args):
-        return False
-
-    with pytest.raises(EnvironmentError, match=r"No text editor found.*"):
-        ed.editor("/path/to/file", exec_fn=assert_exec)
-
-
-def test_exec_fn_executable(editor_var, good_exe, bad_exe):
-    """Make sure editor() works with ``ed.executable`` as well as execv"""
-    os.environ[editor_var] = good_exe
-    assert ed.editor(exec_fn=ed.executable)
-
-    os.environ[editor_var] = bad_exe
-    with pytest.raises(EnvironmentError, match=r"No text editor found.*"):
-        ed.editor(exec_fn=ed.executable)

lib/spack/spack/util/editor.py

@@ -14,18 +14,17 @@
 """
 import os
 import shlex
-from typing import Callable, List
 
 import llnl.util.tty as tty
 
 import spack.config
-import spack.util.executable
+from spack.util.executable import which_string
 
 #: editors to try if VISUAL and EDITOR are not set
 _default_editors = ["vim", "vi", "emacs", "nano", "notepad"]
 
 
-def _find_exe_from_env_var(var: str):
+def _find_exe_from_env_var(var):
     """Find an executable from an environment variable.
 
     Args:
@@ -46,22 +45,12 @@ def _find_exe_from_env_var(var: str):
     if not args:
         return None, []
 
-    exe = spack.util.executable.which_string(args[0])
+    exe = which_string(args[0])
     args = [exe] + args[1:]
     return exe, args
 
 
-def executable(exe: str, args: List[str]) -> int:
+def editor(*args, **kwargs):
-    """Wrapper that makes ``spack.util.executable.Executable`` look like ``os.execv()``.
-
-    Use this with ``editor()`` if you want it to return instead of running ``execv``.
-    """
-    cmd = spack.util.executable.Executable(exe)
-    cmd(*args[1:], fail_on_error=False)
-    return cmd.returncode
-
-
-def editor(*args: List[str], exec_fn: Callable[[str, List[str]], int] = os.execv) -> bool:
     """Invoke the user's editor.
 
     This will try to execute the following, in order:
@@ -76,30 +65,27 @@ def editor(*args: List[str], exec_fn: Callable[[str, List[str]], int] = os.execv
     searching the full list above, we'll raise an error.
 
     Arguments:
-        args: args to pass to editor
+        args (list): args to pass to editor
 
     Optional Arguments:
-        exec_fn: invoke this function to run; use ``spack.util.editor.executable`` if you
+        _exec_func (function): invoke this function instead of ``os.execv()``
-            want something that returns, instead of the default ``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`` does return successfully.
+            ``_exec_func`` does return successfully.
         """
-        # gvim runs in the background by default so we force it to run
-        # in the foreground to ensure it gets attention.
-        if "gvim" in exe:
-            exe, *rest = args
-            args = [exe, "-f"] + rest
-
         try:
-            return exec_fn(exe, args) == 0
+            _exec_func(exe, args)
+            return True
 
-        except (OSError, spack.util.executable.ProcessError) as e:
+        except OSError as e:
             if spack.config.get("config:debug"):
                 raise
 
@@ -127,19 +113,17 @@ def try_env_var(var):
         return try_exec(exe, full_args, var)
 
     # try standard environment variables
-    if try_env_var("SPACK_EDITOR"):
-        return True
     if try_env_var("VISUAL"):
-        return True
+        return
     if try_env_var("EDITOR"):
-        return True
+        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 = spack.util.executable.which_string(*_default_editors)
+    exe = which_string(*_default_editors)
     if exe and try_exec(exe, [exe] + list(args)):
-        return True
+        return
 
     # Fail if nothing could be found
     raise EnvironmentError(