concretizer: refactor handling of special variants dev_build: and patches#20259
concretizer: refactor handling of special variants dev_build: and patches#20259
Conversation
tgamblin
changed the title
|
@alalazo not sure I understand why the tests are failing here -- if I run this passes locally. Is there something I am missing? |
|
@tgamblin I'll have a look at this first thing today |
|
Confirmed. It passes locally also for me. It's very weird, as tests start failing in |
|
I can reproduce it consistently in the container, using the merge commit from this branch. If I use either the head of this branch or the head of develop then there are no failures. EDIT: I have been able to reproduce it locally. The issue is with the merge commit: $ git fetch origin refs/pull/20259/merge:test-branchKeep digging... |
|
I'm trying to understand what is going on, but in the merge commit all the failing tests reach out to the |
|
It seems this PR and #20247 interact in a way that exposes a bug in our test setup. Facts:
|
|
seems more elegant than #20102, but there's another place where a new variant value can come into existence - something like this, perhaps: diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index a4dc98a3abbd..57476173bc26 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1092,6 +1092,7 @@ def preferred_variants(self, pkg_name):
if not preferred_variants:
return
+ spec = spack.spec.Spec(pkg_name)
for variant_name in sorted(preferred_variants):
variant = preferred_variants[variant_name]
values = variant.value
@@ -1100,6 +1101,13 @@ def preferred_variants(self, pkg_name):
values = (values,)
for value in values:
+ pkg_variant = spec.package_class.variants[variant_name]
+ variant = pkg_variant.make_variant(value)
+ spec.variants[variant_name] = variant
+ self.validate_variant_value(spec, variant.name, value)
+ self.variant_values_from_specs.add(
+ (pkg_name, variant.name, value)
+ )
self.gen.fact(fn.variant_default_value_from_packages_yaml(
pkg_name, variant.name, value
))
[edit: possibly a few more places as well, still looking] |
|
@tgamblin A minimal fix for this failure is: diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index 4533021209..189ce0726c 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -1028,6 +1028,10 @@ def external_packages(self):
if pkg_name == 'all':
continue
+ # This package does not appear in any repository
+ if pkg_name not in spack.repo.path:
+ continue
+
if 'externals' not in data:
self.gen.fact(fn.external(pkg_name).symbol(positive=False))
diff --git a/lib/spack/spack/util/mock_package.py b/lib/spack/spack/util/mock_package.py
index 4751f5af7e..66c6feb068 100644
--- a/lib/spack/spack/util/mock_package.py
+++ b/lib/spack/spack/util/mock_package.py
@@ -168,3 +168,6 @@ class MockPackage(MockPackageBase):
self.spec_to_pkg["mockrepo." + name] = mock_package
return mock_package
+
+ def __contains__(self, item):
+ return item in self.spec_to_pkg
Notes:
|
|
@aweits @tgamblin Would it be fine with you to:
? |
alalazo
left a comment
alalazo
left a comment
There was a problem hiding this comment.
LGTM. The only modification necessary is a couple of lines posted in the diff above to skip validating external packages that are not found in any repository
tgamblin
force-pushed
the
refactor/new-concretizer-special-specs
branch
from
5d05642 to
2d61848
Compare
Other parts of the concretizer code build up lists of things we can't know without traversing all specs and packages, and they output these list at the very end. The code for this for variant values from spec literals was intertwined with the code for traversing the input specs. This only covers the input specs and misses variant values that might come from directives in packages. - [x] move ad-hoc value handling code into spec_clauses so we do it in one place for CLI and packages - [x] move handling of `variant_possible_value`, etc. into `concretize.lp`, where we can automatically infer variant existence more concisely. - [x] simplify/clarify some of the code for variants in `spec_clauses()`
tgamblin
force-pushed
the
refactor/new-concretizer-special-specs
branch
from
2d61848 to
1bebfb5
Compare
4 participants
Refactoring a few things in anticipation of adding reuse of installed dependencies.
Other parts of the concretizer code build up lists of things we can't know without traversing all specs and packages, and they output these list at the very end.
The code for this for variant values from spec literals was intertwined with the code for traversing the input specs. This only covers the input specs and misses variant values that might come from directives in packages.
variant_possible_value, etc. intoconcretize.lp, where we can automatically infer variant existencemore concisely.
spec_clauses()