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

deprecate use of True in favour of SYSTEM for system-toolchain dependencies in easyconfigs using a recent toolchain version (>2019b) #16384

Merged

Conversation

jfgrimm
Copy link
Member

@jfgrimm jfgrimm commented Oct 11, 2022

adds a CI check as suggested in #16330
part 2 (updating existing easyconfigs to use SYSTEM) coming soon

@jfgrimm jfgrimm marked this pull request as draft October 11, 2022 10:17
@boegelbot

This comment was marked as outdated.

@casparvl
Copy link
Contributor

@jfgrimm Wouldn't it be easier if this change is also part of #16386 ? Together they should create a CI that passes successfully, right? That's not easy to verify when they are in two different PRs.

@jfgrimm
Copy link
Member Author

jfgrimm commented Oct 11, 2022

@jfgrimm Wouldn't it be easier if this change is also part of #16386 ? Together they should create a CI that passes successfully, right? That's not easy to verify when they are in two different PRs.

I was thinking merge the updates to easyconfigs first, and then the CI after. The switch from True to SYSTEM is already supported, so I'd consider that distinct from the CI enhancement. In any case, any oversights should be caught in this PR

@jfgrimm
Copy link
Member Author

jfgrimm commented Oct 11, 2022

@casparvl besides, the other PR is already so big that it's easier to see the actual CI change when it's separated out

@casparvl
Copy link
Contributor

@jfgrimm Wouldn't it be easier if this change is also part of #16386 ? Together they should create a CI that passes successfully, right? That's not easy to verify when they are in two different PRs.

I was thinking merge the updates to easyconfigs first, and then the CI after. The switch from True to SYSTEM is already supported, so I'd consider that distinct from the CI enhancement. In any case, any oversights should be caught in this PR

Ah you're right, I didn't realize that that one should just pass already with the current develop. Then, indeed, this is the sane way to do it :)

bartoldeman added a commit to bartoldeman/easybuild-easyconfigs that referenced this pull request Oct 11, 2022
…6384)

Co-authored-by: Mikael Öhman <micketeer@gmail.com>
@easybuilders easybuilders deleted a comment from boegelbot Oct 11, 2022
@jfgrimm jfgrimm marked this pull request as ready for review October 12, 2022 08:22
@boegelbot

This comment was marked as outdated.

@boegelbot

This comment was marked as outdated.

Copy link
Contributor

@casparvl casparvl left a comment

Choose a reason for hiding this comment

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

Looks good to me! The CI passes, which is the most important check here. Just as a sanity check, I've also confirmed that the regex indeed correctly matches e.g.

gompi-2016.04.eb
 gompi-2016.06.eb
 gompi-2016.07.eb
 gompi-2016.09.eb
 gompi-2016a.eb
 gompi-2016b.eb
 gompi-2017a.eb
 gompi-2017b.eb
 gompi-2018.08.eb
 gompi-2018a.eb
 gompi-2018b.eb
 gompi-2019a.eb
 gompi-2019b.eb

but not

gompi-2020a.eb
gompi-2020b.eb
gompi-2021a.eb
gompi-2021b.eb
gompi-2022.05.eb
gompi-2022a.eb

as intended.

@casparvl casparvl merged commit 61f1ff3 into easybuilders:develop Oct 13, 2022
@casparvl casparvl removed this from the 4.x milestone Oct 13, 2022
@casparvl casparvl added this to the next release (4.6.2?) milestone Oct 13, 2022
@casparvl
Copy link
Contributor

Thanks @jfgrimm for going through this effort! :)

@boegel boegel changed the title deprecate use of True in favour of SYSTEM for system-toolchain dependencies deprecate use of True in favour of SYSTEM for system-toolchain dependencies in easyconfigs using a recent toolchain version (>2019b) Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants