SPACK-14: Bugfix in Spec.normalize()

- Normalize now updates the provider index as it addes package dependencies.
- Fixes problem where this breaks:

  a depends_on mpi
  a depends_on b
  b depends_on mpich

- Packages now restrict the mpi dependency to mpich

6 files changed, 107 insertions, 32 deletions

diff --git a/lib/spack/spack/cmd/spec.py b/lib/spack/spack/cmd/spec.py
index bce93d45e1..3e4e573f5f 100644
--- a/lib/spack/spack/cmd/spec.py
+++ b/lib/spack/spack/cmd/spec.py
@@ -5,17 +5,23 @@ import spack.tty as tty
 import spack.url as url
 import spack
 
-
-description = "parse specs and print them out to the command line."
+description = "print out abstract and concrete versions of a spec."
 
 def setup_parser(subparser):
     subparser.add_argument('specs', nargs=argparse.REMAINDER, help="specs of packages")
 
 def spec(parser, args):
-    specs = spack.cmd.parse_specs(args.specs)
-    for spec in specs:
+    for spec in spack.cmd.parse_specs(args.specs):
+        print "Input spec"
+        print "------------------------------"
+        print spec.tree(color=True, indent=2)
+
+        print "Normalized"
+        print "------------------------------"
         spec.normalize()
-        print spec.tree(color=True)
+        print spec.tree(color=True, indent=2)
 
+        print "Concretized"
+        print "------------------------------"
         spec.concretize()
-        print spec.tree(color=True)
+        print spec.tree(color=True, indent=2)
diff --git a/lib/spack/spack/packages/__init__.py b/lib/spack/spack/packages/__init__.py
index 5fb4329aeb..d0e6b74f0a 100644
--- a/lib/spack/spack/packages/__init__.py
+++ b/lib/spack/spack/packages/__init__.py
@@ -52,34 +52,42 @@ class ProviderIndex(object):
         # TODO: come up with another name for this.  This "restricts" values to
         # the verbatim impu specs (i.e., it doesn't pre-apply package's constraints, and
         # keeps things as broad as possible, so it's really the wrong name)
-        restrict = kwargs.setdefault('restrict', False)
+        self.restrict = kwargs.setdefault('restrict', False)
 
         self.providers = {}
 
         for spec in specs:
-            if type(spec) != spack.spec.Spec:
+            if not isinstance(spec, spack.spec.Spec):
                 spec = spack.spec.Spec(spec)
 
             if spec.virtual:
                 continue
 
-            pkg = spec.package
-            for provided_spec, provider_spec in pkg.provided.iteritems():
-                if provider_spec.satisfies(spec, deps=False):
-                    provided_name = provided_spec.name
-                    if provided_name not in self.providers:
-                        self.providers[provided_name] = {}
+            self.update(spec)
 
-                    if restrict:
-                        self.providers[provided_name][provided_spec] = spec
 
-                    else:
-                        # Before putting the spec in the map, constrain it so that
-                        # it provides what was asked for.
-                        constrained = spec.copy()
-                        constrained.constrain(provider_spec)
-                        self.providers[provided_name][provided_spec] = constrained
+    def update(self, spec):
+        if type(spec) != spack.spec.Spec:
+            spec = spack.spec.Spec(spec)
 
+        assert(not spec.virtual)
+
+        pkg = spec.package
+        for provided_spec, provider_spec in pkg.provided.iteritems():
+            if provider_spec.satisfies(spec, deps=False):
+                provided_name = provided_spec.name
+                if provided_name not in self.providers:
+                    self.providers[provided_name] = {}
+
+                if self.restrict:
+                    self.providers[provided_name][provided_spec] = spec
+
+                else:
+                    # Before putting the spec in the map, constrain it so that
+                    # it provides what was asked for.
+                    constrained = spec.copy()
+                    constrained.constrain(provider_spec)
+                    self.providers[provided_name][provided_spec] = constrained
 
 
     def providers_for(self, *vpkg_specs):
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 3aba17142b..f2c3d70f23 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -476,11 +476,21 @@ class Spec(object):
         visited.add(self.name)
 
 
+    def _replace_with(self, concrete):
+        """Replace this virtual spec with a concrete spec."""
+        assert(self.virtual)
+        for name, dependent in self.dependents.items():
+            del dependent.dependencies[self.name]
+            dependent._add_dependency(concrete)
+
+
     def _expand_virtual_packages(self):
         """Find virtual packages in this spec, replace them with providers,
            and normalize again to include the provider's (potentially virtual)
            dependencies.  Repeat until there are no virtual deps.
 
+           Precondition: spec is normalized.
+
            .. todo::
 
               If a provider depends on something that conflicts with
@@ -500,10 +510,7 @@ class Spec(object):
                 providers = packages.providers_for(spec)
                 concrete = spack.concretizer.choose_provider(spec, providers)
                 concrete = concrete.copy()
-
-                for name, dependent in spec.dependents.items():
-                    del dependent.dependencies[spec.name]
-                    dependent._add_dependency(concrete)
+                spec._replace_with(concrete)
 
             # If there are duplicate providers or duplicate provider deps, this
             # consolidates them and merges constraints.
@@ -612,12 +619,26 @@ class Spec(object):
                     # The user might have required something insufficient for
                     # pkg_dep -- so we'll get a conflict.  e.g., user asked for
                     # mpi@:1.1 but some package required mpi@2.1:.
-                    providers = provider_index.providers_for(name)
-                    if len(providers) > 1:
-                        raise MultipleProviderError(pkg_dep, providers)
-                    if providers:
-                        raise UnsatisfiableProviderSpecError(providers[0], pkg_dep)
-
+                    required = provider_index.providers_for(name)
+                    if len(required) > 1:
+                        raise MultipleProviderError(pkg_dep, required)
+                    elif required:
+                        raise UnsatisfiableProviderSpecError(
+                            required[0], pkg_dep)
+            else:
+                # if it's a real dependency, check whether it provides something
+                # already required in the spec.
+                index = packages.ProviderIndex([pkg_dep], restrict=True)
+                for vspec in (v for v in spec_deps.values() if v.virtual):
+                    if index.providers_for(vspec):
+                        vspec._replace_with(pkg_dep)
+                        del spec_deps[vspec.name]
+                    else:
+                        required = index.providers_for(vspec.name)
+                        if required:
+                            raise UnsatisfiableProviderSpecError(
+                                required[0], pkg_dep)
+                provider_index.update(pkg_dep)
 
             if name not in spec_deps:
                 # If the spec doesn't reference a dependency that this package
@@ -673,6 +694,7 @@ class Spec(object):
         spec_packages = [d.package for d in spec_deps.values() if not d.virtual]
 
         index = packages.ProviderIndex(spec_deps.values(), restrict=True)
+
         visited = set()
         self._normalize_helper(visited, spec_deps, index)
 
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index b27b6b6813..a25075c617 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -133,3 +133,13 @@ class ConcretizeTest(MockPackagesTest):
         self.assertIn('fake', spec.dependencies['callpath'].dependencies['zmpi'].dependencies)
 
         self.assertNotIn('mpi', spec)
+
+
+    def test_my_dep_depends_on_provider_of_my_virtual_dep(self):
+        spec = Spec('indirect_mpich')
+        spec.normalize()
+
+        print
+        print spec.tree(color=True)
+
+        spec.concretize()
diff --git a/lib/spack/spack/test/mock_packages/direct_mpich.py b/lib/spack/spack/test/mock_packages/direct_mpich.py
new file mode 100644
index 0000000000..929c90b741
--- /dev/null
+++ b/lib/spack/spack/test/mock_packages/direct_mpich.py
@@ -0,0 +1,12 @@
+from spack import *
+
+class DirectMpich(Package):
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/direct_mpich-1.0.tar.gz"
+
+    versions = { 1.0 : 'foobarbaz' }
+
+    depends_on('mpich')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/lib/spack/spack/test/mock_packages/indirect_mpich.py b/lib/spack/spack/test/mock_packages/indirect_mpich.py
new file mode 100644
index 0000000000..6d518d7fca
--- /dev/null
+++ b/lib/spack/spack/test/mock_packages/indirect_mpich.py
@@ -0,0 +1,17 @@
+from spack import *
+
+class IndirectMpich(Package):
+    """Test case for a package that depends on MPI and one of its
+       dependencies requires a *particular version* of MPI.
+    """
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/indirect_mpich-1.0.tar.gz"
+
+    versions = { 1.0 : 'foobarbaz' }
+
+    depends_on('mpi')
+    depends_on('direct_mpich')
+
+    def install(self, spec, prefix):
+        pass