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

use SYSTEM constant for dependency that uses system toolchain in dumped easyconfig #4069

Merged
merged 2 commits into from
Aug 26, 2022

Conversation

boegel
Copy link
Member

@boegel boegel commented Aug 25, 2022

Currently, we usually using True as 4th value in a dependency specification if the system toolchain should be used, for example:

dependencies = [('CUDA', '11.4.1', '', True)]

A more intuitive alternative, which has actually been silently supported already for a long time, is to use the SYSTEM constant instead (since EasyBuild v4.0.0, see #2877):

dependencies = [('CUDA', '11.4.1', '', SYSTEM]

This translates to the following under the covers:

dependencies = [('CUDA', '11.4.1', '', {'name': 'system', 'version': 'system'}]

Directly specifying the toolchain to use for a dependency through the classic name/version dict value has been supported for ages (at least since EasyBuild v2.7.0, but also before already); the SYSTEM constant just makes this (significantly) less ugly.

This practice currently isn't adopted in the central easyconfigs repository though, mainly because the easyconfigs test suite in which we check whether the values of easyconfig parameters for the original easyconfig and for the easyconfig file that's obtained by dumping the parsed easyconfig are equal.
This check fails when using the SYSTEM constant in a dependency specification as shown above, because the dumped easyconfig will use the more cryptic True value instead.

With the changes in this PR, we will be able to use SYSTEM rather than True to indicate that the system toolchain should be used for a dependency for easyconfigs being contributed to the central easyconfigs repository...

Note that this will need a change in the easyconfigs test suite analogous to the workaround that @ocaisa added to easybuilders/easybuild-easyconfigs#15900 though, since the check on the toolchain part of a dependency will fail if True is still being used instead of the SYSTEM constant (which we should keep supporting for now, we can consider deprecating that later, and start requiring that new PRs use SYSTEM rather than True at some point).

@boegel
Copy link
Member Author

boegel commented Aug 25, 2022

Necessary change for easyconfigs test suite is implemented in easybuilders/easybuild-easyconfigs#16126, which should be merged ASAP after this PR gets merged...

Copy link
Contributor

@Micket Micket left a comment

Choose a reason for hiding this comment

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

lgtm

@Micket Micket merged commit b6daaac into easybuilders:develop Aug 26, 2022
@boegel boegel deleted the system_constant_tc_dep branch August 26, 2022 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants