Fix CMake configopts when iterating (multiple builds) #2885
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Partial revert of easybuilders#2541: Do not modify `cfg['configopts']` but simply read them and pass the combined result to `run_cmd`. This fixes an issue with templating while iterating over e.g. multiple Pythons and also avoids keeping values in configopts over multiple iterations/builds. Fixes easybuilders#2881
boegel
reviewed
Mar 15, 2023
| @@ -139,13 +139,20 @@ def build_type(self): | |||
| build_type = 'Debug' if self.toolchain.options.get('debug', None) else 'Release' | |||
| return build_type | |||
|
|
|||
| def prepend_config_opts(self, config_opts): | |||
| """Prepends configure options (-Dkey=value) to configopts ignoring those already set""" | |||
| def combine_config_opts(self, config_opts_default): | |||
There was a problem hiding this comment.
I don't think this is a good approach to fix #2881, since it removes a method (prepend_config_opts) from a commonly used generic easyblock like CMakeMake.
Although we don't have any easyblocks in the central repository that derive from CMakeMake and call prepend_config_opts, sites could have their own easyblocks that do, so we shouldn't be breaking the API like this.
As mentioned, not updating configopts in place also has other downsides, like easyblocks deriving from CMakeMake not being able to inspect the modified configopts.
|
closing, superseded by #2882 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partial revert of #2514:
Do not modify
cfg['configopts']but simply read them and pass the combined result torun_cmd.This fixes an issue with templating while iterating over e.g. multiple Pythons and also avoids keeping values in configopts over multiple iterations/builds.
Fixes #2881
Alternative to #2882
Potential downside: Easyblocks cannot inspect the configopts actually used.