packages.yaml: allow virtuals to specify buildable: false (#14934)

Currently, to force Spack to use an external MPI, you have to specify `buildable: False`
for every MPI provider in Spack in your packages.yaml file. This is both tedious and
fragile, as new MPI providers can be added and break your workflow when you do a
git pull.

This PR allows you to specify an entire virtual dependency as non-buildable, and
specify particular implementations to be built:

```
packages:
all:
    providers:
        mpi: [mpich]
mpi:
    buildable: false
    paths:
        mpich@3.2 %gcc@7.3.0: /usr/packages/mpich-3.2-gcc-7.3.0
```
will force all Spack builds to use the specified `mpich` install.

9 files changed, 155 insertions, 153 deletions

diff --git a/lib/spack/docs/build_settings.rst b/lib/spack/docs/build_settings.rst
index b141f2b717..cfd850af28 100644
--- a/lib/spack/docs/build_settings.rst
+++ b/lib/spack/docs/build_settings.rst
@@ -124,6 +124,39 @@ The ``buildable`` does not need to be paired with external packages.
 It could also be used alone to forbid packages that may be
 buggy or otherwise undesirable.
 
+Virtual packages in Spack can also be specified as not buildable, and
+external implementations can be provided. In the example above,
+OpenMPI is configured as not buildable, but Spack will often prefer
+other MPI implementations over the externally available OpenMPI. Spack
+can be configured with every MPI provider not buildable individually,
+but more conveniently:
+
+.. code-block:: yaml
+
+   packages:
+     mpi:
+       buildable: False
+     openmpi:
+       paths:
+         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64: /opt/openmpi-1.4.3
+         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug: /opt/openmpi-1.4.3-debug
+         openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64: /opt/openmpi-1.6.5-intel
+
+Implementations can also be listed immediately under the virtual they provide:
+
+.. code-block:: yaml
+
+   packages:
+     mpi:
+       buildable: False
+         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64: /opt/openmpi-1.4.3
+         openmpi@1.4.3%gcc@4.4.7 arch=linux-debian7-x86_64+debug: /opt/openmpi-1.4.3-debug
+         openmpi@1.6.5%intel@10.1 arch=linux-debian7-x86_64: /opt/openmpi-1.6.5-intel
+         mpich@3.3 %clang@9.0.0 arch=linux-debian7-x86_64: /opt/mpich-3.3-intel
+
+Spack can then use any of the listed external implementations of MPI
+to satisfy a dependency, and will choose depending on the compiler and
+architecture.
 
 .. _concretization-preferences:
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 9156d7d0dd..b8ded0364b 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -1036,6 +1036,14 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         )
 
     @property
+    def virtuals_provided(self):
+        """
+        virtual packages provided by this package with its spec
+        """
+        return [vspec for vspec, constraints in self.provided.items()
+                if any(self.spec.satisfies(c) for c in constraints)]
+
+    @property
     def installed(self):
         """Installation status of a package.
 
diff --git a/lib/spack/spack/package_prefs.py b/lib/spack/spack/package_prefs.py
index 2801a6d123..0158c7063a 100644
--- a/lib/spack/spack/package_prefs.py
+++ b/lib/spack/spack/package_prefs.py
@@ -6,7 +6,6 @@
 import stat
 
 from six import string_types
-from six import iteritems
 
 import spack.repo
 import spack.error
@@ -23,27 +22,6 @@ def _spec_type(component):
     return _lesser_spec_types.get(component, spack.spec.Spec)
 
 
-def get_packages_config():
-    """Wrapper around get_packages_config() to validate semantics."""
-    config = spack.config.get('packages')
-
-    # Get a list of virtuals from packages.yaml.  Note that because we
-    # check spack.repo, this collects virtuals that are actually provided
-    # by sometihng, not just packages/names that don't exist.
-    # So, this won't include, e.g., 'all'.
-    virtuals = [(pkg_name, pkg_name._start_mark) for pkg_name in config
-                if spack.repo.path.is_virtual(pkg_name)]
-
-    # die if there are virtuals in `packages.py`
-    if virtuals:
-        errors = ["%s: %s" % (line_info, name) for name, line_info in virtuals]
-        raise VirtualInPackagesYAMLError(
-            "packages.yaml entries cannot be virtual packages:",
-            '\n'.join(errors))
-
-    return config
-
-
 class PackagePrefs(object):
     """Defines the sort order for a set of specs.
 
@@ -116,7 +94,7 @@ class PackagePrefs(object):
             pkglist.append('all')
 
         for pkg in pkglist:
-            pkg_entry = get_packages_config().get(pkg)
+            pkg_entry = spack.config.get('packages').get(pkg)
             if not pkg_entry:
                 continue
 
@@ -160,7 +138,8 @@ class PackagePrefs(object):
     def preferred_variants(cls, pkg_name):
         """Return a VariantMap of preferred variants/values for a spec."""
         for pkg in (pkg_name, 'all'):
-            variants = get_packages_config().get(pkg, {}).get('variants', '')
+            variants = spack.config.get('packages').get(pkg, {}).get(
+                'variants', '')
             if variants:
                 break
 
@@ -181,33 +160,29 @@ def spec_externals(spec):
     # break circular import.
     from spack.util.module_cmd import get_path_from_module # NOQA: ignore=F401
 
-    allpkgs = get_packages_config()
-    name = spec.name
+    allpkgs = spack.config.get('packages')
+    names = set([spec.name])
+    names |= set(vspec.name for vspec in spec.package.virtuals_provided)
 
     external_specs = []
-    pkg_paths = allpkgs.get(name, {}).get('paths', None)
-    pkg_modules = allpkgs.get(name, {}).get('modules', None)
-    if (not pkg_paths) and (not pkg_modules):
-        return []
-
-    for external_spec, path in iteritems(pkg_paths):
-        if not path:
-            # skip entries without paths (avoid creating extra Specs)
+    for name in names:
+        pkg_config = allpkgs.get(name, {})
+        pkg_paths = pkg_config.get('paths', {})
+        pkg_modules = pkg_config.get('modules', {})
+        if (not pkg_paths) and (not pkg_modules):
             continue
 
-        external_spec = spack.spec.Spec(external_spec,
-                                        external_path=canonicalize_path(path))
-        if external_spec.satisfies(spec):
-            external_specs.append(external_spec)
-
-    for external_spec, module in iteritems(pkg_modules):
-        if not module:
-            continue
+        for external_spec, path in pkg_paths.items():
+            external_spec = spack.spec.Spec(
+                external_spec, external_path=canonicalize_path(path))
+            if external_spec.satisfies(spec):
+                external_specs.append(external_spec)
 
-        external_spec = spack.spec.Spec(
-            external_spec, external_module=module)
-        if external_spec.satisfies(spec):
-            external_specs.append(external_spec)
+        for external_spec, module in pkg_modules.items():
+            external_spec = spack.spec.Spec(
+                external_spec, external_module=module)
+            if external_spec.satisfies(spec):
+                external_specs.append(external_spec)
 
     # defensively copy returned specs
     return [s.copy() for s in external_specs]
@@ -215,12 +190,11 @@ def spec_externals(spec):
 
 def is_spec_buildable(spec):
     """Return true if the spec pkgspec is configured as buildable"""
-    allpkgs = get_packages_config()
-    if spec.name not in allpkgs:
-        return True
-    if 'buildable' not in allpkgs[spec.name]:
-        return True
-    return allpkgs[spec.name]['buildable']
+    allpkgs = spack.config.get('packages')
+    do_not_build = [name for name, entry in allpkgs.items()
+                    if not entry.get('buildable', True)]
+    return not (spec.name in do_not_build or
+                any(spec.package.provides(name) for name in do_not_build))
 
 
 def get_package_dir_permissions(spec):
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index 81b5360869..922d5a11d8 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -185,36 +185,39 @@ class TestConcretizePreferences(object):
         spec.concretize()
         assert spec.version == Version('0.2.15.develop')
 
-    def test_no_virtuals_in_packages_yaml(self):
-        """Verify that virtuals are not allowed in packages.yaml."""
+    def test_external_mpi(self):
+        # make sure this doesn't give us an external first.
+        spec = Spec('mpi')
+        spec.concretize()
+        assert not spec['mpi'].external
 
-        # set up a packages.yaml file with a vdep as a key.  We use
-        # syaml.load_config here to make sure source lines in the config are
-        # attached to parsed strings, as the error message uses them.
+        # load config
         conf = syaml.load_config("""\
-mpi:
+all:
+    providers:
+        mpi: [mpich]
+mpich:
+    buildable: false
     paths:
-      mpi-with-lapack@2.1: /path/to/lapack
+        mpich@3.0.4: /dummy/path
 """)
         spack.config.set('packages', conf, scope='concretize')
 
-        # now when we get the packages.yaml config, there should be an error
-        with pytest.raises(spack.package_prefs.VirtualInPackagesYAMLError):
-            spack.package_prefs.get_packages_config()
-
-    def test_all_is_not_a_virtual(self):
-        """Verify that `all` is allowed in packages.yaml."""
-        conf = syaml.load_config("""\
-all:
-        variants: [+mpi]
-""")
-        spack.config.set('packages', conf, scope='concretize')
+        # ensure that once config is in place, external is used
+        spec = Spec('mpi')
+        spec.concretize()
+        assert spec['mpich'].external_path == '/dummy/path'
 
-        # should be no error for 'all':
-        spack.package_prefs.get_packages_config()
+    def test_external_module(self, monkeypatch):
+        """Test that packages can find externals specified by module
 
-    def test_external_mpi(self):
+        The specific code for parsing the module is tested elsewhere.
+        This just tests that the preference is accounted for"""
         # make sure this doesn't give us an external first.
+        def mock_module(cmd, module):
+            return 'prepend-path PATH /dummy/path'
+        monkeypatch.setattr(spack.util.module_cmd, 'module', mock_module)
+
         spec = Spec('mpi')
         spec.concretize()
         assert not spec['mpi'].external
@@ -224,10 +227,10 @@ all:
 all:
     providers:
         mpi: [mpich]
-mpich:
+mpi:
     buildable: false
-    paths:
-        mpich@3.0.4: /dummy/path
+    modules:
+        mpich@3.0.4: dummy
 """)
         spack.config.set('packages', conf, scope='concretize')
 
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index a186463907..8912c0219b 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -1040,6 +1040,13 @@ class MockPackage(object):
         self.conflicts = {}
         self.patches = {}
 
+    def provides(self, vname):
+        return vname in self.provided
+
+    @property
+    def virtuals_provided(self):
+        return [v.name for v, c in self.provided]
+
 
 class MockPackageMultiRepo(object):
     def __init__(self, packages):
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index 570c329b71..05cf46dc20 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -52,52 +52,52 @@ def check_mirror():
         mirror_root = os.path.join(stage.path, 'test-mirror')
         # register mirror with spack config
         mirrors = {'spack-mirror-test': 'file://' + mirror_root}
-        spack.config.set('mirrors', mirrors)
-        with spack.config.override('config:checksum', False):
-            specs = [Spec(x).concretized() for x in repos]
-            spack.mirror.create(mirror_root, specs)
-
-        # Stage directory exists
-        assert os.path.isdir(mirror_root)
-
-        for spec in specs:
-            fetcher = spec.package.fetcher[0]
-            per_package_ref = os.path.join(
-                spec.name, '-'.join([spec.name, str(spec.version)]))
-            mirror_paths = spack.mirror.mirror_archive_paths(
-                fetcher,
-                per_package_ref)
-            expected_path = os.path.join(
-                mirror_root, mirror_paths.storage_path)
-            assert os.path.exists(expected_path)
-
-        # Now try to fetch each package.
-        for name, mock_repo in repos.items():
-            spec = Spec(name).concretized()
-            pkg = spec.package
-
+        with spack.config.override('mirrors', mirrors):
             with spack.config.override('config:checksum', False):
-                with pkg.stage:
-                    pkg.do_stage(mirror_only=True)
-
-                    # Compare the original repo with the expanded archive
-                    original_path = mock_repo.path
-                    if 'svn' in name:
-                        # have to check out the svn repo to compare.
-                        original_path = os.path.join(
-                            mock_repo.path, 'checked_out')
-
-                        svn = which('svn', required=True)
-                        svn('checkout', mock_repo.url, original_path)
-
-                    dcmp = filecmp.dircmp(
-                        original_path, pkg.stage.source_path)
-
-                    # make sure there are no new files in the expanded
-                    # tarball
-                    assert not dcmp.right_only
-                    # and that all original files are present.
-                    assert all(l in exclude for l in dcmp.left_only)
+                specs = [Spec(x).concretized() for x in repos]
+                spack.mirror.create(mirror_root, specs)
+
+            # Stage directory exists
+            assert os.path.isdir(mirror_root)
+
+            for spec in specs:
+                fetcher = spec.package.fetcher[0]
+                per_package_ref = os.path.join(
+                    spec.name, '-'.join([spec.name, str(spec.version)]))
+                mirror_paths = spack.mirror.mirror_archive_paths(
+                    fetcher,
+                    per_package_ref)
+                expected_path = os.path.join(
+                    mirror_root, mirror_paths.storage_path)
+                assert os.path.exists(expected_path)
+
+            # Now try to fetch each package.
+            for name, mock_repo in repos.items():
+                spec = Spec(name).concretized()
+                pkg = spec.package
+
+                with spack.config.override('config:checksum', False):
+                    with pkg.stage:
+                        pkg.do_stage(mirror_only=True)
+
+                        # Compare the original repo with the expanded archive
+                        original_path = mock_repo.path
+                        if 'svn' in name:
+                            # have to check out the svn repo to compare.
+                            original_path = os.path.join(
+                                mock_repo.path, 'checked_out')
+
+                            svn = which('svn', required=True)
+                            svn('checkout', mock_repo.url, original_path)
+
+                        dcmp = filecmp.dircmp(
+                            original_path, pkg.stage.source_path)
+
+                        # make sure there are no new files in the expanded
+                        # tarball
+                        assert not dcmp.right_only
+                        # and that all original files are present.
+                        assert all(l in exclude for l in dcmp.left_only)
 
 
 def test_url_mirror(mock_archive):
diff --git a/lib/spack/spack/test/module_parsing.py b/lib/spack/spack/test/module_parsing.py
index bbe18b1ad0..0bf485913f 100644
--- a/lib/spack/spack/test/module_parsing.py
+++ b/lib/spack/spack/test/module_parsing.py
@@ -20,28 +20,11 @@ test_module_lines = ['prepend-path LD_LIBRARY_PATH /path/to/lib',
                      'setenv LDFLAGS -L/path/to/lib',
                      'prepend-path PATH /path/to/bin']
 
+_test_template = "'. %s 2>&1' % args[1]"
 
-@pytest.fixture
-def module_function_test_mode():
-    old_mode = spack.util.module_cmd._test_mode
-    spack.util.module_cmd._test_mode = True
 
-    yield
-
-    spack.util.module_cmd._test_mode = old_mode
-
-
-@pytest.fixture
-def save_module_func():
-    old_func = spack.util.module_cmd.module
-
-    yield
-
-    spack.util.module_cmd.module = old_func
-
-
-def test_module_function_change_env(tmpdir, working_env,
-                                    module_function_test_mode):
+def test_module_function_change_env(tmpdir, working_env, monkeypatch):
+    monkeypatch.setattr(spack.util.module_cmd, '_cmd_template', _test_template)
     src_file = str(tmpdir.join('src_me'))
     with open(src_file, 'w') as f:
         f.write('export TEST_MODULE_ENV_VAR=TEST_SUCCESS\n')
@@ -53,7 +36,8 @@ def test_module_function_change_env(tmpdir, working_env,
     assert os.environ['NOT_AFFECTED'] == "NOT_AFFECTED"
 
 
-def test_module_function_no_change(tmpdir, module_function_test_mode):
+def test_module_function_no_change(tmpdir, monkeypatch):
+    monkeypatch.setattr(spack.util.module_cmd, '_cmd_template', _test_template)
     src_file = str(tmpdir.join('src_me'))
     with open(src_file, 'w') as f:
         f.write('echo TEST_MODULE_FUNCTION_PRINT')
@@ -65,11 +49,11 @@ def test_module_function_no_change(tmpdir, module_function_test_mode):
     assert os.environ == old_env
 
 
-def test_get_path_from_module_faked(save_module_func):
+def test_get_path_from_module_faked(monkeypatch):
     for line in test_module_lines:
         def fake_module(*args):
             return line
-        spack.util.module_cmd.module = fake_module
+        monkeypatch.setattr(spack.util.module_cmd, 'module', fake_module)
 
         path = get_path_from_module('mod')
         assert path == '/path/to'
diff --git a/lib/spack/spack/test/spec_dag.py b/lib/spack/spack/test/spec_dag.py
index 25917f9424..e031f02c25 100644
--- a/lib/spack/spack/test/spec_dag.py
+++ b/lib/spack/spack/test/spec_dag.py
@@ -387,7 +387,6 @@ class TestSpecDag(object):
         with pytest.raises(spack.spec.UnsatisfiableArchitectureSpecError):
             spec.normalize()
 
-    @pytest.mark.usefixtures('config')
     def test_invalid_dep(self):
         spec = Spec('libelf ^mpich')
         with pytest.raises(spack.spec.InvalidDependencyError):
@@ -602,7 +601,6 @@ class TestSpecDag(object):
         copy_ids = set(id(s) for s in copy.traverse())
         assert not orig_ids.intersection(copy_ids)
 
-    @pytest.mark.usefixtures('config')
     def test_copy_concretized(self):
         orig = Spec('mpileaks')
         orig.concretize()
diff --git a/lib/spack/spack/util/module_cmd.py b/lib/spack/spack/util/module_cmd.py
index 0edf7e6102..74790156ae 100644
--- a/lib/spack/spack/util/module_cmd.py
+++ b/lib/spack/spack/util/module_cmd.py
@@ -19,16 +19,11 @@ import llnl.util.tty as tty
 # If we need another option that changes the environment, add it here.
 module_change_commands = ['load', 'swap', 'unload', 'purge', 'use', 'unuse']
 py_cmd = "'import os;import json;print(json.dumps(dict(os.environ)))'"
-
-# This is just to enable testing. I hate it but we can't find a better way
-_test_mode = False
+_cmd_template = "'module ' + ' '.join(args) + ' 2>&1'"
 
 
 def module(*args):
-    module_cmd = 'module ' + ' '.join(args) + ' 2>&1'
-    if _test_mode:
-        tty.warn('module function operating in test mode')
-        module_cmd = ". %s 2>&1" % args[1]
+    module_cmd = eval(_cmd_template)  # So we can monkeypatch for testing
     if args[0] in module_change_commands:
         # Do the module manipulation, then output the environment in JSON
         # and read the JSON back in the parent process to update os.environ