Conversation
- Dependencies in concrete specs did not previously have their cache fields (_concrete, _normal, etc.) preserved. - _dup and _dup_deps weren't passing each other enough information to preserve concreteness properly, so only the root was properly preserved. - cached concreteness is now preserved properly for the entire DAG, not just the root. - added method docs.
|
@adamjstewart and everyone else should be happy because this pretty much gets rid of the distinction between |
|
Note that this should be rebased; commits here are meaningful. |
|
I'll be able to look at this by the end of today. |
tgamblin
force-pushed
the
feature/better-concreteness-behavior
branch
from
be589fb to
d6ea85b
Compare
| # in build caches are concrete (as they aer built) so | ||
| # we need to mark this spec concrete on read-in. | ||
| spec = spack.spec.Spec.from_yaml(f) | ||
| spec._mark_concrete() |
There was a problem hiding this comment.
This stuff actually comes out in testing with the new changes. Before it would slip by and could potentially be wrong.
|
|
||
| # satisfy_condition = self.owner.satisfies( | ||
| # condition, strict=True | ||
| # ) |
There was a problem hiding this comment.
I am so glad we removed this function. Fixing it following the old logic would have been a real pain.
There was a problem hiding this comment.
Fixing it following the old logic would essentially require reimplementing concretize() 😄 . I looked at that function as part of the work on dependency patching and it's also just wrong. It's the fact that these concrete() functions are still called lazily as part of concretize() that was screwing everything up -- they weren't really intended to do that. Spec is going to get a lot nicer soon.
There was a problem hiding this comment.
and yes this function is horrible.
|
@scheibelp: I removed the review request since @alalazo got to it first, but feel free to review this if you want. |
- Fixes bugs where concretization would fail due to an erroneously cached
_concrete attribute.
- Ripped out a bunch of code in spec.py that isn't needed/valid anymore:
- The various concrete() methods on different types of Specs would
attempt to statically compute whether the Spec was concrete.
- This dates back to when DAGs were simpler and there were no optional
dependencies. It's actually NOT possible to compute statically
whether a Spec is concrete now. The ONLY way you know is if it goes
through concretization and is marked concrete once that completes.
- This commit removes all simple concreteness checks and relies only on
the _concrete attribute. This should make thinking about
concreteness simpler.
- Fixed a couple places where Specs need to be marked concrete explicitly.
- Specs read from files and Specs that are destructively copied from
concrete Specs now need to be marked concrete explicitly.
- These spots may previously have "worked", but they were brittle and
should be explcitly marked anyway.
tgamblin
force-pushed
the
feature/better-concreteness-behavior
branch
from
d6ea85b to
bbd10a6
Compare
OK - I think @alalazo's review is sufficient. If I have any comment it's that it it would be nice to write I can't actually merge it (since the mac tests didn't complete) but I assume you will do that. |
3 participants
Fixes #5281.
@obreitwi @ax3l @alalazo @scheibelp @healther
Fixes bugs where concretization would fail due to an erroneously cached
_concrete attribute.
Ripped out a bunch of code in spec.py that isn't needed/valid anymore:
attempt to statically compute whether the Spec was concrete.
dependencies. It's actually NOT possible to compute statically
whether a Spec is concrete now. The ONLY way you know is if it goes
through concretization and is marked concrete once that completes.
the _concrete attribute. This should make thinking about
concreteness simpler.
Fixed a couple places where Specs need to be marked concrete explicitly.
concrete Specs now need to be marked concrete explicitly.
should be explcitly marked anyway.
Fixed bug -
_dup()and_dup_deps()that would lose cachedconcreteness of dependencies.