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

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)

lib/spack/spack/repository.py

@@ -819,7 +819,7 @@ def _read_config(self):
                     % (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,19 +828,14 @@ def get(self, spec, new=False):
                 "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.
+            return package_class(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]
-
     @_autospec
     def dump_provenance(self, spec, path):
         """Dump provenance information for a spec to a particular path.

lib/spack/spack/spec.py

@@ -1197,7 +1197,9 @@ def root(self):
 
     @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 @@ def concretize(self):
 
         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 @@ def concretize(self):
         # 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 @@ def concretize(self):
         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 @@ def _evaluate_dependency_conditions(self, name):
         If no conditions are True (and we don't depend on it), return
         ``(None, None)``.
         """
-        pkg = spack.repo.get(self.fullname)
+        conditions = self.package_class.dependencies[name]
-        conditions = pkg.dependencies[name]
 
         substitute_abstract_variants(self)
         # evaluate when specs to figure out constraints on the dependency.
@@ -2136,10 +2123,9 @@ def _normalize_helper(self, visited, spec_deps, provider_index):
         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 @@ def patches(self):
         # 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 @@ def patches(self):
             # 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 @@ def _dup(self, other, deps=True, cleardeps=True, caches=None):
                        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 @@ def spec(self, name):
         spec._hash = None
         spec._cmp_key_cache = None
 
+        spec._package = None
         spec._normal = False
         spec._concrete = False
 

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():

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

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

lib/spack/spack/test/packaging.py

@@ -93,7 +93,7 @@ def test_packaging(mock_archive, tmpdir):
     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 @@ def test_packaging(mock_archive, tmpdir):
     # 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

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

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']: