Skip to content

Package.is_extension now uses Spec.concrete#3853

Closed
scheibelp wants to merge 1 commit intospack:developfrom
scheibelp:bugfix/package-is-extension
Closed

Package.is_extension now uses Spec.concrete#3853
scheibelp wants to merge 1 commit intospack:developfrom
scheibelp:bugfix/package-is-extension

Conversation

@scheibelp
Copy link
Copy Markdown
Member

This addresses an issue observed in #3227 where extendee_spec on a package was present because the package was not considered concrete (according to the ._concrete property).

  • I'd like to add a test where a package conditionally extends another package and make sure this fixes the issue

Originally Package.is_extension was using Spec._concrete. This replaces the direct access to the cached variable with a call to the property function (since in some cases, e.g. after a copy, _concrete is not set properly).

Originally Package.is_extension was using Spec._concrete. This
replaces the direct access to the cached variable with a call to
the property function (since in some cases, e.g. after a copy,
_concrete is not set properly).
@adamjstewart
Copy link
Copy Markdown
Member

The use of spec.concrete and spec._concrete seems to be really inconsistent. @alalazo recently made the exact opposite change in #3686 to fix another bug. Whatever we do, unit tests would be much appreciated.

@davydden
Copy link
Copy Markdown
Member

I think only in exceptional cases we should merge core PRs without regression tests. Otherwise, given tge complexity of Spack's core, a fix for one issues would be breaking a fix for another and we would go in circles.

@scheibelp
Copy link
Copy Markdown
Member Author

I agree with the above points. I've been talking with @tgamblin about it. In the meantime I've opened an issue about the problem this is attempting to address at #3887

alalazo added a commit to alalazo/spack that referenced this pull request Aug 20, 2020
becker33 pushed a commit that referenced this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants