Remove Package instance caching in Repo#6367
Merged
tgamblin merged 8 commits intospack:developfrom Jan 29, 2018
Merged
Conversation
7 tasks
…ores its own copy of its package)
…pies dont refer to the mocked fetcher (and it is also not required for the test)
Member
Author
|
Closing and reopening for coverage stats |
Member
Author
ch741
pushed a commit
to ch741/spack
that referenced
this pull request
Apr 20, 2018
This attempts to address one of the complaints at spack#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 spack#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
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This attempts to address one of the complaints at #5996 (comment):
With this update,
Repo's package cache is removed andSpecscache the package reference themselves. One consequence is thatSpecswhich compare as equal will store separate instances of aPackageclass (not doing this creates issues for #4595 (comment)).There were several references to
Spec.packagethat could be replaced withSpec.package_classwithout any additional modifications. There are still a couple remaining references toSpec.packageinSpecthat would require adding functionality before replacing (e.g. callingPackage.providesandPackage.installed).Note this makes it difficult to mock fetchers for tests which invokes code that reconstructs specs.
test_packagingwas one example of this where the updates caused a failure (in that case the error was avoided by not making an unnecessary call).