Skip to content

Conversation

@jack-morrison
Copy link
Member

reframe.core.buildsystems.Spack supports setting install_tree, but there are many other ways to configure a Spack environment. This proposal introduces support for setting arbitrary Spack environment configuration options. The implementation is nearly identical to how install_tree works.

As an example with the changes proposed here, one could set the following:
self.build_system.config_opts = ['concretizer:unify:false']

If the configuration option is not already present in the default spack.yaml, it is added. If the configuration option is present in the default spack.yaml, Spack overwrites them with the values given via spack config add.

@jenkins-cscs
Copy link
Collaborator

Can I test this patch?

@vkarak vkarak changed the title Support arbitrary Spack environment configuration [feat] Support arbitrary Spack environment configuration Mar 7, 2023
@vkarak vkarak requested review from victorusu and vkarak March 7, 2023 13:42
@vkarak vkarak added this to the ReFrame 4.2 milestone Mar 7, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9992947) 86.77% compared to head (e6d6ffc) 86.77%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2819   +/-   ##
========================================
  Coverage    86.77%   86.77%           
========================================
  Files           60       60           
  Lines        11356    11361    +5     
========================================
+ Hits          9854     9859    +5     
  Misses        1502     1502           
Impacted Files Coverage Δ
reframe/core/buildsystems.py 96.18% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jack-morrison
Copy link
Member Author

jack-morrison commented Mar 7, 2023

Assuming this is moving forwards and there're no name changes, etc, I'm happy to add docs changes for this PR as well. I'm wondering if config_opts is clear enough. I guess it does have the context of build_system, but only applies to Spack.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Hi @jack-morrison, thanks for the PR, I think it makes sense.

I would also add a unit test in test_buildsystems.py that simply checks whether the spack config commands are emitted correctly (there are already some examples that you could use as a template for your unit test).

Regarding the docs, I think the documentation of the config_opts is sufficient. Perhaps, you can also add a comment about this in the Spack build system tutorial.

@jack-morrison
Copy link
Member Author

I would also add a unit test in test_buildsystems.py that simply checks whether the spack config commands are emitted correctly (there are already some examples that you could use as a template for your unit test).

Addressed & passing locally for me via 48d5706

@jack-morrison
Copy link
Member Author

Perhaps, you can also add a comment about this in the Spack build system tutorial.

Done via f9bc8e6

@jack-morrison jack-morrison requested review from vkarak and removed request for victorusu March 8, 2023 20:51
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Lgtm!

@vkarak
Copy link
Contributor

vkarak commented Mar 8, 2023

ok to test

@vkarak vkarak requested a review from victorusu March 8, 2023 22:24
@vkarak vkarak merged commit b992afd into reframe-hpc:develop Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants