Skip to content

Fix missing initialization of CMakeMake in CMakePythonPackage#2876

Merged
boegel merged 1 commit intoeasybuilders:developfrom
Flamefire:20230203150323_new_pr_cmakepythonpackage
Mar 15, 2023
Merged

Fix missing initialization of CMakeMake in CMakePythonPackage#2876
boegel merged 1 commit intoeasybuilders:developfrom
Flamefire:20230203150323_new_pr_cmakepythonpackage

Conversation

@Flamefire
Copy link
Copy Markdown
Contributor

@Flamefire Flamefire commented Feb 3, 2023 •
edited
Loading

(created using eb --new-pr)

The CMakeMake easyblock does some initialization in its __init__ method which get skipped by CMakePythonPackage as that only initializes the PythonPackage class.

That is wrong as changes to the CMakeMake easyblock will be missed if not manually added as done before with self._lib_ext = None.

I observed an actual failure caused by this when testing #2838 which adds another member variable in its init and then e.g. GenomeWorks-2021.02.2-fosscuda-2020b.eb fails with

ERROR: Traceback (most recent call last):
  File "/easybuild-framework/easybuild/main.py", line 132, in build_and_install_software
    (ec_res['success'], app_log, err) = build_and_install_one(ec, init_env)
  File "/easybuild-framework/easybuild/framework/easyblock.py", line 4187, in build_and_install_one
    result = app.run_all_steps(run_test_cases=run_test_cases)
  File "/easybuild-framework/easybuild/framework/easyblock.py", line 4070, in run_all_steps
    self.run_step(step_name, step_methods)
  File "/easybuild-framework/easybuild/framework/easyblock.py", line 3905, in run_step
    step_method(self)()
  File "/easybuild-easyblocks/easybuild/easyblocks/generic/cmakepythonpackage.py", line 65, in configure_step
    return CMakeMake.configure_step(self, *args, **kwargs)
  File "/easybuild-easyblocks/easybuild/easyblocks/generic/cmakemake.py", line 252, in configure_step
    if LooseVersion(self.cmake_version) < LooseVersion('2.8.0') or cache_exists:
  File "/easybuild-easyblocks/easybuild/easyblocks/generic/cmakemake.py", line 139, in cmake_version
    if self._cmake_version is None:
AttributeError: 'CMakePythonPackage' object has no attribute '_cmake_version'

This fixes it by simply removing the custom (incomplete) __init__ function. Now the build succeeds.

@Flamefire Flamefire mentioned this pull request Feb 3, 2023
Merged
2 tasks
boegel
boegel reviewed Feb 4, 2023
View reviewed changes
Comment thread easybuild/easyblocks/generic/cmakepythonpackage.py
def __init__(self, *args, **kwargs):
"""Initialize with PythonPackage."""
PythonPackage.__init__(self, *args, **kwargs)
self._lib_ext = None # From CMakeMake.__init__
Copy link
Copy Markdown
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
Copy Markdown
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).

@boegel boegel added the bug fix label Feb 4, 2023
@boegel boegel added this to the next release (4.7.1?) milestone Feb 4, 2023
@boegel
Copy link
Copy Markdown
Member

boegel commented Mar 15, 2023

Test report by @boegel

Overview of tested easyconfigs (in order)

  • SUCCESS DOLFIN-2019.1.0.post0-foss-2019b-Python-3.7.4.eb
  • SUCCESS tmap-20220502-GCC-11.2.0.eb
  • SUCCESS pygmo-2.18.0-foss-2021b.eb
  • SUCCESS ITK-5.1.2-foss-2020a-Python-3.8.2.eb
  • SUCCESS gemmi-0.4.5-GCCcore-10.2.0.eb
  • SUCCESS GenomeWorks-2021.02.2-fosscuda-2020b.eb
  • SUCCESS CRPropa-3.1.6-foss-2020a-Python-3.8.2.eb
  • SUCCESS pybind11-2.9.2-GCCcore-11.3.0.eb

Build succeeded for 8 out of 8 (8 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/d07ea827c2fdce35a8dadd42ab754286 for a full test report.

@boegel
Copy link
Copy Markdown
Member

boegel commented Mar 15, 2023

Tests tell me it should be OK to merge, so merging... Thanks @Flamefire!

@boegel boegel merged commit 663d2bb into easybuilders:develop Mar 15, 2023
@Flamefire Flamefire deleted the 20230203150323_new_pr_cmakepythonpackage branch March 15, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@boegel boegel boegel approved these changes

Assignees

No one assigned

Labels

bug fix

Projects

None yet

Milestone

4.7.1

Development

Successfully merging this pull request may close these issues.

2 participants

@Flamefire @boegel