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

ASP-based solver: always consider version of installed packages #29933

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Apr 7, 2022

fixes #29201
fixes #30064

Explicitly add facts for versions of installed software when using the --reuse option, so that we could consider versions that are not declared in package.py

Modifications:

  • Fixed the bug
  • Add unit test

@spackbot-app spackbot-app bot added new-variant new-version tests General test capability(ies) labels Apr 7, 2022
@alalazo alalazo force-pushed the fixes/consider_unknown_version_of_installed_software branch 2 times, most recently from c7a4e74 to c63d55d Compare April 7, 2022 12:47
@alalazo alalazo marked this pull request as ready for review April 7, 2022 15:04
@alalazo alalazo requested review from haampie and becker33 April 7, 2022 15:15
@alalazo alalazo added this to In progress in Spack v0.18.0 release via automation Apr 7, 2022
@alalazo alalazo added this to In progress in Spack reuse specs by default via automation Apr 7, 2022
@alalazo alalazo force-pushed the fixes/consider_unknown_version_of_installed_software branch from c63d55d to 5dcb505 Compare April 12, 2022 16:52
@tgamblin tgamblin moved this from In progress to Review in progress in Spack reuse specs by default Apr 12, 2022
Copy link
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

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

Have you tested the following scenario:

Package foo declares versions a, b
foo@c installed
foo@b preferred
foo@b conflicts with something in spec

I think in that case this will lead to concretizing foo@c even though it's only a valid version for reusing.

@@ -1654,6 +1656,12 @@ def _facts_from_concrete_spec(self, spec, possible):

# add OS to possible OS's
for dep in spec.traverse():
self.possible_versions[dep.name].add(dep.version)
Copy link
Member

Choose a reason for hiding this comment

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

The comment above this block probably should change to reflect the new content, maybe

# ensure all attributes of installed specs are considered possible values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 7727cd1

fixes spack#29201

Explicitly add facts for versions of installed software when
using the --reuse option, so that we could consider versions
that are not declared in package.py
@alalazo alalazo force-pushed the fixes/consider_unknown_version_of_installed_software branch from 5dcb505 to 7727cd1 Compare April 15, 2022 08:09
@alalazo
Copy link
Member Author

alalazo commented Apr 15, 2022

Have you tested the following scenario: [ ... ]

That scenario was working, but I came up with something along the same lines that wasn't. Fixed and added a new unit test for it. Thanks!

Comment on lines 1552 to 1554
"""Test that we can reuse installed specs with versions not
declared in package.py
"""
Copy link
Member

Choose a reason for hiding this comment

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

This docstring looks like it was mistakenly copied from the previous test.

Suggested change
"""Test that we can reuse installed specs with versions not
declared in package.py
"""
"""Test that we cannot build with versions only declared for reuse.
"""

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9876f66

Spack v0.18.0 release automation moved this from In progress to Review in progress Apr 22, 2022
@alalazo alalazo requested a review from becker33 April 23, 2022 12:49
Spack v0.18.0 release automation moved this from Review in progress to Reviewer approved Apr 25, 2022
Spack reuse specs by default automation moved this from Review in progress to Reviewer approved Apr 25, 2022
@becker33 becker33 merged commit 268c671 into spack:develop Apr 25, 2022
Spack v0.18.0 release automation moved this from Reviewer approved to Done Apr 25, 2022
Spack reuse specs by default automation moved this from Reviewer approved to Done Apr 25, 2022
@alalazo alalazo deleted the fixes/consider_unknown_version_of_installed_software branch April 25, 2022 17:29
tgamblin pushed a commit that referenced this pull request Apr 25, 2022
* ASP-based solver: always consider version of installed packages

fixes #29201

Explicitly add facts for versions of installed software when
using the --reuse option, so that we could consider versions
that are not declared in package.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
2 participants