Remove Package instance caching in Repo (#6367)

This attempts to address one of the complaints at #5996 (comment):

> Repo currently caches package instances by Spec, and those Package instances have a Spec. 
> This is unnecessary and causes confusion. I think I thought that we'd need to cache instances 
> after loading package classes, but really just caching the classes is fine.

With this update, Repo's package cache is removed and Specs cache the package reference themselves. One consequence is that Specs which compare as equal will store separate instances of a Package class (not doing this creates issues for #4595 (comment)).

There were several references to Spec.package that could be replaced with Spec.package_class without any additional modifications. There are still a couple remaining references to Spec.package in Spec that would require adding functionality before replacing (e.g. calling Package.provides and Package.installed).

Note this makes it difficult to mock fetchers for tests which invokes code that reconstructs specs. test_packaging was one example of this where the updates caused a failure (in that case the error was avoided by not making an unnecessary call).

Details:
* Replace instances of spec.package with spec.package_class where a class method is being called
* Remove instances of Repo.get where Spec.package_class can be used in its place
* remove Repo.get caching instances of Package class for specs
* remove redundant check (which is also incorrect now that each spec stores its own copy of its package)
* avoid creating mirror with specs because it copies specs and those copies dont refer to the mocked fetcher (and it is also not required for the test)
* remove checks that are no longer necessary since repo doesn't cache specs

9 files changed, 27 insertions, 60 deletions

diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py
index b2a7e1f6d6..81626d7361 100644
--- a/lib/spack/spack/build_environment.py
+++ b/lib/spack/spack/build_environment.py
@@ -575,19 +575,6 @@ def setup_package(pkg, dirty):
     spack_env = EnvironmentModifications()
     run_env = EnvironmentModifications()
 
-    # Before proceeding, ensure that specs and packages are consistent
-    #
-    # This is a confusing behavior due to how packages are
-    # constructed.  `setup_dependent_package` may set attributes on
-    # specs in the DAG for use by other packages' install
-    # method. However, spec.package will look up a package via
-    # spack.repo, which defensively copies specs into packages.  This
-    # code ensures that all packages in the DAG have pieces of the
-    # same spec object at build time.
-    #
-    for s in pkg.spec.traverse():
-        assert s.package.spec is s
-
     set_compiler_environment_variables(pkg, spack_env)
     set_build_environment_variables(pkg, spack_env, dirty)
     pkg.architecture.platform.setup_platform_environment(pkg, spack_env)
diff --git a/lib/spack/spack/repository.py b/lib/spack/spack/repository.py
index c9111e9b3d..ec5e7ddc44 100644
--- a/lib/spack/spack/repository.py
+++ b/lib/spack/spack/repository.py
@@ -819,7 +819,7 @@ class Repo(object):
                     % (self.config_file, self.root))
 
     @_autospec
-    def get(self, spec, new=False):
+    def get(self, spec):
         if not self.exists(spec.name):
             raise UnknownPackageError(spec.name)
 
@@ -828,18 +828,13 @@ class Repo(object):
                 "Repository %s does not contain package %s"
                 % (self.namespace, spec.fullname))
 
-        key = hash(spec)
-        if new or key not in self._instances:
-            package_class = self.get_pkg_class(spec.name)
-            try:
-                copy = spec.copy()  # defensive copy.  Package owns its spec.
-                self._instances[key] = package_class(copy)
-            except Exception:
-                if spack.debug:
-                    sys.excepthook(*sys.exc_info())
-                raise FailedConstructorError(spec.fullname, *sys.exc_info())
-
-        return self._instances[key]
+        package_class = self.get_pkg_class(spec.name)
+        try:
+            return package_class(spec)
+        except Exception:
+            if spack.debug:
+                sys.excepthook(*sys.exc_info())
+            raise FailedConstructorError(spec.fullname, *sys.exc_info())
 
     @_autospec
     def dump_provenance(self, spec, path):
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index b0eba7363d..61f127a66c 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1197,7 +1197,9 @@ class Spec(object):
 
     @property
     def package(self):
-        return spack.repo.get(self)
+        if not self._package:
+            self._package = spack.repo.get(self)
+        return self._package
 
     @property
     def package_class(self):
@@ -1773,17 +1775,8 @@ class Spec(object):
 
         Some rigorous validation and checks are also performed on the spec.
         Concretizing ensures that it is self-consistent and that it's
-        consistent with requirements of its pacakges. See flatten() and
+        consistent with requirements of its packages. See flatten() and
         normalize() for more details on this.
-
-        It also ensures that:
-
-        .. code-block:: python
-
-            for x in self.traverse():
-                assert x.package.spec == x
-
-        which may not be true *during* the concretization step.
         """
         if not self.name:
             raise SpecError("Attempting to concretize anonymous spec")
@@ -1872,7 +1865,7 @@ class Spec(object):
         # there are declared conflicts
         matches = []
         for x in self.traverse():
-            for conflict_spec, when_list in x.package.conflicts.items():
+            for conflict_spec, when_list in x.package_class.conflicts.items():
                 if x.satisfies(conflict_spec, strict=True):
                     for when_spec, msg in when_list:
                         if x.satisfies(when_spec, strict=True):
@@ -1880,11 +1873,6 @@ class Spec(object):
         if matches:
             raise ConflictsInSpecError(self, matches)
 
-        # At this point the spec-package mutual references should
-        # be self-consistent
-        for x in self.traverse():
-            x.package.spec = x
-
     def _mark_concrete(self, value=True):
         """Mark this spec and its dependencies as concrete.
 
@@ -1968,8 +1956,7 @@ class Spec(object):
         If no conditions are True (and we don't depend on it), return
         ``(None, None)``.
         """
-        pkg = spack.repo.get(self.fullname)
-        conditions = pkg.dependencies[name]
+        conditions = self.package_class.dependencies[name]
 
         substitute_abstract_variants(self)
         # evaluate when specs to figure out constraints on the dependency.
@@ -2136,10 +2123,9 @@ class Spec(object):
         any_change = False
         changed = True
 
-        pkg = spack.repo.get(self.fullname)
         while changed:
             changed = False
-            for dep_name in pkg.dependencies:
+            for dep_name in self.package_class.dependencies:
                 # Do we depend on dep_name?  If so pkg_dep is not None.
                 dep = self._evaluate_dependency_conditions(dep_name)
                 # If dep is a needed dependency, merge it.
@@ -2556,7 +2542,7 @@ class Spec(object):
         # FIXME: concretization to store the order of patches somewhere.
         # FIXME: Needs to be refactored in a cleaner way.
         for sha256 in self.variants['patches']._patches_in_order_of_appearance:
-            patch = self.package.lookup_patch(sha256)
+            patch = self.package_class.lookup_patch(sha256)
             if patch:
                 patches.append(patch)
                 continue
@@ -2564,7 +2550,7 @@ class Spec(object):
             # if not found in this package, check immediate dependents
             # for dependency patches
             for dep_spec in self._dependents.values():
-                patch = dep_spec.parent.package.lookup_patch(sha256)
+                patch = dep_spec.parent.package_class.lookup_patch(sha256)
 
                 if patch:
                     patches.append(patch)
@@ -2610,6 +2596,8 @@ class Spec(object):
                        self.external_module != other.external_module and
                        self.compiler_flags != other.compiler_flags)
 
+        self._package = None
+
         # Local node attributes get copied first.
         self.name = other.name
         self.versions = other.versions.copy()
@@ -3374,6 +3362,7 @@ class SpecParser(spack.parse.Parser):
         spec._hash = None
         spec._cmp_key_cache = None
 
+        spec._package = None
         spec._normal = False
         spec._concrete = False
 
diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py
index 3720e9bd38..85e52f2daf 100644
--- a/lib/spack/spack/test/cmd/install.py
+++ b/lib/spack/spack/test/cmd/install.py
@@ -72,9 +72,6 @@ def test_install_package_and_dependency(
     assert 'failures="0"' in content
     assert 'errors="0"' in content
 
-    s = Spec('libdwarf').concretized()
-    assert not spack.repo.get(s).stage.created
-
 
 @pytest.mark.usefixtures('noop_install', 'builtin_mock', 'config')
 def test_install_runtests():
diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py
index b28c553753..4ca8ce506f 100644
--- a/lib/spack/spack/test/git_fetch.py
+++ b/lib/spack/spack/test/git_fetch.py
@@ -89,7 +89,7 @@ def test_fetch(type_of_test,
     # Construct the package under test
     spec = Spec('git-test')
     spec.concretize()
-    pkg = spack.repo.get(spec, new=True)
+    pkg = spack.repo.get(spec)
     pkg.versions[ver('git')] = t.args
 
     # Enter the stage directory and check some properties
diff --git a/lib/spack/spack/test/hg_fetch.py b/lib/spack/spack/test/hg_fetch.py
index 6a22502e86..07e7840eaa 100644
--- a/lib/spack/spack/test/hg_fetch.py
+++ b/lib/spack/spack/test/hg_fetch.py
@@ -61,7 +61,7 @@ def test_fetch(
     # Construct the package under test
     spec = Spec('hg-test')
     spec.concretize()
-    pkg = spack.repo.get(spec, new=True)
+    pkg = spack.repo.get(spec)
     pkg.versions[ver('hg')] = t.args
 
     # Enter the stage directory and check some properties
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 7cb8dfc1fe..df45d96189 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -93,7 +93,7 @@ echo $PATH"""
     spec = Spec('trivial-install-test-package')
     spec.concretize()
     assert spec.concrete
-    pkg = spack.repo.get(spec)
+    pkg = spec.package
     fake_fetchify(mock_archive.url, pkg)
     pkg.do_install()
     pkghash = '/' + spec.dag_hash(7)
@@ -107,9 +107,8 @@ echo $PATH"""
     # put it directly into the mirror
 
     mirror_path = os.path.join(str(tmpdir), 'test-mirror')
-    specs = [spec]
     spack.mirror.create(
-        mirror_path, specs, no_checksum=True
+        mirror_path, specs=[], no_checksum=True
     )
 
     # register mirror with spack config
diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py
index f00b0b8259..7e107ec423 100644
--- a/lib/spack/spack/test/svn_fetch.py
+++ b/lib/spack/spack/test/svn_fetch.py
@@ -61,7 +61,7 @@ def test_fetch(
     # Construct the package under test
     spec = Spec('svn-test')
     spec.concretize()
-    pkg = spack.repo.get(spec, new=True)
+    pkg = spack.repo.get(spec)
     pkg.versions[ver('svn')] = t.args
 
     # Enter the stage directory and check some properties
diff --git a/lib/spack/spack/test/url_fetch.py b/lib/spack/spack/test/url_fetch.py
index 168bda5f64..83e184f196 100644
--- a/lib/spack/spack/test/url_fetch.py
+++ b/lib/spack/spack/test/url_fetch.py
@@ -60,7 +60,7 @@ def test_fetch(
     spec = Spec('url-test')
     spec.concretize()
 
-    pkg = spack.repo.get('url-test', new=True)
+    pkg = spack.repo.get('url-test')
     pkg.url = mock_archive.url
     pkg.versions[ver('test')] = {checksum_type: checksum, 'url': pkg.url}
     pkg.spec = spec
@@ -84,7 +84,7 @@ def test_fetch(
 
 
 def test_from_list_url(builtin_mock, config):
-    pkg = spack.repo.get('url-list-test', new=True)
+    pkg = spack.repo.get('url-list-test')
     for ver_str in ['0.0.0', '1.0.0', '2.0.0',
                     '3.0', '4.5', '2.0.0b2',
                     '3.0a1', '4.5-rc5']: