Skip to content

handle templating correctly in CMakeMake when playing with configopts#2882

Merged
boegel merged 4 commits intoeasybuilders:developfrom
ComputeCanada:fix-cmake-templating
Mar 15, 2023
Merged

handle templating correctly in CMakeMake when playing with configopts#2882
boegel merged 4 commits intoeasybuilders:developfrom
ComputeCanada:fix-cmake-templating

Conversation

@mboisson
Copy link
Copy Markdown
Contributor

@mboisson mboisson commented Feb 8, 2023 •
edited by boegel
Loading

fixes #2881

@mboisson mboisson changed the title handle templating correctly when playing with configopts handle templating correctly in CMakeMake when playing with configopts Feb 8, 2023
boegel
boegel requested changes Feb 9, 2023
View reviewed changes
Comment thread easybuild/easyblocks/generic/cmakemake.py Outdated
@boegel boegel added the bug fix label Feb 9, 2023
@boegel boegel added this to the next release (4.7.1?) milestone Feb 9, 2023
@Flamefire Flamefire mentioned this pull request Feb 9, 2023
Closed
Flamefire
Flamefire suggested changes Feb 9, 2023
View reviewed changes
Comment thread easybuild/easyblocks/generic/cmakemake.py Outdated
@Flamefire Flamefire mentioned this pull request Feb 9, 2023
Closed
Flamefire
Flamefire approved these changes Feb 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire 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 now but I was looking into another approach: Not modifying configopts at all: #2885

I noticed another issue with #2514: When iterating the values from a previous iteration will still be in configopts. So I see a much larger issue with the implementation of the iteration: Shouldn't the iterations be independent of each other? EasyBlocks may rightfully modify the self.cfg values to keep them across steps but that means the next iteration does not start with the same values as it should.

Until that is fixed in framework #2885 stops modifying configopts as it was the case before #2514

boegel
boegel approved these changes Mar 15, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel
Copy link
Copy Markdown
Member

boegel commented Mar 15, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS MetaBAT-2.14-gompi-2019a.eb

Build succeeded for 1 out of 1 (1 easyconfigs in total)
node3170.skitty.os - Linux RHEL 8.6, x86_64, Intel(R) Xeon(R) Gold 6140 CPU @ 2.30GHz (skylake_avx512), Python 3.6.8
See https://gist.github.com/0228bca735afebd25c801a3242cdb434 for a full test report.

@boegel boegel merged commit 1089a62 into easybuilders:develop Mar 15, 2023
@mboisson mboisson deleted the fix-cmake-templating branch March 15, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@branfosj branfosj branfosj left review comments

@boegel boegel boegel approved these changes

+1 more reviewer

@Flamefire Flamefire Flamefire approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.7.1

Development

Successfully merging this pull request may close these issues.

Regression: Broken templating in EasyBuild 4.7.0

4 participants

@mboisson @boegel @branfosj @Flamefire