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
This commit is contained in:
Todd Gamblin
committed by
Greg Becker
Greg Becker
parent
5fb6145f6f
commit
0aed3bcea6
129
lib/spack/spack/test/util/editor.py
Normal file
129
lib/spack/spack/test/util/editor.py
Normal file
@@ -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)
|
||||
@@ -12,30 +12,120 @@
|
||||
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.
|
||||
"""Invoke the user's 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.')
|
||||
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.')
|
||||
|
||||
@@ -224,6 +224,26 @@ def __str__(self):
|
||||
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):
|
||||
|
||||
Reference in New Issue
Block a user