Perform concretization of build-only deps separately - part two#4595
Closed
scheibelp wants to merge 102 commits intospack:developfrom
Closed
Perform concretization of build-only deps separately - part two#4595scheibelp wants to merge 102 commits intospack:developfrom
scheibelp wants to merge 102 commits intospack:developfrom
Conversation
…s link (should be 'include')
…encies (2) rename variable storing all packages which should be in the path for build
…e that all contexts are updated
…dencies they were not referring to the initialized spec)
…pdate compiler concretization test to specify compiler preferences for a non-build dep (as of now in this branch this is not supported)
… it should be an 'include' dep but for now its deptype is 'link')
…d dependency can appear multiple times in the dag
…wn version of the package (which results in a non-diamond shape)
…ackages constrain one another; dont need to maintain dependency contexts to figure out which specs have been visited.
…ified dependencies) - this fixes a couple failing tests for directory_layout
…spec included dependency constraints; they were swapped to using Spec.normalize_top
tgamblin
pushed a commit
that referenced
this pull request
Jan 29, 2018
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
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
3 tasks
Contributor
|
Are they any plan to go forward with this PR ? |
|
Is this still planned or ever going forward? Packages like glib are completely broken due to python 3 build only dependencies for stuff like meson. While python 2 is going out of support many scientific libraries, opensource and paid, still depend on it. |
Member
|
@scheibelp is this PR still a work-in-progress or will it become obsolete with the new concretizer? |
Member
|
Closing, as this has been superseded by PRs in this project: https://github.com/spack/spack/projects/48. |
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
9 participants
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.
@luigi-calori @svenevs you seemed interested in #2548 and this is supposed to be better
This is intended as a replacement to #2548. It can handle a few cases that #2548 cannot. It is also intended to be useful to support concretization of new specs using previously-concretized specs, which will be useful for contexts (#4585).
It can currently concretize:
It is a relaxation on the current concretization model, which requires that only one instance of any particular package exists in a dependency DAG. The new (looser) constraints are:
For example, given:
This is not as flexible as the deptype model in #3768 would allow, but it is a compromise between that and simplicity of implementation.
It works by
TODOs
Limitations introduced:
spec['X'], X must either be (a) a transitive run/link/include dependency or (b) a transitive run/link/include dependency of a direct build dependency. In other words, indirect build dependencies are excluded(8/31/17) when a user specifies constraints on dependencies from the command line, they cannot constrain build-only dependencies(EDIT) no longer an issue(internal change) Spec.normalize should no longer be called by itself(EDIT) no longer a problem