fix extendee_spec for transitive dependencies on potential extendees #48025
Conversation
spackbot-app
bot
added
core
becker33
force-pushed
the
bugfix/extendee-transitive-deps
branch
from
99b0d3a to
e4e02b3
Compare
|
Is this also the time and place to fix depends_on("python")
extends("python", when="@2:")if I read the code correctly just having a dependency is sufficient to extend something, completely ignoring the when condition of the extends directive. I find the current test a bit "deceiving" because you could interpret it as testing "conditionally extending a dependency" but in reality it tests "extending a conditional dependency". If that's not fixed in this PR, which is fine by me, then should we at least leave a comment noting the limitations of |
|
generally lgtm now 👍 could still use a test to cover the case when the dependency is there but it is not extended because the when condition doesn't apply |
| dep | ||
| for dep in self.spec.dependencies(deptype=("link", "run")) | ||
| for d, when in self.extendees.values() | ||
| if dep.satisfies(d) and self.spec.satisfies(when) |
There was a problem hiding this comment.
dep.satisfies(d) is true by construction, but maybe you added this check defensively?
There was a problem hiding this comment.
Yes, added because I can imagine future changes that would make it not true by construction -- like if we allow separate concretization of run deps in the future. Agreed that it is not relevant for current specs.
Co-authored-by: Harmen Stoppels <[email protected]>
That's covered by |
|
Are we waiting on something to merge this? @haampie sounds like all concerns are resolved. |
fixes #48024
Only consider transitive dependencies in
extendee_spec().