When iterating, make all iteropts except builddependencies lists #4848
Conversation
Since some easyblocks, e.g. `CMakeMake` modify `self.cfg['configopts']` it's safer to start afresh for every iteration. This PR implements this by turning options like these, if not already lists, into lists with identical element copies, e.g. `[self.cfg['configopts']] * <number_of_iterations>` This modification does not affect the logic of iterative `builddependencies` itself since it is used in some other places in the code.
|
Framework alternative to easybuilders/easybuild-easyblocks#3693 Still needs a test. |
easybuild/framework/easyblock.py
Outdated
| self.iter_opts[opt] = self.cfg[opt] # copy | ||
| elif iter_cnt > 1: | ||
| # make iter_cnt copies for every opt since easyblocks can modify them | ||
| self.iter_opts[opt] = [self.cfg[opt]] * iter_cnt |
There was a problem hiding this comment.
Although this will help, it's not a perfect solution since for some easyconfig parameters (which have a list or tuple value but are not iterated over), we won't be doing a proper reset...
Can we come up with a more robust way or resetting all easyconfig parameters before we start a new iteration?
There was a problem hiding this comment.
it's only a few easyconfig parameters that are affected by this logic:
ITERATE_OPTIONS = ['builddependencies',
'preconfigopts', 'configopts', 'prebuildopts', 'buildopts', 'preinstallopts', 'installopts']
which are normally always strings right?
The exception being builddependencies; I will have to special case that one here I think.
There was a problem hiding this comment.
I simplified this PR by only touching one file, and it passes the test suite now, so I'll undraft.
It'd be more complicated to add a test that fails without the PR and passes with it. Is that needed @boegel even if the actual behaviour of the function is tested (and yes self.iter_opts now includes all iterable options from ITERATE_OPTIONS including those that stay constant, so it changes how the function affects self).
If the above are a problem one quick fix for 5.0.1 would be to use $EBROOTPYTHON in the easyblock instead of get_software_root("python") in what is passed to cmake.
There was a problem hiding this comment.
Adding a test that mimics the buggy behaviour was quite easy in the end. So that addresses the above "more complicated" bit :)
There was a problem hiding this comment.
oh I may have misunderstood the all easyconfig parameters, it's more a wish list.
IMHO that would be a much bigger change, not for a bugfix release. You could also argue that a reset is only really needed for all easyconfig parameters that are affected by iteration; there really is no point doing it for checksums and sources, though one could reasonably make things like pretestopts and testopts iterable and saved (they aren't now).
I'd then much rather than go for a way of making most easyconfig parameters read-only, i.e. __setitem__ can complain when an easyblock changes something it shouldn't.
This way each iteration will start from the original copy of configopts etc, as it's in the iter_opts dict.
This deals with easyconfigs that have lists with just one element
so they will be iterated: `toy.eb` in tests has
`prebuildopts = "echo \"int main() { return 0; }\" > exts-git.c && ",`
the trailing comma makes that a tuple with one element.
| # 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Looks good to me, I expect this works. Instead of using ITERATE_OPTIONS we could use another constant which for now has the same values but could be extended if the need arises. It should basically contain all relevant keys we want to save/restore. We could also use |
Apply this patch directly to the code without easybuilders#4848 makes it fail with AssertionError: 'FOO=bar ' != '' - FOO=bar +
Since some easyblocks, e.g.
CMakeMakemodifyself.cfg['configopts']it's safer to start afresh for every iteration. This PR implements this by turning options like these, if not already lists, into lists with identical element copies, e.g.[self.cfg['configopts']] * <number_of_iterations>This modification does not affect the logic of iterative
builddependenciesitself since it is used in some other places in the code.