Spack command extensions: error-handling (#13635)

Generally speaking, errors that are encountered when attempting to load
command extensions now terminate the running Spack instance.

* Added new exceptions `spack.cmd.PythonNameError` and
  `spack.cmd.CommandNameError`.
* New functions `spack.cmd.require_python_name(pname)` and
  `spack.cmd.require_cmd_name(cname)` check that `pname` and `cname`
  respectively meet requirements, throwing the appropriate error if not.
* `spack.cmd.get_module()` uses `require_cmd_name()` and passes through
  exceptions from module load attempts.
* `spack.cmd.get_command()` uses `require_cmd_name()` and invokes
  `get_module()` with the correct command-name form rather than the
  previous (incorrect) Python name.
* Added New exceptions `spack.extensions.CommandNotFoundError` and
  `spack.extensions.ExtensionNamingError`.
* `_extension_regexp` has a new leading underscore to indicate expected
  privacy.
* `spack.extensions.extension_name()` raises an `ExtensionNamingError`
  rather than using `tty.warn()`.
* `spack.extensions.load_command_extension()` checks command source
  existence early and bails out if missing. Also, exceptions raised by
  `load_module_from_file()` are passed through.
* `spack.extensions.get_module()` raises `CommandNotFoundError` as
  appropriate.
* Spack `main()` allows `parser.add_command()` exceptions to cause
  program end.

Tests:

* More common boilerplate has been pulled out into fixtures including
  `sys.modules` dictionary cleanup and resource-managed creation of a
  simple command extension with specified contents in the source file
  for a single named command.
* "Hello, World!" test now uses a command named `hello-world` instead of
  `hello` in order to verify correct handling of commands with hyphens.
* New tests for:
  * Missing (or misnamed) command.
  * Badly-named extension.
  * Verification that errors encountered during import of a command are
    propagated upward.

Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com>

5 files changed, 332 insertions, 85 deletions

diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index 83e12004a1..594b5e3a8e 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -43,11 +43,28 @@ def python_name(cmd_name):
     return cmd_name.replace("-", "_")
 
 
+def require_python_name(pname):
+    """Require that the provided name is a valid python name (per
+    python_name()). Useful for checking parameters for function
+    prerequisites."""
+    if python_name(pname) != pname:
+        raise PythonNameError(pname)
+
+
 def cmd_name(python_name):
     """Convert module name (with ``_``) to command name (with ``-``)."""
     return python_name.replace('_', '-')
 
 
+def require_cmd_name(cname):
+    """Require that the provided name is a valid command name (per
+    cmd_name()). Useful for checking parameters for function
+    prerequisites.
+    """
+    if cmd_name(cname) != cname:
+        raise CommandNameError(cname)
+
+
 #: global, cached list of all commands -- access through all_commands()
 _all_commands = None
 
@@ -91,6 +108,7 @@ def get_module(cmd_name):
         cmd_name (str): name of the command for which to get a module
             (contains ``-``, not ``_``).
     """
+    require_cmd_name(cmd_name)
     pname = python_name(cmd_name)
 
     try:
@@ -102,8 +120,6 @@ def get_module(cmd_name):
         tty.debug('Imported {0} from built-in commands'.format(pname))
     except ImportError:
         module = spack.extensions.get_module(cmd_name)
-        if not module:
-            raise
 
     attr_setdefault(module, SETUP_PARSER, lambda *args: None)  # null-op
     attr_setdefault(module, DESCRIPTION, "")
@@ -116,14 +132,16 @@ def get_module(cmd_name):
 
 
 def get_command(cmd_name):
-    """Imports the command's function from a module and returns it.
+    """Imports the command function associated with cmd_name.
+
+    The function's name is derived from cmd_name using python_name().
 
     Args:
-        cmd_name (str): name of the command for which to get a module
-            (contains ``-``, not ``_``).
+        cmd_name (str): name of the command (contains ``-``, not ``_``).
     """
+    require_cmd_name(cmd_name)
     pname = python_name(cmd_name)
-    return getattr(get_module(pname), pname)
+    return getattr(get_module(cmd_name), pname)
 
 
 def parse_specs(args, **kwargs):
@@ -419,6 +437,22 @@ def spack_is_git_repo():
         return os.path.isdir('.git')
 
 
+class PythonNameError(spack.error.SpackError):
+    """Exception class thrown for impermissible python names"""
+    def __init__(self, name):
+        self.name = name
+        super(PythonNameError, self).__init__(
+            '{0} is not a permissible Python name.'.format(name))
+
+
+class CommandNameError(spack.error.SpackError):
+    """Exception class thrown for impermissible command names"""
+    def __init__(self, name):
+        self.name = name
+        super(CommandNameError, self).__init__(
+            '{0} is not a permissible Spack command name.'.format(name))
+
+
 ########################################
 # argparse types for argument validation
 ########################################
diff --git a/lib/spack/spack/extensions.py b/lib/spack/spack/extensions.py
index b8fde7d7fa..4358dcd52f 100644
--- a/lib/spack/spack/extensions.py
+++ b/lib/spack/spack/extensions.py
@@ -11,10 +11,17 @@ import sys
 import types
 
 import llnl.util.lang
-import llnl.util.tty as tty
 import spack.config
+import spack.error
 
-extension_regexp = re.compile(r'spack-([\w]*)')
+_extension_regexp = re.compile(r'spack-(\w[-\w]*)$')
+
+
+# TODO: For consistency we should use spack.cmd.python_name(), but
+#       currently this would create a circular relationship between
+#       spack.cmd and spack.extensions.
+def _python_name(cmd_name):
+    return cmd_name.replace('-', '_')
 
 
 def extension_name(path):
@@ -24,15 +31,16 @@ def extension_name(path):
         path (str): path where the extension resides
 
     Returns:
-        The extension name or None if path doesn't match the format
-        for Spack's extension.
+        The extension name.
+
+    Raises:
+         ExtensionNamingError: if path does not match the expected format
+             for a Spack command extension.
     """
-    regexp_match = re.search(extension_regexp, os.path.basename(path))
+    regexp_match = re.search(_extension_regexp,
+                             os.path.basename(os.path.normpath(path)))
     if not regexp_match:
-        msg = "[FOLDER NAMING]"
-        msg += " {0} doesn't match the format for Spack's extensions"
-        tty.warn(msg.format(path))
-        return None
+        raise ExtensionNamingError(path)
     return regexp_match.group(1)
 
 
@@ -40,23 +48,30 @@ def load_command_extension(command, path):
     """Loads a command extension from the path passed as argument.
 
     Args:
-        command (str): name of the command
+        command (str): name of the command (contains ``-``, not ``_``).
         path (str): base path of the command extension
 
     Returns:
-        A valid module object if the command is found or None
+        A valid module if found and loadable; None if not found. Module
+    loading exceptions are passed through.
     """
-    extension = extension_name(path)
-    if not extension:
-        return None
+    extension = _python_name(extension_name(path))
 
     # Compute the name of the module we search, exit early if already imported
     cmd_package = '{0}.{1}.cmd'.format(__name__, extension)
-    python_name = command.replace('-', '_')
+    python_name = _python_name(command)
     module_name = '{0}.{1}'.format(cmd_package, python_name)
     if module_name in sys.modules:
         return sys.modules[module_name]
 
+    # Compute the absolute path of the file to be loaded, along with the
+    # name of the python module where it will be stored
+    cmd_path = os.path.join(path, extension, 'cmd', python_name + '.py')
+
+    # Short circuit if the command source file does not exist
+    if not os.path.exists(cmd_path):
+        return None
+
     def ensure_package_creation(name):
         package_name = '{0}.{1}'.format(__name__, name)
         if package_name in sys.modules:
@@ -82,17 +97,10 @@ def load_command_extension(command, path):
     ensure_package_creation(extension)
     ensure_package_creation(extension + '.cmd')
 
-    # Compute the absolute path of the file to be loaded, along with the
-    # name of the python module where it will be stored
-    cmd_path = os.path.join(path, extension, 'cmd', command + '.py')
-
-    try:
-        # TODO: Upon removal of support for Python 2.6 substitute the call
-        # TODO: below with importlib.import_module(module_name)
-        module = llnl.util.lang.load_module_from_file(module_name, cmd_path)
-        sys.modules[module_name] = module
-    except (ImportError, IOError):
-        module = None
+    # TODO: Upon removal of support for Python 2.6 substitute the call
+    # TODO: below with importlib.import_module(module_name)
+    module = llnl.util.lang.load_module_from_file(module_name, cmd_path)
+    sys.modules[module_name] = module
 
     return module
 
@@ -103,9 +111,8 @@ def get_command_paths():
     extension_paths = spack.config.get('config:extensions') or []
 
     for path in extension_paths:
-        extension = extension_name(path)
-        if extension:
-            command_paths.append(os.path.join(path, extension, 'cmd'))
+        extension = _python_name(extension_name(path))
+        command_paths.append(os.path.join(path, extension, 'cmd'))
 
     return command_paths
 
@@ -144,7 +151,7 @@ def get_module(cmd_name):
         if module:
             return module
     else:
-        return None
+        raise CommandNotFoundError(cmd_name)
 
 
 def get_template_dirs():
@@ -154,3 +161,23 @@ def get_template_dirs():
     extension_dirs = spack.config.get('config:extensions') or []
     extensions = [os.path.join(x, 'templates') for x in extension_dirs]
     return extensions
+
+
+class CommandNotFoundError(spack.error.SpackError):
+    """Exception class thrown when a requested command is not recognized as
+    such.
+    """
+    def __init__(self, cmd_name):
+        super(CommandNotFoundError, self).__init__(
+            '{0} is not a recognized Spack command or extension command;'
+            ' check with `spack commands`.'.format(cmd_name))
+
+
+class ExtensionNamingError(spack.error.SpackError):
+    """Exception class thrown when a configured extension does not follow
+    the expected naming convention.
+    """
+    def __init__(self, path):
+        super(ExtensionNamingError, self).__init__(
+            '{0} does not match the format for a Spack extension path.'
+            .format(path))
diff --git a/lib/spack/spack/main.py b/lib/spack/spack/main.py
index f6da7b0fb0..3677764cb6 100644
--- a/lib/spack/spack/main.py
+++ b/lib/spack/spack/main.py
@@ -736,17 +736,11 @@ def main(argv=None):
         # ensure options on spack command come before everything
         setup_main_options(args)
 
-        # Try to load the particular command the caller asked for.  If there
-        # is no module for it, just die.
+        # Try to load the particular command the caller asked for.
         cmd_name = args.command[0]
         cmd_name = aliases.get(cmd_name, cmd_name)
 
-        try:
-            command = parser.add_command(cmd_name)
-        except ImportError:
-            if spack.config.get('config:debug'):
-                raise
-            tty.die("Unknown command: %s" % args.command[0])
+        command = parser.add_command(cmd_name)
 
         # Re-parse with the proper sub-parser added.
         args, unknown = parser.parse_known_args()
diff --git a/lib/spack/spack/test/cmd/init_py_functions.py b/lib/spack/spack/test/cmd/init_py_functions.py
new file mode 100644
index 0000000000..2e63106283
--- /dev/null
+++ b/lib/spack/spack/test/cmd/init_py_functions.py
@@ -0,0 +1,29 @@
+# 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 pytest
+from spack.cmd import require_python_name, python_name, PythonNameError, \
+    require_cmd_name, cmd_name, CommandNameError
+
+
+def test_require_python_name():
+    """Python module names should not contain dashes---ensure that
+    require_python_name() raises the appropriate exception if one is
+    detected.
+    """
+    require_python_name("okey_dokey")
+    with pytest.raises(PythonNameError):
+        require_python_name("okey-dokey")
+    require_python_name(python_name("okey-dokey"))
+
+
+def test_require_cmd_name():
+    """By convention, Spack command names should contain dashes rather than
+    underscores---ensure that require_cmd_name() raises the appropriate
+    exception if underscores are detected.
+    """
+    require_cmd_name("okey-dokey")
+    with pytest.raises(CommandNameError):
+        require_cmd_name("okey_dokey")
+    require_cmd_name(cmd_name("okey_dokey"))
diff --git a/lib/spack/spack/test/cmd_extensions.py b/lib/spack/spack/test/cmd_extensions.py
index 486ac925cf..cb16609278 100644
--- a/lib/spack/spack/test/cmd_extensions.py
+++ b/lib/spack/spack/test/cmd_extensions.py
@@ -5,24 +5,71 @@
 
 import pytest
 
+import contextlib
+import os
 import sys
 
+import spack.cmd
 import spack.config
+import spack.extensions
 import spack.main
 
 
-@pytest.fixture()
-def extension_root(tmpdir):
-    root = tmpdir.mkdir('spack-testcommand')
-    root.ensure('testcommand', 'cmd', dir=True)
-    return root
+class Extension:
+    """Helper class to simplify the creation of simple command extension
+    directory structures with a conventional format for testing.
+    """
+    def __init__(self, name, root):
+        """Create a command extension.
 
+        Args:
+            name (str): The name of the command extension.
+            root (path object): The temporary root for the command extension
+                (e.g. from tmpdir.mkdir()).
+        """
+        self.name = name
+        self.pname = spack.cmd.python_name(name)
+        self.root = root
+        self.main = self.root.ensure(self.pname, dir=True)
+        self.cmd = self.main.ensure('cmd', dir=True)
 
-@pytest.fixture()
-def hello_world_cmd(extension_root):
-    """Simple extension command with code contained in a single file."""
-    hello = extension_root.ensure('testcommand', 'cmd', 'hello.py')
-    hello.write("""
+    def add_command(self, command_name, contents):
+        """Add a command to this command extension.
+
+        Args:
+            command_name (str): The name of the command.
+            contents (str): the desired contents of the new command module
+                file."""
+        spack.cmd.require_cmd_name(command_name)
+        python_name = spack.cmd.python_name(command_name)
+        cmd = self.cmd.ensure(python_name + '.py')
+        cmd.write(contents)
+
+
+@pytest.fixture(scope='function')
+def extension_creator(tmpdir, config):
+    """Create a basic extension command directory structure"""
+    @contextlib.contextmanager
+    def _ce(extension_name='testcommand'):
+        root = tmpdir.mkdir('spack-' + extension_name)
+        extension = Extension(extension_name, root)
+        with spack.config.override('config:extensions',
+                                   [str(extension.root)]):
+            yield extension
+    list_of_modules = list(sys.modules.keys())
+    try:
+        yield _ce
+    finally:
+        to_be_deleted = [x for x in sys.modules if x not in list_of_modules]
+        for module_name in to_be_deleted:
+            del sys.modules[module_name]
+
+
+@pytest.fixture(scope='function')
+def hello_world_extension(extension_creator):
+    """Create an extension with a hello-world command."""
+    with extension_creator() as extension:
+        extension.add_command('hello-world', """
 description = "hello world extension command"
 section = "test command"
 level = "long"
@@ -31,27 +78,33 @@ def setup_parser(subparser):
     pass
 
 
-def hello(parser, args):
+def hello_world(parser, args):
     print('Hello world!')
 """)
-    list_of_modules = list(sys.modules.keys())
-    with spack.config.override('config:extensions', [str(extension_root)]):
-        yield spack.main.SpackCommand('hello')
+        yield extension
 
-    to_be_deleted = [x for x in sys.modules if x not in list_of_modules]
-    for module_name in to_be_deleted:
-        del sys.modules[module_name]
 
+@pytest.fixture(scope='function')
+def hello_world_cmd(hello_world_extension):
+    """Create and return an invokable "hello-world" extension command."""
+    yield spack.main.SpackCommand('hello-world')
 
-@pytest.fixture()
-def hello_world_with_module_in_root(extension_root):
-    """Extension command with additional code in the root folder."""
-    extension_root.ensure('testcommand', '__init__.py')
-    command_root = extension_root.join('testcommand', 'cmd')
-    hello = command_root.ensure('hello.py')
-    hello.write("""
+
+@pytest.fixture(scope='function')
+def hello_world_with_module_in_root(extension_creator):
+    """Create a "hello-world" extension command with additional code in the
+    root folder.
+    """
+    @contextlib.contextmanager
+    def _hwwmir(extension_name=None):
+        with extension_creator(extension_name) \
+            if extension_name else \
+                extension_creator() as extension:
+            # Note that the namespace of the extension is derived from the
+            # fixture.
+            extension.add_command('hello', """
 # Test an absolute import
-from spack.extensions.testcommand.implementation import hello_world
+from spack.extensions.{ext_pname}.implementation import hello_world
 
 # Test a relative import
 from ..implementation import hello_folks
@@ -79,33 +132,143 @@ def hello(parser, args):
         hello_folks()
     elif args.subcommand == 'global':
         print(global_message)
-""")
-    implementation = extension_root.ensure('testcommand', 'implementation.py')
-    implementation.write("""
+""".format(ext_pname=extension.pname))
+
+            extension.main.ensure('__init__.py')
+            implementation \
+                = extension.main.ensure('implementation.py')
+            implementation.write("""
 def hello_world():
     print('Hello world!')
 
 def hello_folks():
     print('Hello folks!')
 """)
-    list_of_modules = list(sys.modules.keys())
-    with spack.config.override('config:extensions', [str(extension_root)]):
-        yield spack.main.SpackCommand('hello')
+            yield spack.main.SpackCommand('hello')
 
-    to_be_deleted = [x for x in sys.modules if x not in list_of_modules]
-    for module_name in to_be_deleted:
-        del sys.modules[module_name]
+    yield _hwwmir
 
 
 def test_simple_command_extension(hello_world_cmd):
+    """Basic test of a functioning command."""
     output = hello_world_cmd()
     assert 'Hello world!' in output
 
 
-def test_command_with_import(hello_world_with_module_in_root):
-    output = hello_world_with_module_in_root('world')
-    assert 'Hello world!' in output
-    output = hello_world_with_module_in_root('folks')
-    assert 'Hello folks!' in output
-    output = hello_world_with_module_in_root('global')
-    assert 'bar' in output
+def test_multi_extension_search(hello_world_extension, extension_creator):
+    """Ensure we can find an extension command even if it's not in the first
+    place we look.
+    """
+
+    with extension_creator('testcommand2'):
+        assert ('Hello world') in spack.main.SpackCommand('hello-world')()
+
+
+def test_duplicate_module_load(hello_world_cmd, capsys):
+    """Ensure duplicate module load attempts are successful.
+
+    The command module will already have been loaded once by the
+    hello_world_cmd fixture.
+    """
+    parser = spack.main.make_argument_parser()
+    args = []
+    hw_cmd = spack.cmd.get_command(hello_world_cmd.command_name)
+    hw_cmd(parser, args)
+    captured = capsys.readouterr()
+    assert captured == ('Hello world!\n', '')
+
+
+@pytest.mark.parametrize('extension_name',
+                         [None, 'hyphenated-extension'],
+                         ids=['simple', 'hyphenated_extension_name'])
+def test_command_with_import(extension_name, hello_world_with_module_in_root):
+    """Ensure we can write a functioning command with multiple imported
+    subcommands, including where the extension name contains a hyphen.
+    """
+    with hello_world_with_module_in_root(extension_name) as hello_world:
+        output = hello_world('world')
+        assert 'Hello world!' in output
+        output = hello_world('folks')
+        assert 'Hello folks!' in output
+        output = hello_world('global')
+        assert 'bar' in output
+
+
+def test_missing_command():
+    """Ensure that we raise the expected exception if the desired command is
+    not present.
+    """
+    with pytest.raises(spack.extensions.CommandNotFoundError):
+        spack.cmd.get_module("no-such-command")
+
+
+@pytest.mark.\
+    parametrize('extension_path,expected_exception',
+                [('/my/bad/extension',
+                  spack.extensions.ExtensionNamingError),
+                 ('', spack.extensions.ExtensionNamingError),
+                 ('/my/bad/spack--extra-hyphen',
+                  spack.extensions.ExtensionNamingError),
+                 ('/my/good/spack-extension',
+                  spack.extensions.CommandNotFoundError),
+                 ('/my/still/good/spack-extension/',
+                  spack.extensions.CommandNotFoundError),
+                 ('/my/spack-hyphenated-extension',
+                  spack.extensions.CommandNotFoundError)],
+                ids=['no_stem', 'vacuous', 'leading_hyphen',
+                     'basic_good', 'trailing_slash', 'hyphenated'])
+def test_extension_naming(extension_path, expected_exception, config):
+    """Ensure that we are correctly validating configured extension paths
+    for conformity with the rules: the basename should match
+    ``spack-<name>``; <name> may have embedded hyphens but not begin with one.
+    """
+    with spack.config.override('config:extensions', [extension_path]):
+        with pytest.raises(expected_exception):
+            spack.cmd.get_module("no-such-command")
+
+
+def test_missing_command_function(extension_creator, capsys):
+    """Ensure we die as expected if a command module does not have the
+    expected command function defined.
+    """
+    with extension_creator() as extension:
+        extension.\
+            add_command('bad-cmd',
+                        """\ndescription = "Empty command implementation"\n""")
+        with pytest.raises(SystemExit):
+            spack.cmd.get_module('bad-cmd')
+        capture = capsys.readouterr()
+        assert "must define function 'bad_cmd'." in capture[1]
+
+
+def test_get_command_paths(config):
+    """Exercise the construction of extension command search paths."""
+    extensions = ('extension-1', 'extension-2')
+    ext_paths = []
+    expected_cmd_paths = []
+    for ext in extensions:
+        ext_path = os.path.join('my', 'path', 'to', 'spack-' + ext)
+        ext_paths.append(ext_path)
+        expected_cmd_paths.append(os.path.join(ext_path,
+                                               spack.cmd.python_name(ext),
+                                               'cmd'))
+
+    with spack.config.override('config:extensions', ext_paths):
+        assert spack.extensions.get_command_paths() == expected_cmd_paths
+
+
+@pytest.mark.parametrize('command_name,contents,exception',
+                         [('bad-cmd', 'from oopsie.daisy import bad\n',
+                           ImportError),
+                          ('bad-cmd', """var = bad_function_call('blech')\n""",
+                           NameError),
+                          ('bad-cmd', ')\n', SyntaxError)],
+                         ids=['ImportError', 'NameError', 'SyntaxError'])
+def test_failing_command(command_name, contents, exception, extension_creator):
+    """Ensure that the configured command fails to import with the specified
+    error.
+    """
+    with extension_creator() as extension:
+        extension.add_command(command_name, contents)
+        with pytest.raises(exception):
+            spack.extensions.get_module(command_name)