Don't change properties on already-installed packages (#5580)

* edits to address issues where spack concretization attempts to set properties on already-installed specs

* most added checks only need to check if the spec is concrete; they dont also need to check if the package is installed

* add test to ensure that patches are not applied to an installed spec

* add test to ensure that an error is detected when a dependent requests a dependency constraint which conflicts with a requested installed dependency

5 files changed, 163 insertions, 0 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 624c28285a..24da1af564 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1612,6 +1612,10 @@ class Spec(object):
         if self.name in visited:
             return False
 
+        if self.concrete:
+            visited.add(self.name)
+            return False
+
         changed = False
 
         # Concretize deps first -- this is a bottom-up process.
@@ -1805,6 +1809,9 @@ class Spec(object):
             if s.namespace is None:
                 s.namespace = spack.repo.repo_for_pkg(s.name).namespace
 
+            if s.concrete:
+                continue
+
             # Add any patches from the package to the spec.
             patches = []
             for cond, patch_list in s.package_class.patches.items():
@@ -1826,6 +1833,9 @@ class Spec(object):
             if dspec.spec.name not in pkg_deps:
                 continue
 
+            if dspec.spec.concrete:
+                continue
+
             patches = []
             for cond, dependency in pkg_deps[dspec.spec.name].items():
                 if dspec.parent.satisfies(cond):
@@ -1878,6 +1888,8 @@ class Spec(object):
         unless there is a need to force a spec to be concrete.
         """
         for s in self.traverse(deptype_query=all):
+            if s.concrete and s.package.installed:
+                continue
             s._normal = value
             s._concrete = value
 
@@ -2067,6 +2079,9 @@ class Spec(object):
         # caller, it's a copy from _evaluate_dependency_conditions. If it
         # comes from a vdep, it's a defensive copy from _find_provider.
         if dep.name not in spec_deps:
+            if self.concrete:
+                return False
+
             spec_deps[dep.name] = dep
             changed = True
         else:
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 8887de8584..454adbac5e 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -111,6 +111,36 @@ def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch):
             pass
 
 
+def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
+    import sys
+    dependency = Spec('dependency-install')
+    dependency.concretize()
+    dependency.package.do_install()
+
+    dependency.package.patches['dependency-install'] = [
+        sys.modules['spack.patch'].Patch.create(
+            None, 'file://fake.patch', sha256='unused-hash')]
+
+    dependency_hash = dependency.dag_hash()
+    dependent = Spec('dependent-install ^/' + dependency_hash)
+    dependent.concretize()
+
+    assert dependent['dependency-install'] == dependency
+
+
+def test_installed_dependency_request_conflicts(
+        install_mockery, mock_fetch, refresh_builtin_mock):
+    dependency = Spec('dependency-install')
+    dependency.concretize()
+    dependency.package.do_install()
+
+    dependency_hash = dependency.dag_hash()
+    dependent = Spec(
+        'conflicting-dependent ^/' + dependency_hash)
+    with pytest.raises(spack.spec.UnsatisfiableSpecError):
+        dependent.concretize()
+
+
 def test_partial_install_keep_prefix(install_mockery, mock_fetch):
     spec = Spec('canfail')
     spec.concretize()
diff --git a/var/spack/repos/builtin.mock/packages/conflicting-dependent/package.py b/var/spack/repos/builtin.mock/packages/conflicting-dependent/package.py
new file mode 100644
index 0000000000..255ad1fc1c
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/conflicting-dependent/package.py
@@ -0,0 +1,41 @@
+##############################################################################
+# Copyright (c) 2013-2017, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class ConflictingDependent(Package):
+    """By itself this package does not have conflicts, but it is used to
+       ensure that if a user tries to build with an installed instance
+       of dependency-install@2 that there is a failure."""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/a-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    depends_on('dependency-install@:1.0')
+
+    def install(self, spec, prefix):
+        pass
diff --git a/var/spack/repos/builtin.mock/packages/dependency-install/package.py b/var/spack/repos/builtin.mock/packages/dependency-install/package.py
new file mode 100644
index 0000000000..72e2ce18af
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/dependency-install/package.py
@@ -0,0 +1,38 @@
+##############################################################################
+# Copyright (c) 2013-2017, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class DependencyInstall(Package):
+    """Dependency which has a working install method"""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/a-1.0.tar.gz"
+
+    version('1.0', 'hash1.0')
+    version('2.0', 'hash2.0')
+
+    def install(self, spec, prefix):
+        touch(join_path(prefix, 'an_installation_file'))
diff --git a/var/spack/repos/builtin.mock/packages/dependent-install/package.py b/var/spack/repos/builtin.mock/packages/dependent-install/package.py
new file mode 100644
index 0000000000..450bdacbf5
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/dependent-install/package.py
@@ -0,0 +1,39 @@
+##############################################################################
+# Copyright (c) 2013-2017, Lawrence Livermore National Security, LLC.
+# Produced at the Lawrence Livermore National Laboratory.
+#
+# This file is part of Spack.
+# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved.
+# LLNL-CODE-647188
+#
+# For details, see https://github.com/llnl/spack
+# Please also see the NOTICE and LICENSE files for our notice and the LGPL.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU Lesser General Public License (as
+# published by the Free Software Foundation) version 2.1, February 1999.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and
+# conditions of the GNU Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+from spack import *
+
+
+class DependentInstall(Package):
+    """Dependent which has a working install method"""
+
+    homepage = "http://www.example.com"
+    url      = "http://www.example.com/a-1.0.tar.gz"
+
+    version('1.0', '0123456789abcdef0123456789abcdef')
+
+    depends_on('dependency-install')
+
+    def install(self, spec, prefix):
+        touch(join_path(prefix, 'an_installation_file'))