From 6845f41d671f65f005d28d9d9f6dcacf5730e715 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Tue, 18 Apr 2023 03:57:24 -0700 Subject: Revert addition of SPACK_EDITOR pending review. This reverts commit d8a26905ee0fc9e72a05a87bf7ced7e7fc2d4900. This reverts commit 1ee049ccc3d4d4bf1e8dafad0247f3cef01dda58. These were spuriously pushed to `develop`. --- lib/spack/docs/getting_started.rst | 3 +- lib/spack/docs/packaging_guide.rst | 30 +--------- lib/spack/spack/hooks/licensing.py | 29 ++++++++-- lib/spack/spack/test/util/editor.py | 110 ++++++------------------------------ lib/spack/spack/util/editor.py | 50 ++++++---------- 5 files changed, 62 insertions(+), 160 deletions(-) diff --git a/lib/spack/docs/getting_started.rst b/lib/spack/docs/getting_started.rst index 46bf657d62..3c847b876e 100644 --- a/lib/spack/docs/getting_started.rst +++ b/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//compilers.yaml`` file. You can do this by running -``spack config edit compilers``, which will open the file in -:ref:`your favorite editor `. +``spack config edit compilers``, which will open the file in your ``$EDITOR``. Each compiler configuration in the file looks like this: diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst index c81c68dc80..0b1e596abf 100644 --- a/lib/spack/docs/packaging_guide.rst +++ b/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` -for details): +``package.py`` in your favorite ``$EDITOR``: .. 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 `. It will look like +file using the editor set in ``$EDITOR``, or vi if unset. It will look like this: .. code-block:: sh diff --git a/lib/spack/spack/hooks/licensing.py b/lib/spack/spack/hooks/licensing.py index e21445c302..27628776f2 100644 --- a/lib/spack/spack/hooks/licensing.py +++ b/lib/spack/spack/hooks/licensing.py @@ -9,7 +9,8 @@ import llnl.util.tty as tty 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) - - # use spack.util.executable so the editor does not hang on return here - ed.editor(license_path, exec_fn=ed.executable) + # 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) 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 7ba21a2d0b..613185537a 100644 --- a/lib/spack/spack/test/util/editor.py +++ b/lib/spack/spack/test/util/editor.py @@ -18,30 +18,6 @@ pytestmark = [ ] -# 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): - os.environ[editor_var] = good_exe +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"] - 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 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_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) - - 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) + 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 87dea8b831..737a71d433 100644 --- a/lib/spack/spack/util/editor.py +++ b/lib/spack/spack/util/editor.py @@ -14,18 +14,17 @@ 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 -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: - """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: +def editor(*args, **kwargs): """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 - want something that returns, instead of the default ``os.execv()``. + _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`` 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 editor(*args: List[str], exec_fn: Callable[[str, List[str]], int] = os.execv 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( -- cgit v1.2.3-60-g2f50