-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Allow conditional possible values in variants #29530
Allow conditional possible values in variants #29530
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this. Huge thumbs up!
I freely admit I didn't put my braintime into it, but is this composable with conditional variants (#24858)?
I'm curious if there would be any utility in extending this to depends_on
and conflicts
. With those, it would really be more of a convenience function so that a package author could aggregate those calls for a given dependency (we have lots of those in Dyninst). I'm not sure. I can't really make a strong use case for it. Of course, there would need to be a mechanism to pass args that aren't when
.
var/spack/repos/builtin.mock/packages/conditional-values-in-variant/package.py
Outdated
Show resolved
Hide resolved
Not sure what you are suggesting here. I don't see how this can be extended beyond variant values, but was wondering what is your use case. |
In Dyninst, we have
Using the new
As I noted, this is really more of a convenience than a new behavior. I'm still not sure if it would be useful. |
d5f11cc
to
f4fff4a
Compare
f4fff4a
to
150dd8d
Compare
lib/spack/docs/packaging_guide.rst
Outdated
values=( | ||
'98', '11', '14', | ||
# C++17 is not supported by Boost < 1.63.0. | ||
value('17', when='@1.63.0:'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that nobody starts asking for this:
when(
value('17'),
value('18'),
value('19'),
value('20'),
when='@1.63.0:',
)
we should probably allow this:
value('17', '18', '19', '20', when='@1.63.0:')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to check what is needed for this modification. Can we use another name though instead of value
if we have to handle lists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, value
is singular, it's looks odd when it takes multiple values
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values
/ value_list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about conditional
:
variant(
'cxxstd', default='98',
values=(
'98', '11', '14',
# C++17 is not supported by Boost < 1.63.0.
conditional('17', when='@1.63.0:'),
# C++20/2a is not support by Boost < 1.73.0
conditional('2a', '2b', when='@1.73.0:')
)
)
?
lib/spack/spack/variant.py
Outdated
class value(object): | ||
"""Conditional value that might be used in variants.""" | ||
def __init__(self, v, when): | ||
self.variant_value = v |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above -- v
should be *values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I implemented it in in different way:
- The
Value
class above is a single conditional value and remained the same - The
conditional
directive is a factory of lists of these values. - The list of conditional values is flattened at
Variant
creation
Allow declaring possible values for variants which have a condition associated with them. If the variant takes one of those values, the associated condition is imposed as a further constraint.
150dd8d
to
7513e8a
Compare
@tgamblin Ready for another review |
Allow declaring possible values for variants with an associated condition. If the variant takes one of those values, the condition is imposed as a further constraint. The idea of this PR is to implement part of the mechanisms needed for modeling [packages with multiple build-systems]( spack/seps#3). After this PR the build-system directive can be implemented as: ```python variant( 'build-system', default='cmake', values=( 'autotools', conditional('cmake', when='@X.Y:') ), description='...', ) ``` Modifications: - [x] Allow conditional possible values in variants - [x] Add a unit-test for the feature - [x] Add documentation
Allow declaring possible values for variants with an associated condition. If the variant takes one of those values, the condition is imposed as a further constraint. The idea of this PR is to implement part of the mechanisms needed for modeling [packages with multiple build-systems]( spack/seps#3). After this PR the build-system directive can be implemented as: ```python variant( 'build-system', default='cmake', values=( 'autotools', conditional('cmake', when='@X.Y:') ), description='...', ) ``` Modifications: - [x] Allow conditional possible values in variants - [x] Add a unit-test for the feature - [x] Add documentation
Does this allow for conditional defaults? One of the most common questions I see on Slack is "how do I make the default of variant A be True only when variant B is true". |
Also, if I have a MVV, is there a way to set the default to include a value that is conditional on version? Will that be automatically removed for versions where it isn't supported or will that break? |
Also also, what's the best way to get a list of all possible values? I was using |
No, this doesn't allow conditional defaults mainly to avoid complicating the logic for dealing with variant weights (the weight of a variant with conditional defaults will become dependent on other choices the concretizer could make). Can you point to a real use case to get a better understanding of the question? I think we might be able to solve slightly different variations of what you say in a couple of ways. |
That should work. I didn't check explicitly MVV, but in case it's a bug.
Hmm, we don't have a clear API for that - meaning we can work out some. |
@alalazo I think I have an example. I would like to replace spack/var/spack/repos/builtin/packages/llvm/package.py Lines 190 to 195 in b6ea2a4
with variant(
"omp",
values=("project", conditional("runtime", when="+clang @12:")),
default="runtime",
description="Build OpenMP either as a project (with the compiler in use) "
"or as a runtime (with just-build Clang)",
) The reason for that is to make it consistent with another change that I would like to make, namely, replace spack/var/spack/repos/builtin/packages/llvm/package.py Lines 114 to 119 in b6ea2a4
with variant(
"libcxx",
values=("none", "project", conditional("runtime", when="+clang @12:")),
default="runtime",
description="Build the LLVM C++ standard library "
"either as a project (with the compiler in use) "
"or as a runtime (with just-build Clang)",
) The problem is that the first modification seems to revert #30782 and might reintroduce #30700 (I'm not sure though). It would be nice to be able to set |
@skosukhin We might either list those preferences in |
Thanks. Another solution I had in mind for this case was to create a single multi-value variant requested_runtimes = self.spec["runtimes"].value
if "auto" in requested_runtimes:
runtimes = []
if self.spec.satisfies("@12:"):
runtimes.append("omp")
if "+libcxx" in self.spec:
runtimes.append("libcxx")
else:
runtimes = list(requested_runtimes) Would that be better? |
Allow declaring possible values for variants with an associated condition. If the variant takes one of those values, the condition is imposed as a further constraint.
The idea of this PR is to implement part of the mechanisms needed for modeling packages with multiple build-systems. After this PR the build-system directive can be implemented as:
Modifications: