spack load exits with 1 if module does not exist or is not installed

fixes #2215
fixes #2570
fixes #6676
fixes #7281
closes #3827

This PR reverts the use of `spack module loads` in favor of
`spack module find` when loading module files via Spack. After this PR
`spack load` will accept a single spec at a time, and will be able
to interpret correctly the `--dependencies` option.

3 files changed, 76 insertions, 7 deletions

diff --git a/lib/spack/spack/cmd/common/modules.py b/lib/spack/spack/cmd/common/modules.py
index 964d89d668..11ee9cdb5e 100644
--- a/lib/spack/spack/cmd/common/modules.py
+++ b/lib/spack/spack/cmd/common/modules.py
@@ -71,7 +71,9 @@ def setup_parser(subparser):
         help='display full path to module file',
         action='store_true'
     )
-    arguments.add_common_arguments(find_parser, ['constraint'])
+    arguments.add_common_arguments(
+        find_parser, ['constraint', 'recurse_dependencies']
+    )
 
     rm_parser = sp.add_parser('rm', help='remove module files')
     arguments.add_common_arguments(
@@ -180,17 +182,36 @@ def find(module_type, specs, args):
     spec = one_spec_or_raise(specs)
 
     # Check if the module file is present
-    writer = spack.modules.module_types[module_type](spec)
-    if not os.path.isfile(writer.layout.filename):
+    def module_exists(spec):
+        writer = spack.modules.module_types[module_type](spec)
+        return os.path.isfile(writer.layout.filename)
+
+    if not module_exists(spec):
         msg = 'Even though {1} is installed, '
         msg += 'no {0} module has been generated for it.'
         tty.die(msg.format(module_type, spec))
 
+    # Check if we want to recurse and load all dependencies. In that case
+    # modify the list of specs adding all the dependencies in post order
+    if args.recurse_dependencies:
+        specs = [
+            item for item in spec.traverse(order='post', cover='nodes')
+            if module_exists(item)
+        ]
+
     # ... and if it is print its use name or full-path if requested
-    if args.full_path:
-        print(writer.layout.filename)
-    else:
-        print(writer.layout.use_name)
+    def module_str(specs):
+        modules = []
+        for x in specs:
+            writer = spack.modules.module_types[module_type](x)
+            if args.full_path:
+                modules.append(writer.layout.filename)
+            else:
+                modules.append(writer.layout.use_name)
+
+        return ' '.join(modules)
+
+    print(module_str(specs))
 
 
 def rm(module_type, specs, args):
diff --git a/lib/spack/spack/test/cmd/tcl.py b/lib/spack/spack/test/cmd/tcl.py
index ba3e322b49..d5deffce6b 100644
--- a/lib/spack/spack/test/cmd/tcl.py
+++ b/lib/spack/spack/test/cmd/tcl.py
@@ -93,3 +93,43 @@ def test_remove_and_add_tcl(database, parser):
 def test_find(database, cli_args):
     """Tests 'spack tcl find' under a few common scenarios."""
     tcl(*(['find'] + cli_args))
+
+
+@pytest.mark.db
+@pytest.mark.usefixtures('database')
+@pytest.mark.regression('2215')
+def test_find_fails_on_multiple_matches():
+    # As we installed multiple versions of mpileaks, the command will
+    # fail because of multiple matches
+    out = tcl('find', 'mpileaks', fail_on_error=False)
+    assert tcl.returncode == 1
+    assert 'matches multiple packages' in out
+
+    # Passing multiple packages from the command line also results in the
+    # same failure
+    out = tcl('find', 'mpileaks ^mpich', 'libelf', fail_on_error=False)
+    assert tcl.returncode == 1
+    assert 'matches multiple packages' in out
+
+
+@pytest.mark.db
+@pytest.mark.usefixtures('database')
+@pytest.mark.regression('2570')
+def test_find_fails_on_non_existing_packages():
+    # Another way the command might fail is if the package does not exist
+    out = tcl('find', 'doesnotexist', fail_on_error=False)
+    assert tcl.returncode == 1
+    assert 'matches no package' in out
+
+
+@pytest.mark.db
+@pytest.mark.usefixtures('database')
+def test_find_recursive():
+    # If we call find without options it should return only one module
+    out = tcl('find', 'mpileaks ^zmpi')
+    assert len(out.split()) == 1
+
+    # If instead we call it with the recursive option the length should
+    # be greater
+    out = tcl('find', '-r', 'mpileaks ^zmpi')
+    assert len(out.split()) > 1
diff --git a/share/spack/setup-env.sh b/share/spack/setup-env.sh
index 5774e8f762..6e5ab2b897 100755
--- a/share/spack/setup-env.sh
+++ b/share/spack/setup-env.sh
@@ -123,18 +123,26 @@ function spack {
                 "use")
                     if _sp_full_spec=$(command spack $_sp_flags dotkit find $_sp_subcommand_args "${_sp_spec[@]}"); then
                         use $_sp_module_args $_sp_full_spec
+                    else
+                        $(exit 1)
                     fi ;;
                 "unuse")
                     if _sp_full_spec=$(command spack $_sp_flags dotkit find $_sp_subcommand_args "${_sp_spec[@]}"); then
                         unuse $_sp_module_args $_sp_full_spec
+                    else
+                        $(exit 1)
                     fi ;;
                 "load")
                     if _sp_full_spec=$(command spack $_sp_flags tcl find $_sp_subcommand_args "${_sp_spec[@]}"); then
                         module load $_sp_module_args $_sp_full_spec
+                    else
+                        $(exit 1)
                     fi ;;
                 "unload")
                     if _sp_full_spec=$(command spack $_sp_flags tcl find $_sp_subcommand_args "${_sp_spec[@]}"); then
                         module unload $_sp_module_args $_sp_full_spec
+                    else
+                        $(exit 1)
                     fi ;;
             esac
             ;;