Skip to content

Conversation

@Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Feb 3, 2023

(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.

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

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

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
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
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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants