Skip to content

Fix missing initialization of CMakeMake in CMakePythonPackage #2876

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

Merged
boegel merged 1 commit into from
Mar 15, 2023
Merged

Fix missing initialization of CMakeMake in CMakePythonPackage #2876

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 0 additions & 5 deletions easybuild/easyblocks/generic/cmakepythonpackage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,11 +53,6 @@ def extra_options(extra_vars=None):
extra_vars['runtest'][0] = None
return extra_vars

def __init__(self, *args, **kwargs):
"""Initialize with PythonPackage."""
PythonPackage.__init__(self, *args, **kwargs)
self._lib_ext = None # From CMakeMake.__init__
Copy link
Member

@boegel boegel Feb 4, 2023

Choose a reason for hiding this comment

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

Removing this makes sense to me, but maybe we should try and hunt down why we have it like this.
There may be a reason it's like this...

I'd say we definitely need to try and test this modified easyblock extensively (ideally with all easyconfigs we have that use CMakePythonPackage, directly or indirectly, although I can't come up with a reason why this would break something, from the top of my head...

Copy link
Contributor Author

@Flamefire Flamefire Feb 5, 2023

Choose a reason for hiding this comment

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

git blame shows that this code is very old. The addition with _lib_ext was done by me when I ran into this earlier and didn't wanted to touch anything else.
So I believe it was rather missing experience (e.g. not knowing about super()) especially with multi-inheritance than intentional.

I did test with GenomeWorks-2021.02.2-fosscuda-2020b.eb (where I encountered it) and also verified that now both __init__ functions are called. Might make sense to test some more but I guess for "plain" ECs using this block there won't be any issues if that one worked so rather other easyblocks deriving from this may be affected although I'd consider that a bug in those as I'm very sure this is correct (now).


def configure_step(self, *args, **kwargs):
"""Main configuration using cmake"""

Expand Down