ASP-based solver: always consider version of installed packages (#29933)

* ASP-based solver: always consider version of installed packages

fixes #29201

Explicitly add facts for versions of installed software when
using the --reuse option, so that we could consider versions
that are not declared in package.py

3 files changed, 115 insertions, 19 deletions

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index cb2b697282..78b465dddc 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -80,16 +80,18 @@ ast_sym = ast_getter("symbol", "term")
 
 #: Enumeration like object to mark version provenance
 version_provenance = collections.namedtuple(  # type: ignore
-    'VersionProvenance', ['external', 'packages_yaml', 'package_py', 'spec']
-)(spec=0, external=1, packages_yaml=2, package_py=3)
+    'VersionProvenance',
+    ['external', 'packages_yaml', 'package_py', 'spec', 'installed']
+)(installed=0, spec=1, external=2, packages_yaml=3, package_py=4)
 
 #: String representation of version origins, to emit legible
 # facts for the ASP solver
 version_origin_str = {
-    0: 'spec',
-    1: 'external',
-    2: 'packages_yaml',
-    3: 'package_py'
+    0: 'installed',
+    1: 'spec',
+    2: 'external',
+    3: 'packages_yaml',
+    4: 'package_py'
 }
 
 #: Named tuple to contain information on declared versions
@@ -1652,8 +1654,16 @@ class SpackSolverSetup(object):
             self.impose(h, spec, body=True)
             self.gen.newline()
 
-            # add OS to possible OS's
+            # Declare as possible parts of specs that are not in package.py
+            # - Add versions to possible versions
+            # - Add OS to possible OS's
             for dep in spec.traverse():
+                self.possible_versions[dep.name].add(dep.version)
+                self.declared_versions[dep.name].append(DeclaredVersion(
+                    version=dep.version,
+                    idx=0,
+                    origin=version_provenance.installed
+                ))
                 self.possible_oses.add(dep.os)
 
             # add the hash to the one seen so far
diff --git a/lib/spack/spack/solver/concretize.lp b/lib/spack/spack/solver/concretize.lp
index 3206ed60a4..19bec37908 100644
--- a/lib/spack/spack/solver/concretize.lp
+++ b/lib/spack/spack/solver/concretize.lp
@@ -37,6 +37,12 @@ version_declared(Package, Version, Weight) :- version_declared(Package, Version,
    Origin1 < Origin2,
    error("Internal error: two versions with identical weights").
 
+% We cannot use a version declared for an installed package if we end up building it
+:- version_declared(Package, Version, Weight, "installed"),
+   version(Package, Version),
+   version_weight(Package, Weight),
+   not hash(Package, _).
+
 % versions are declared w/priority -- declared with priority implies declared
 version_declared(Package, Version) :- version_declared(Package, Version, _).
 
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 607ec85cd8..010d269507 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -129,6 +129,8 @@ class Root(Package):
 
     version(1.0, sha256='abcde')
     depends_on('changing')
+
+    conflicts('changing~foo')
 """
     packages_dir.join('root', 'package.py').write(
         root_pkg_str, ensure=True
@@ -139,32 +141,65 @@ class Changing(Package):
     homepage = "http://www.example.com"
     url      = "http://www.example.com/changing-1.0.tar.gz"
 
+
+{% if not delete_version %}
     version(1.0, sha256='abcde')
+{% endif %}
+    version(0.9, sha256='abcde')
+
 {% if not delete_variant %}
     variant('fee', default=True, description='nope')
 {% endif %}
     variant('foo', default=True, description='nope')
 {% if add_variant %}
     variant('fum', default=True, description='nope')
+    variant('fum2', default=True, description='nope')
 {% endif %}
 """
-    repo = spack.repo.Repo(str(repo_dir))
-    mutable_mock_repo.put_first(repo)
 
     class _ChangingPackage(object):
-        def change(self, context):
-            # To ensure we get the changed package we need to
-            # invalidate the cache
-            repo._modules = {}
-
+        default_context = [
+            ('delete_version', True),
+            ('delete_variant', False),
+            ('add_variant', False)
+        ]
+
+        def __init__(self, repo_directory):
+            self.repo_dir = repo_directory
+            self.repo = spack.repo.Repo(str(repo_directory))
+            mutable_mock_repo.put_first(self.repo)
+
+        def change(self, changes=None):
+            changes = changes or {}
+            context = dict(self.default_context)
+            context.update(changes)
+            # Remove the repo object and delete Python modules
+            mutable_mock_repo.remove(self.repo)
+            # TODO: this mocks a change in the recipe that should happen in a
+            # TODO: different process space. Leaving this comment as a hint
+            # TODO: in case tests using this fixture start failing.
+            if sys.modules.get('spack.pkg.changing.changing'):
+                del sys.modules['spack.pkg.changing.changing']
+                del sys.modules['spack.pkg.changing.root']
+                del sys.modules['spack.pkg.changing']
+
+            # Change the recipe
             t = jinja2.Template(changing_template)
             changing_pkg_str = t.render(**context)
             packages_dir.join('changing', 'package.py').write(
                 changing_pkg_str, ensure=True
             )
 
-    _changing_pkg = _ChangingPackage()
-    _changing_pkg.change({'delete_variant': False, 'add_variant': False})
+            # Re-add the repository
+            self.repo = spack.repo.Repo(str(self.repo_dir))
+            mutable_mock_repo.put_first(self.repo)
+
+    _changing_pkg = _ChangingPackage(repo_dir)
+    _changing_pkg.change({
+        'delete_version': False,
+        'delete_variant': False,
+        'add_variant': False
+    })
 
     return _changing_pkg
 
@@ -1210,10 +1245,12 @@ class TestConcretize(object):
         {'add_variant': False, 'delete_variant': True},
         {'add_variant': True, 'delete_variant': True}
     ])
-    @pytest.mark.xfail()
     def test_reuse_installed_packages_when_package_def_changes(
             self, context, mutable_database, repo_with_changing_recipe
     ):
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.xfail('Known failure of the original concretizer')
+
         # Install a spec
         root = Spec('root').concretized()
         dependency = root['changing'].copy()
@@ -1223,11 +1260,14 @@ class TestConcretize(object):
         repo_with_changing_recipe.change(context)
 
         # Try to concretize with the spec installed previously
-        new_root = Spec('root ^/{0}'.format(
+        new_root_with_reuse = Spec('root ^/{0}'.format(
             dependency.dag_hash())
         ).concretized()
 
-        assert root.dag_hash() == new_root.dag_hash()
+        new_root_without_reuse = Spec('root').concretized()
+
+        assert root.dag_hash() == new_root_with_reuse.dag_hash()
+        assert root.dag_hash() != new_root_without_reuse.dag_hash()
 
     @pytest.mark.regression('20784')
     def test_concretization_of_test_dependencies(self):
@@ -1485,3 +1525,43 @@ class TestConcretize(object):
 
         s = Spec('conditional-values-in-variant@1.60.0').concretized()
         assert 'cxxstd' in s.variants
+
+    @pytest.mark.regression('29201')
+    def test_delete_version_and_reuse(
+            self, mutable_database, repo_with_changing_recipe
+    ):
+        """Test that we can reuse installed specs with versions not
+        declared in package.py
+        """
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.xfail('Known failure of the original concretizer')
+
+        root = Spec('root').concretized()
+        root.package.do_install(fake=True, explicit=True)
+        repo_with_changing_recipe.change({'delete_version': True})
+
+        with spack.config.override("concretizer:reuse", True):
+            new_root = Spec('root').concretized()
+
+        assert root.dag_hash() == new_root.dag_hash()
+
+    @pytest.mark.regression('29201')
+    def test_installed_version_is_selected_only_for_reuse(
+            self, mutable_database, repo_with_changing_recipe
+    ):
+        """Test that a version coming from an installed spec is a possible
+        version only for reuse
+        """
+        if spack.config.get('config:concretizer') == 'original':
+            pytest.xfail('Known failure of the original concretizer')
+
+        # Install a dependency that cannot be reused with "root"
+        # because of a conflict a variant, then delete its version
+        dependency = Spec('changing@1.0~foo').concretized()
+        dependency.package.do_install(fake=True, explicit=True)
+        repo_with_changing_recipe.change({'delete_version': True})
+
+        with spack.config.override("concretizer:reuse", True):
+            new_root = Spec('root').concretized()
+
+        assert not new_root['changing'].satisfies('@1.0')