From 0aed3bcea676a9df4a5c1c26a9a5ca7151d85215 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sat, 20 Apr 2019 20:51:45 -0700 Subject: 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 --- lib/spack/spack/test/util/editor.py | 129 +++++++++++++++++++++++++++++++++++ lib/spack/spack/util/editor.py | 132 ++++++++++++++++++++++++++++++------ lib/spack/spack/util/executable.py | 38 ++++++----- 3 files changed, 262 insertions(+), 37 deletions(-) create mode 100644 lib/spack/spack/test/util/editor.py (limited to 'lib') 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 # the "visual" editor (per POSIX) + 2. $EDITOR # the regular editor (per POSIX) + 3. some default editor (see ``_default_editors``) with + + 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): -- cgit v1.2.3-60-g2f50