-
Notifications
You must be signed in to change notification settings - Fork 772
remove superflous -DCMAKE_BUILD_TYPE=Release, use of 'build_type = Release', or enabling separate_build_dir from easyconfigs using CMakeMake easyblock #13384
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
Conversation
9448a47 to
9f790a0
Compare
f0ccda1 to
fcd7b12
Compare
|
Test report by @Flamefire |
|
@Micket Rebuild the whole lot that I still can build (i.e. Intel 2017 doesn't work due to licenses) Would be great if this could be merged soon so future ECs can use the new style/don't C&P old stuff. |
f9017a3 to
3634104
Compare
|
This is all caught up now, so a rebase is the next step? |
7d6a298 to
1b7cf23
Compare
|
Test report by @Flamefire |
|
@Flamefire Do you think it makes sense to add a check in the easyconfigs test suite to avoid that new easyconfigs re-introduce the use of This is still a huge PR to get merged btw, but we'll try... |
|
I'll add a CI check soonish. And yeah, it's big: 64 ECs :/ But the changes are trivial after fleshing out the other PRs. |
|
Test report by @Flamefire |
|
CI check added by #14008 |
|
Racon failure fixed with #14010 |
38414b7 to
d896c10
Compare
|
Rebased to resolve conflicts and remove the changes done in the other PRs. |
|
Test report by @boegel |
boegel
left a comment
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 gave this a thorough review, and submitted a test report for a bunch of recent easyconfigs touched in this PR, looks good to go, thanks for your efforts here @Flamefire!
As next step, we should add a check to the easyconfigs test suite to avoid that easyconfigs being contributed still specify -DCMAKE_BUILD_TYPE in configopts, or set build_type to Release, or enable separate_build_dir...
|
Going in, thanks @Flamefire! |
I usually use plain sentences to describe changes which yields the PR title, i.e. it starts with uppercase. What is the reason for the change to lowercase and do you want that for future PRs? |
|
@Flamefire It's mainly because the PR titles go straight into the release notes, where they're in a list, see https://docs.easybuild.io/en/latest/Release_notes.html I often reword PR titles if I feel they're not clear enough, or to make PRs easier to find, to avoid having to do that when composing the release notes... |
This is the default for a few releases now and may lead to confusion or bugs.
Examples:
RELEASEused instead ofRelease-> confusingONused in 1 place -> bugBonus: CMakeMake reacts on toolchainopts.debug to set the build type correctly which works now after this change
Dependend on: