Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: fix sticky variants in externals #42253

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

becker33
Copy link
Member

Fixes #42172

Currently, the concretizer does not preserve information about which variants were set explicitly on externals, which leads to concretization failures when an external specifies a sticky variant. This PR fixes the generation of facts for externals to preserve that information.

Includes regression test.

@spackbot-app spackbot-app bot added core PR affects Spack core functionality stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies) labels Jan 23, 2024
@becker33
Copy link
Member Author

@spackbot fix style

Copy link

spackbot-app bot commented Jan 23, 2024

Let me see if I can fix that for you!

Copy link

spackbot-app bot commented Jan 23, 2024

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/solver/asp.py
  lib/spack/spack/test/concretize.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/test/concretize.py
reformatted lib/spack/spack/solver/asp.py
All done! ✨ 🍰 ✨
2 files reformatted.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 610 source files
  mypy checks were clean
==> spack style checks were clean
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

Copy link
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

Basically LGTM

@@ -1522,6 +1522,18 @@ def test_sticky_variant_in_package(self):
s = Spec("sticky-variant %clang").concretized()
assert s.satisfies("%clang") and s.satisfies("~allow-gcc")

@pytest.mark.only_clingo("Original concretizer cannot use sticky variants")
def test_sticky_variant_in_external(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def test_sticky_variant_in_external(self):
@pytest.mark.regression("42172")
def test_sticky_variant_in_external(self):
"""Tests that we can set sticky variants from an external spec."""

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we test setting the sticky variant both ways + not having it?

@becker33
Copy link
Member Author

@spackbot fix style

Copy link

spackbot-app bot commented Jan 24, 2024

Let me see if I can fix that for you!

Copy link

spackbot-app bot commented Jan 24, 2024

I was able to run spack style --fix for you!

spack style --fix
==> Running style checks on spack
  selected: isort, black, flake8, mypy
==> Modified files
  lib/spack/spack/solver/asp.py
  lib/spack/spack/test/concretize.py
  var/spack/repos/builtin.mock/packages/sticky-variant-dependent/package.py
==> Running isort checks
  isort checks were clean
==> Running black checks
reformatted lib/spack/spack/test/concretize.py
All done! ✨ 🍰 ✨
1 file reformatted, 2 files left unchanged.
  black checks were clean
==> Running flake8 checks
  flake8 checks were clean
==> Running mypy checks
Success: no issues found in 610 source files
  mypy checks were clean
==> spack style checks were clean
Keep in mind that I cannot fix your flake8 or mypy errors, so if you have any you'll need to fix them and update the pull request. If I was able to push to your branch, if you make further changes you will need to pull from your updated branch before pushing again.

I've updated the branch with style fixes.

@alalazo alalazo merged commit d3c1f7a into develop Jan 25, 2024
34 checks passed
@alalazo alalazo deleted the bugfix/sticky-variant-external branch January 25, 2024 16:22
@alalazo alalazo mentioned this pull request Jan 25, 2024
13 tasks
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jan 25, 2024
RikkiButler20 pushed a commit to RikkiButler20/spack that referenced this pull request Jan 31, 2024
alalazo pushed a commit that referenced this pull request Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conflicts core PR affects Spack core functionality dependencies new-version stand-alone-tests Stand-alone (or smoke) tests for installed packages tests General test capability(ies)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sticky variants seem to not be working with externals
2 participants