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
This commit is contained in:
Todd Gamblin
parent
480b7f397e
commit
d8a26905ee
3 changed files with 104 additions and 59 deletions
|
|
@ -9,8 +9,7 @@
|
|||
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)
|
||||
|
|
|
|||
|
|
@ -18,6 +18,30 @@
|
|||
]
|
||||
|
||||
|
||||
# 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 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(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)
|
||||
|
|
|
|||
|
|
@ -14,17 +14,18 @@
|
|||
"""
|
||||
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.
|
||||
"""
|
||||
try:
|
||||
_exec_func(exe, args)
|
||||
return True
|
||||
# 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
|
||||
|
||||
except OSError as e:
|
||||
try:
|
||||
return exec_fn(exe, args) == 0
|
||||
|
||||
except (OSError, spack.util.executable.ProcessError) as e:
|
||||
if spack.config.get("config:debug"):
|
||||
raise
|
||||
|
||||
|
|
@ -114,16 +128,16 @@ def try_env_var(var):
|
|||
|
||||
# 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(
|
||||
|
|
|
|||
Loading…
Reference in a new issue