Skip to content

When iterating, make all iteropts except builddependencies lists #4848

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

When iterating, make all iteropts except builddependencies lists #4848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc96e4e
When iterating, make all iteropts except builddependencies lists
bartoldeman Apr 9, 2025
1a5ed61
Create copy for all options in ITERATE_OPTS-iterate_opts
bartoldeman Apr 9, 2025
3052441
Correct ITERATE_OPTIONS
bartoldeman Apr 9, 2025
1535bc9
Adapt test to changed iter_opts dict
bartoldeman Apr 9, 2025
e246ee8
Check for self.cfg.iterate_options instead of #iters>1
bartoldeman Apr 10, 2025
64ca7d8
Add test for PR 4848 testing modification of preconfigopts
bartoldeman Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions easybuild/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2378,19 +2378,24 @@ def handle_iterate_opts(self):
# handle configure/build/install options that are specified as lists (+ perhaps builddependencies)
# set first element to be used, keep track of list in self.iter_opts
# only needs to be done during first iteration, since after that the options won't be lists anymore
if self.iter_idx == 0:
if self.iter_idx == 0 and self.cfg.iterate_options:
# keep track of list, supply first element as first option to handle
for opt in self.cfg.iterate_options:
for opt in ITERATE_OPTIONS:
self.iter_opts[opt] = self.cfg[opt] # copy
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do a deepcopy here just to be safe? IIRC it doesn't change anything for strings but if we were to use e.g. lists now or in the future it would still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

once upon a time, it was a deeper copy, then commit 56e2fc4 changed it from

-                self.iter_opts[opt + suffix] = self.cfg[opt][::-1]  # reversed copy
+                self.iter_opts[opt + suffix] = self.cfg[opt]  # copy

which changed the by-reference semantics. I doubt @boegel remembers though since that change is 12 years old :)

In general, and that applies to the below comment too, for a bugfix release I'd favour the absolute minimal change that fixes the bug, any other nice-to-have should wait.

self.log.debug("Found list for %s: %s", opt, self.iter_opts[opt])
self.log.debug("Iterating opt %s: %s", opt, self.iter_opts[opt])

if self.iter_opts:
print_msg("starting iteration #%s ..." % self.iter_idx, log=self.log, silent=self.silent)
self.log.info("Current iteration index: %s", self.iter_idx)

# pop first element from all iterative easyconfig parameters as next value to use
for opt, value in self.iter_opts.items():
if len(value) > self.iter_idx:
if opt not in self.cfg.iterate_options:
# Use original value even for options that were specified as strings so don't change
# (ie. elements in ITERATE_OPTIONS but not in self.cfg.iterate_options), so they
# are restored after an easyblock such as CMakeMake changes them
self.cfg[opt] = value
elif len(value) > self.iter_idx:
self.cfg[opt] = value[self.iter_idx]
else:
self.cfg[opt] = '' # empty list => empty option as next value
Expand Down
10 changes: 8 additions & 2 deletions test/framework/easyblock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
from easybuild.base import fancylogger
from easybuild.framework.easyblock import EasyBlock, get_easyblock_instance
from easybuild.framework.easyconfig import CUSTOM
from easybuild.framework.easyconfig.easyconfig import EasyConfig
from easybuild.framework.easyconfig.easyconfig import EasyConfig, ITERATE_OPTIONS
from easybuild.framework.easyconfig.tools import avail_easyblocks, process_easyconfig
from easybuild.framework.extensioneasyblock import ExtensionEasyBlock
from easybuild.tools import LooseVersion, config
Expand Down Expand Up @@ -1173,7 +1173,9 @@ def test_handle_iterate_opts(self):
self.assertEqual(eb.cfg.iterate_options, [])
self.assertEqual(eb.cfg['configopts'], ["--opt1 --anotheropt", "--opt2", "--opt3 --optbis"])

expected_iter_opts = {'configopts': ["--opt1 --anotheropt", "--opt2", "--opt3 --optbis"]}
expected_iter_opts = dict.fromkeys(ITERATE_OPTIONS, "")
expected_iter_opts['builddependencies'] = []
expected_iter_opts['configopts'] = ["--opt1 --anotheropt", "--opt2", "--opt3 --optbis"]

# once iteration mode is set, we're still in iteration #0
self.mock_stdout(True)
Expand All @@ -1185,6 +1187,8 @@ def test_handle_iterate_opts(self):
self.assertEqual(eb.cfg.iterating, True)
self.assertEqual(eb.cfg.iterate_options, ['configopts'])
self.assertEqual(eb.cfg['configopts'], "--opt1 --anotheropt")
# mimic easyblock that changes this in-place
eb.cfg['preconfigopts'] = "FOO=bar "
self.assertEqual(eb.iter_opts, expected_iter_opts)

# when next iteration is start, iteration index gets bumped
Expand All @@ -1196,6 +1200,8 @@ def test_handle_iterate_opts(self):
self.assertEqual(stdout, "== starting iteration #1 ...\n")
self.assertEqual(eb.cfg.iterating, True)
self.assertEqual(eb.cfg.iterate_options, ['configopts'])
# preconfigopts should have been restored (https://github.com/easybuilders/easybuild-framework/pull/4848)
self.assertEqual(eb.cfg['preconfigopts'], "")
self.assertEqual(eb.cfg['configopts'], "--opt2")
self.assertEqual(eb.iter_opts, expected_iter_opts)

Expand Down