Allow conditional possible values in variants by alalazo · Pull Request #29530 · spack/spack

alalazo · 2022-03-15T20:37:27Z

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:

variant(
    'build-system',
    default='cmake',
    values=(
        'autotools',
        conditional('cmake', when='@X.Y:')
    ), 
    description='...',
)

Modifications:

Allow conditional possible values in variants
Add a unit-test for the feature
Add documentation

hainest

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

lib/spack/spack/solver/asp.py

alalazo · 2022-03-16T15:59:15Z

I freely admit I didn't put my braintime into it, but is this composable with conditional variants (#24858)?

It is. I added an additional unit test to ensure compatibility with this feature is tested too, see b25ca92

alalazo · 2022-03-16T16:01:03Z

I'm curious if there would be any utility in extending this to depends_on and conflicts.

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.

hainest · 2022-03-16T21:27:25Z

I'm curious if there would be any utility in extending this to depends_on and conflicts.

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

depends_on('[email protected]:', type='link', when='@12.0.1:')
depends_on('[email protected]:', type='link', when='@10.2.0:')
depends_on('elfutils', type='link', when='@9.3.0:10.1')
depends_on('libelf', type='link', when='@:9.2')

Using the new Value type, we could have

depends_on(
  Value('[email protected]:', when='@12.0.1:', type='link'),
  Value('[email protected]:', when='@10.2.0:', type='link'),
  Value('elfutils', when='@9.3.0:10.1', type='link'),
  Value('libelf', when='@:9.2', type='link')
)

As I noted, this is really more of a convenience than a new behavior. I'm still not sure if it would be useful.

lib/spack/spack/variant.py

tgamblin · 2022-03-23T19:05:21Z

lib/spack/docs/packaging_guide.rst

+       values=(
+           '98', '11', '14',
+           # C++17 is not supported by Boost < 1.63.0.
+           value('17', when='@1.63.0:'),


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:')

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?

I agree, value is singular, it's looks odd when it takes multiple values.

values / value_list?

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:') ) )

?

tgamblin · 2022-03-23T19:06:46Z

lib/spack/spack/variant.py

+class value(object):
+    """Conditional value that might be used in variants."""
+    def __init__(self, v, when):
+        self.variant_value = v


see above -- v should be *values

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.

alalazo · 2022-03-24T16:53:57Z

@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

adamjstewart · 2022-05-28T18:41:04Z

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".

adamjstewart · 2022-05-28T18:43:56Z

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?

adamjstewart · 2022-05-29T20:35:26Z

Also also, what's the best way to get a list of all possible values? I was using self.variants['foo'][0].values in cmake_args but that includes conditional values that shouldn't be in that version (not sure if that's a bug or not since self.variants['foo'][0].values isn't really documented anywhere).

alalazo · 2022-05-30T08:24:59Z

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"

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.

alalazo · 2022-05-30T09:03:24Z

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?

That should work. I didn't check explicitly MVV, but in case it's a bug.

Also also, what's the best way to get a list of all possible values? I was using self.variants['foo'][0].values in cmake_args but that includes conditional values that shouldn't be in that version (not sure if that's a bug or not since self.variants['foo'][0].values isn't really documented anywhere)

Hmm, we don't have a clear API for that - meaning we can work out some.

skosukhin · 2022-08-26T13:42:15Z

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.

@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

    
           variant( 
        
               "omp_as_runtime", 
        
               default=True, 
        
               when="+clang @12:", 
        
               description="Build OpenMP runtime via ENABLE_RUNTIME by just-built Clang", 
        
           )

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

    
           variant( 
        
               "libcxx", 
        
               default=True, 
        
               when="+clang", 
        
               description="Build the LLVM C++ standard library", 
        
           )

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 default="none" when ~clang, default="project" when +clang @:11 and default="runtime" when +clang @12:.

alalazo · 2022-08-26T13:59:11Z

@skosukhin We might either list those preferences in packages.yaml under llvm or mark the variant sticky - in which case using an old clang without being explicit on those variant should fail.

skosukhin · 2022-08-26T14:10:16Z

Thanks. Another solution I had in mind for this case was to create a single multi-value variant runtimes with the default auto (disjoint set). I would then implement the logic for auto in the package:

    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?

spackbot-app bot added new-variant new-version tests General test capability(ies) update-package labels Mar 15, 2022

spackbot-app bot requested a review from hainest March 15, 2022 20:37

hainest reviewed Mar 15, 2022

View reviewed changes

var/spack/repos/builtin.mock/packages/conditional-values-in-variant/package.py Outdated Show resolved Hide resolved

lib/spack/spack/solver/asp.py Show resolved Hide resolved

spackbot-app bot added the documentation Improvements or additions to documentation label Mar 16, 2022

alalazo marked this pull request as ready for review March 16, 2022 17:14

hainest approved these changes Mar 16, 2022

View reviewed changes

hainest previously approved these changes Mar 16, 2022

View reviewed changes

alalazo force-pushed the features/conditional_possible_values branch from d5f11cc to f4fff4a Compare March 18, 2022 08:31

alalazo dismissed hainest’s stale review via f4fff4a March 18, 2022 09:41

haampie previously approved these changes Mar 21, 2022

View reviewed changes

tgamblin reviewed Mar 21, 2022

View reviewed changes

lib/spack/spack/variant.py Show resolved Hide resolved

alalazo dismissed haampie’s stale review via 150dd8d March 22, 2022 13:57

alalazo force-pushed the features/conditional_possible_values branch from f4fff4a to 150dd8d Compare March 22, 2022 13:57

spackbot-app bot added the build-systems label Mar 22, 2022

alalazo requested a review from tgamblin March 22, 2022 16:33

tgamblin requested changes Mar 23, 2022

View reviewed changes

alalazo added 4 commits March 24, 2022 11:12

Allow conditional possible values in variants

853a97c

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.

Ensure conditional possible values play well with conditional variants

780a352

Added documentation

4ec51a1

Allow list of values in the new directive, rename it to "conditional"

7513e8a

alalazo force-pushed the features/conditional_possible_values branch from 150dd8d to 7513e8a Compare March 24, 2022 14:44

spackbot-app bot added the utilities label Mar 24, 2022

alalazo added 2 commits March 24, 2022 15:57

Fix documentation text

73cc927

Be compatible with Python 2.7

7f9931b

alalazo requested a review from tgamblin March 24, 2022 16:53

tgamblin approved these changes Apr 5, 2022

View reviewed changes

tgamblin merged commit f2fc4ee into spack:develop Apr 5, 2022

alalazo deleted the features/conditional_possible_values branch April 5, 2022 07:03

alalazo mentioned this pull request Apr 14, 2022

package/plumed: added fine grained control over optional modules #29743

Merged

skosukhin mentioned this pull request Sep 1, 2022

llvm: optionally build compiler-rt, libcxx and libunwind as runtimes #32476

Merged

Conversation

alalazo commented Mar 15, 2022 • edited by tgamblin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hainest left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alalazo commented Mar 16, 2022

Uh oh!

alalazo commented Mar 16, 2022

Uh oh!

hainest commented Mar 16, 2022

Uh oh!

Uh oh!

tgamblin Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

haampie Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

haampie Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgamblin Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo Mar 24, 2022

Choose a reason for hiding this comment

Uh oh!

alalazo commented Mar 24, 2022

Uh oh!

adamjstewart commented May 28, 2022

Uh oh!

adamjstewart commented May 28, 2022

Uh oh!

adamjstewart commented May 29, 2022

Uh oh!

alalazo commented May 30, 2022

Uh oh!

alalazo commented May 30, 2022

Uh oh!

skosukhin commented Aug 26, 2022

Uh oh!

alalazo commented Aug 26, 2022

Uh oh!

skosukhin commented Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alalazo commented Mar 15, 2022 •

edited by tgamblin

Loading

alalazo Mar 24, 2022 •

edited

Loading