From f27c5e74ed64260407c342f297cd60488304b8bf Mon Sep 17 00:00:00 2001 From: scheibelp Date: Sun, 28 Jan 2018 16:58:08 -0800 Subject: [PATCH] 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 | 13 ----------- lib/spack/spack/repository.py | 21 +++++++---------- lib/spack/spack/spec.py | 35 ++++++++++------------------ lib/spack/spack/test/cmd/install.py | 3 --- lib/spack/spack/test/git_fetch.py | 2 +- lib/spack/spack/test/hg_fetch.py | 2 +- lib/spack/spack/test/packaging.py | 5 ++-- lib/spack/spack/test/svn_fetch.py | 2 +- lib/spack/spack/test/url_fetch.py | 4 ++-- 9 files changed, 27 insertions(+), 60 deletions(-) diff --git a/lib/spack/spack/build_environment.py b/lib/spack/spack/build_environment.py index b2a7e1f6d6e..81626d73618 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 c9111e9b3d2..ec5e7ddc44b 100644 --- a/lib/spack/spack/repository.py +++ b/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,18 +828,13 @@ 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. - 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 b0eba7363d3..61f127a66c9 100644 --- a/lib/spack/spack/spec.py +++ b/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 = 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 @@ 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 diff --git a/lib/spack/spack/test/cmd/install.py b/lib/spack/spack/test/cmd/install.py index 3720e9bd38f..85e52f2dafb 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 b28c5537534..4ca8ce506ff 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 6a22502e869..07e7840eaa1 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 7cb8dfc1fe0..df45d961893 100644 --- a/lib/spack/spack/test/packaging.py +++ b/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 diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py index f00b0b82597..7e107ec423a 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 168bda5f645..83e184f1966 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']: