Disallow vdeps in `packages.yaml` (#2699)

* Consolidate packages.yaml code to preferred_packages

* Add validation check and a test for packages.py parsing.

* flake8

7 files changed, 177 insertions, 104 deletions

diff --git a/lib/spack/spack/__init__.py b/lib/spack/spack/__init__.py
index 39a0b78f82..771fc7b32a 100644
--- a/lib/spack/spack/__init__.py
+++ b/lib/spack/spack/__init__.py
@@ -77,7 +77,7 @@ import spack.error
 import spack.config
 import spack.fetch_strategy
 from spack.file_cache import FileCache
-from spack.preferred_packages import PreferredPackages
+from spack.package_prefs import PreferredPackages
 from spack.abi import ABI
 from spack.concretize import DefaultConcretizer
 from spack.version import Version
@@ -99,11 +99,6 @@ except spack.error.SpackError, e:
     tty.die('while initializing Spack RepoPath:', e.message)
 
 
-# PreferredPackages controls preference sort order during concretization.
-# More preferred packages are sorted first.
-pkgsort = PreferredPackages()
-
-
 # Tests ABI compatibility between packages
 abi = ABI()
 
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 4690691174..36e8b30196 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -42,8 +42,7 @@ import spack.error
 from spack.version import *
 from functools import partial
 from itertools import chain
-from spack.config import *
-import spack.preferred_packages
+from spack.package_prefs import *
 
 
 class DefaultConcretizer(object):
@@ -64,11 +63,11 @@ class DefaultConcretizer(object):
                 raise UnsatisfiableProviderSpecError(providers[0], spec)
             spec_w_preferred_providers = find_spec(
                 spec,
-                lambda x: spack.pkgsort.spec_has_preferred_provider(
+                lambda x: pkgsort().spec_has_preferred_provider(
                     x.name, spec.name))
             if not spec_w_preferred_providers:
                 spec_w_preferred_providers = spec
-            provider_cmp = partial(spack.pkgsort.provider_compare,
+            provider_cmp = partial(pkgsort().provider_compare,
                                    spec_w_preferred_providers.name,
                                    spec.name)
             candidates = sorted(providers, cmp=provider_cmp)
@@ -169,8 +168,8 @@ class DefaultConcretizer(object):
 
         # ---------- Produce prioritized list of versions
         # Get list of preferences from packages.yaml
-        preferred = spack.pkgsort
-        # NOTE: spack.pkgsort == spack.preferred_packages.PreferredPackages()
+        preferred = pkgsort()
+        # NOTE: pkgsort() == spack.package_prefs.PreferredPackages()
 
         yaml_specs = [
             x[0] for x in
@@ -277,7 +276,7 @@ class DefaultConcretizer(object):
            the package specification.
         """
         changed = False
-        preferred_variants = spack.pkgsort.spec_preferred_variants(
+        preferred_variants = pkgsort().spec_preferred_variants(
             spec.package_class.name)
         for name, variant in spec.package_class.variants.items():
             if name not in spec.variants:
@@ -342,7 +341,7 @@ class DefaultConcretizer(object):
         compiler_list = all_compilers if not other_compiler else \
             spack.compilers.find(other_compiler)
         cmp_compilers = partial(
-            spack.pkgsort.compiler_compare, other_spec.name)
+            pkgsort().compiler_compare, other_spec.name)
         matches = sorted(compiler_list, cmp=cmp_compilers)
         if not matches:
             arch = spec.architecture
@@ -467,41 +466,6 @@ def find_spec(spec, condition):
     return None   # Nothing matched the condition.
 
 
-def cmp_specs(lhs, rhs):
-    # Package name sort order is not configurable, always goes alphabetical
-    if lhs.name != rhs.name:
-        return cmp(lhs.name, rhs.name)
-
-    # Package version is second in compare order
-    pkgname = lhs.name
-    if lhs.versions != rhs.versions:
-        return spack.pkgsort.version_compare(
-            pkgname, lhs.versions, rhs.versions)
-
-    # Compiler is third
-    if lhs.compiler != rhs.compiler:
-        return spack.pkgsort.compiler_compare(
-            pkgname, lhs.compiler, rhs.compiler)
-
-    # Variants
-    if lhs.variants != rhs.variants:
-        return spack.pkgsort.variant_compare(
-            pkgname, lhs.variants, rhs.variants)
-
-    # Architecture
-    if lhs.architecture != rhs.architecture:
-        return spack.pkgsort.architecture_compare(
-            pkgname, lhs.architecture, rhs.architecture)
-
-    # Dependency is not configurable
-    lhash, rhash = hash(lhs), hash(rhs)
-    if lhash != rhash:
-        return -1 if lhash < rhash else 1
-
-    # Equal specs
-    return 0
-
-
 class UnavailableCompilerVersionError(spack.error.SpackError):
 
     """Raised when there is no available compiler that satisfies a
diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index 989b3da169..56c6421457 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -199,6 +199,7 @@ class ConfigScope(object):
     def __repr__(self):
         return '<ConfigScope: %s: %s>' % (self.name, self.path)
 
+
 #
 # Below are configuration scopes.
 #
@@ -458,54 +459,6 @@ def print_section(section):
         raise ConfigError("Error reading configuration: %s" % section)
 
 
-def spec_externals(spec):
-    """Return a list of external specs (with external directory path filled in),
-       one for each known external installation."""
-    # break circular import.
-    from spack.build_environment import get_path_from_module
-
-    allpkgs = get_config('packages')
-    name = spec.name
-
-    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 pkg_paths.iteritems():
-        if not path:
-            # skip entries without paths (avoid creating extra Specs)
-            continue
-
-        external_spec = spack.spec.Spec(external_spec, external=path)
-        if external_spec.satisfies(spec):
-            external_specs.append(external_spec)
-
-    for external_spec, module in pkg_modules.iteritems():
-        if not module:
-            continue
-
-        path = get_path_from_module(module)
-
-        external_spec = spack.spec.Spec(
-            external_spec, external=path, external_module=module)
-        if external_spec.satisfies(spec):
-            external_specs.append(external_spec)
-
-    return external_specs
-
-
-def is_spec_buildable(spec):
-    """Return true if the spec pkgspec is configured as buildable"""
-    allpkgs = get_config('packages')
-    if spec.name not in allpkgs:
-        return True
-    if 'buildable' not in allpkgs[spec.name]:
-        return True
-    return allpkgs[spec.name]['buildable']
-
-
 class ConfigError(SpackError):
     pass
 
diff --git a/lib/spack/spack/preferred_packages.py b/lib/spack/spack/package_prefs.py
index dc6cb3e921..190647bb81 100644
--- a/lib/spack/spack/preferred_packages.py
+++ b/lib/spack/spack/package_prefs.py
@@ -24,12 +24,33 @@
 ##############################################################################
 
 import spack
+import spack.error
 from spack.version import *
 
 
+def get_packages_config():
+    """Wrapper around get_packages_config() to validate semantics."""
+    config = spack.config.get_config('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.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:", *errors)
+
+    return config
+
+
 class PreferredPackages(object):
     def __init__(self):
-        self.preferred = spack.config.get_config('packages')
+        self.preferred = get_packages_config()
         self._spec_for_pkgname_cache = {}
 
     # Given a package name, sort component (e.g, version, compiler, ...), and
@@ -194,3 +215,100 @@ class PreferredPackages(object):
            pkgname. One compiler is less-than another if it is preferred over
            the other."""
         return self._spec_compare(pkgname, 'compiler', a, b, False, None)
+
+
+def spec_externals(spec):
+    """Return a list of external specs (with external directory path filled in),
+       one for each known external installation."""
+    # break circular import.
+    from spack.build_environment import get_path_from_module
+
+    allpkgs = get_packages_config()
+    name = spec.name
+
+    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 pkg_paths.iteritems():
+        if not path:
+            # skip entries without paths (avoid creating extra Specs)
+            continue
+
+        external_spec = spack.spec.Spec(external_spec, external=path)
+        if external_spec.satisfies(spec):
+            external_specs.append(external_spec)
+
+    for external_spec, module in pkg_modules.iteritems():
+        if not module:
+            continue
+
+        path = get_path_from_module(module)
+
+        external_spec = spack.spec.Spec(
+            external_spec, external=path, external_module=module)
+        if external_spec.satisfies(spec):
+            external_specs.append(external_spec)
+
+    return external_specs
+
+
+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']
+
+
+def cmp_specs(lhs, rhs):
+    # Package name sort order is not configurable, always goes alphabetical
+    if lhs.name != rhs.name:
+        return cmp(lhs.name, rhs.name)
+
+    # Package version is second in compare order
+    pkgname = lhs.name
+    if lhs.versions != rhs.versions:
+        return pkgsort().version_compare(
+            pkgname, lhs.versions, rhs.versions)
+
+    # Compiler is third
+    if lhs.compiler != rhs.compiler:
+        return pkgsort().compiler_compare(
+            pkgname, lhs.compiler, rhs.compiler)
+
+    # Variants
+    if lhs.variants != rhs.variants:
+        return pkgsort().variant_compare(
+            pkgname, lhs.variants, rhs.variants)
+
+    # Architecture
+    if lhs.architecture != rhs.architecture:
+        return pkgsort().architecture_compare(
+            pkgname, lhs.architecture, rhs.architecture)
+
+    # Dependency is not configurable
+    lhash, rhash = hash(lhs), hash(rhs)
+    if lhash != rhash:
+        return -1 if lhash < rhash else 1
+
+    # Equal specs
+    return 0
+
+
+_pkgsort = None
+
+
+def pkgsort():
+    global _pkgsort
+    if _pkgsort is None:
+        _pkgsort = PreferredPackages()
+    return _pkgsort
+
+
+class VirtualInPackagesYAMLError(spack.error.SpackError):
+    """Raised when a disallowed virtual is found in packages.yaml"""
diff --git a/lib/spack/spack/repository.py b/lib/spack/spack/repository.py
index d77700c01f..1536ecb0e6 100644
--- a/lib/spack/spack/repository.py
+++ b/lib/spack/spack/repository.py
@@ -337,8 +337,16 @@ class RepoPath(object):
         return self.repo_for_pkg(pkg_name).filename_for_package_name(pkg_name)
 
     def exists(self, pkg_name):
+        """Whether package with the give name exists in the path's repos.
+
+        Note that virtual packages do not "exist".
+        """
         return any(repo.exists(pkg_name) for repo in self.repos)
 
+    def is_virtual(self, pkg_name):
+        """True if the package with this name is virtual, False otherwise."""
+        return pkg_name in self.provider_index
+
     def __contains__(self, pkg_name):
         return self.exists(pkg_name)
 
@@ -772,6 +780,10 @@ class Repo(object):
         filename = self.filename_for_package_name(pkg_name)
         return os.path.exists(filename)
 
+    def is_virtual(self, pkg_name):
+        """True if the package with this name is virtual, False otherwise."""
+        return self.provider_index.contains(pkg_name)
+
     def _get_pkg_module(self, pkg_name):
         """Create a module for a particular package.
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 5291a7c9d0..c96ef1dc30 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2546,6 +2546,8 @@ class Spec(object):
         return ''.join("^" + dep.format() for dep in self.sorted_deps())
 
     def __cmp__(self, other):
+        from package_prefs import pkgsort
+
         # Package name sort order is not configurable, always goes alphabetical
         if self.name != other.name:
             return cmp(self.name, other.name)
@@ -2553,22 +2555,22 @@ class Spec(object):
         # Package version is second in compare order
         pkgname = self.name
         if self.versions != other.versions:
-            return spack.pkgsort.version_compare(
+            return pkgsort().version_compare(
                 pkgname, self.versions, other.versions)
 
         # Compiler is third
         if self.compiler != other.compiler:
-            return spack.pkgsort.compiler_compare(
+            return pkgsort().compiler_compare(
                 pkgname, self.compiler, other.compiler)
 
         # Variants
         if self.variants != other.variants:
-            return spack.pkgsort.variant_compare(
+            return pkgsort().variant_compare(
                 pkgname, self.variants, other.variants)
 
         # Target
         if self.architecture != other.architecture:
-            return spack.pkgsort.architecture_compare(
+            return pkgsort().architecture_compare(
                 pkgname, self.architecture, other.architecture)
 
         # Dependency is not configurable
diff --git a/lib/spack/spack/test/concretize_preferences.py b/lib/spack/spack/test/concretize_preferences.py
index 21d457d2e0..b95fc83010 100644
--- a/lib/spack/spack/test/concretize_preferences.py
+++ b/lib/spack/spack/test/concretize_preferences.py
@@ -25,7 +25,9 @@
 import pytest
 
 import spack
+import spack.util.spack_yaml as syaml
 from spack.spec import Spec
+from spack.package_prefs import PreferredPackages
 
 
 @pytest.fixture()
@@ -39,7 +41,7 @@ def concretize_scope(config, tmpdir):
     # This is kind of weird, but that's how config scopes are
     # set in ConfigScope.__init__
     spack.config.config_scopes.pop('concretize')
-    spack.pkgsort = spack.PreferredPackages()
+    spack.package_prefs._pkgsort = PreferredPackages()
 
 
 def concretize(abstract_spec):
@@ -50,7 +52,7 @@ def update_packages(pkgname, section, value):
     """Update config and reread package list"""
     conf = {pkgname: {section: value}}
     spack.config.update_config('packages', conf, 'concretize')
-    spack.pkgsort = spack.PreferredPackages()
+    spack.package_prefs._pkgsort = PreferredPackages()
 
 
 def assert_variant_values(spec, **variants):
@@ -114,3 +116,30 @@ class TestConcretizePreferences(object):
         spec = Spec('builtin.mock.develop-test')
         spec.concretize()
         assert spec.version == spack.spec.Version('0.2.15')
+
+    def test_no_virtuals_in_packages_yaml(self):
+        """Verify that virtuals are not allowed in packages.yaml."""
+
+        # set up a packages.yaml file with a vdep as a key.  We use
+        # syaml.load here to make sure source lines in the config are
+        # attached to parsed strings, as the error message uses them.
+        conf = syaml.load("""mpi:
+    paths:
+      mpi-with-lapack@2.1: /path/to/lapack
+""")
+        spack.config.update_config('packages', conf, '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("""all:
+        variants: [+mpi]
+""")
+        spack.config.update_config('packages', conf, 'concretize')
+
+        # should be no error for 'all':
+        spack.package_prefs._pkgsort = PreferredPackages()
+        spack.package_prefs.get_packages_config()