Skip to content

Remove Package instance caching in Repo#6367

Merged
tgamblin merged 8 commits intospack:developfrom
scheibelp:features/no-pkg-repo-cache
Jan 29, 2018
Merged

Remove Package instance caching in Repo#6367
tgamblin merged 8 commits intospack:developfrom
scheibelp:features/no-pkg-repo-cache

Conversation

@scheibelp
Copy link
Copy Markdown
Member

@scheibelp scheibelp commented Nov 18, 2017

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).

@scheibelp scheibelp removed the WIP label Nov 29, 2017
@scheibelp
Copy link
Copy Markdown
Member Author

Closing and reopening for coverage stats

@scheibelp scheibelp closed this Nov 29, 2017
@scheibelp scheibelp reopened this Nov 29, 2017
@scheibelp scheibelp closed this Nov 30, 2017
@scheibelp scheibelp reopened this Nov 30, 2017
@scheibelp
Copy link
Copy Markdown
Member Author

@tgamblin @becker33 this is the PR I mentioned which removes the package cache from spack.repo

@tgamblin tgamblin self-requested a review January 29, 2018 00:32
Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @scheibelp!

@tgamblin tgamblin merged commit f27c5e7 into spack:develop Jan 29, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants