refactor: unify use of spack.util.editor

Code from `spack.util.editor` was duplicated into our licensing hook in #11968. We
really only want one place where editor search logic is implemented. This consolidates
the logic into `spack.util.editor`, including a special case to run `gvim` with `-f`.

- [x] consolidate editor search logic in spack.util.editor
- [x] add tests for licensing case, where `Executable` is used instead of `os.execv`
- [x] make `_exec_func` argument of `editor()` into public `exec_fn` arg
- [x] add type annotations

3 files changed, 104 insertions, 59 deletions

diff --git a/lib/spack/spack/hooks/licensing.py b/lib/spack/spack/hooks/licensing.py
index 27628776f2..e21445c302 100644
--- a/lib/spack/spack/hooks/licensing.py
+++ b/lib/spack/spack/hooks/licensing.py
@@ -9,8 +9,7 @@ import llnl.util.tty as tty
 from llnl.util.filesystem import mkdirp
 from llnl.util.symlink import symlink
 
-from spack.util.editor import editor
-from spack.util.executable import Executable, which
+import spack.util.editor as ed
 
 
 def pre_install(spec):
@@ -38,29 +37,9 @@ 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."
-                )
-
-            def editor_wrapper(exe, args):
-                editor_exe(license_path)
-
-            editor(license_path, _exec_func=editor_wrapper)
+
+            # use spack.util.executable so the editor does not hang on return here
+            ed.editor(license_path, exec_fn=ed.executable)
         else:
             # Use already existing license file
             tty.msg("Found already existing license %s" % license_path)
diff --git a/lib/spack/spack/test/util/editor.py b/lib/spack/spack/test/util/editor.py
index 613185537a..fea4ff03d2 100644
--- a/lib/spack/spack/test/util/editor.py
+++ b/lib/spack/spack/test/util/editor.py
@@ -18,6 +18,30 @@ pytestmark = [
 ]
 
 
+# env vars that control the editor
+EDITOR_VARS = ["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"
@@ -49,6 +73,11 @@ 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])
@@ -64,20 +93,32 @@ 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_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 test_editor(editor_var, good_exe):
+    os.environ[editor_var] = 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_func=assert_exec)
+    ed.editor("/path/to/file", exec_fn=assert_exec)
 
 
 def test_editor_visual_bad(good_exe, bad_exe):
@@ -90,34 +131,32 @@ def test_editor_visual_bad(good_exe, bad_exe):
 
         assert exe == good_exe
         assert args == [good_exe, "/path/to/file"]
+        return 0
 
-    ed.editor("/path/to/file", _exec_func=assert_exec)
+    ed.editor("/path/to/file", exec_fn=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_func=assert_exec)
+    ed.editor("/path/to/file", exec_fn=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_func=assert_exec)
+    ed.editor("/path/to/file", exec_fn=assert_exec)
 
 
 def test_editor_both_bad(nosuch_exe, vim_exe):
@@ -129,19 +168,32 @@ 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_func=assert_exec)
+    ed.editor("/path/to/file", exec_fn=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_func=assert_exec)
+        ed.editor("/path/to/file", exec_fn=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)
diff --git a/lib/spack/spack/util/editor.py b/lib/spack/spack/util/editor.py
index 737a71d433..8d053ac8eb 100644
--- a/lib/spack/spack/util/editor.py
+++ b/lib/spack/spack/util/editor.py
@@ -14,17 +14,18 @@ raising an EnvironmentError if we are unable to find one.
 """
 import os
 import shlex
+from typing import Callable, List
 
 import llnl.util.tty as tty
 
 import spack.config
-from spack.util.executable import which_string
+import spack.util.executable
 
 #: editors to try if VISUAL and EDITOR are not set
 _default_editors = ["vim", "vi", "emacs", "nano", "notepad"]
 
 
-def _find_exe_from_env_var(var):
+def _find_exe_from_env_var(var: str):
     """Find an executable from an environment variable.
 
     Args:
@@ -45,12 +46,22 @@ def _find_exe_from_env_var(var):
     if not args:
         return None, []
 
-    exe = which_string(args[0])
+    exe = spack.util.executable.which_string(args[0])
     args = [exe] + args[1:]
     return exe, args
 
 
-def editor(*args, **kwargs):
+def executable(exe: str, args: List[str]) -> int:
+    """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:
@@ -65,27 +76,30 @@ def editor(*args, **kwargs):
     searching the full list above, we'll raise an error.
 
     Arguments:
-        args (list): args to pass to editor
+        args: args to pass to editor
 
     Optional Arguments:
-        _exec_func (function): invoke this function instead of ``os.execv()``
-
+        exec_fn: invoke this function to run; use ``spack.util.editor.executable`` if you
+            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_func`` does return successfully.
+            ``exec`` 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:
-            _exec_func(exe, args)
-            return True
+            return exec_fn(exe, args) == 0
 
-        except OSError as e:
+        except (OSError, spack.util.executable.ProcessError) as e:
             if spack.config.get("config:debug"):
                 raise
 
@@ -114,16 +128,16 @@ def editor(*args, **kwargs):
 
     # try standard environment variables
     if try_env_var("VISUAL"):
-        return
+        return True
     if try_env_var("EDITOR"):
-        return
+        return True
 
     # 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)
+    exe = spack.util.executable.which_string(*_default_editors)
     if exe and try_exec(exe, [exe] + list(args)):
-        return
+        return True
 
     # Fail if nothing could be found
     raise EnvironmentError(