Skip to content
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.

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

WIP: Implementation of cmake policy error for python #3055

Closed
wants to merge 8 commits into from

Conversation

MartinsNadia
Copy link

Implementation of cmake policy error for python: https://gitlab.com/eessi/support/-/issues/17

@MartinsNadia
Copy link
Author

This implementation is in progress

@boegel
Copy link
Member

boegel commented Jan 2, 2024

@MartinsNadia Do let us know if you need any help with this (preferable via Slack)!

# Use CMP0094 policy for python resolution. This is only available for CMake versions newer than 3.15 included
cmake_version = get_software_version('CMake')
if LooseVersion(cmake_version) >= LooseVersion('3.15'):
add_cmake_opts['MAKE_POLICY_DEFAULT_CMP0094'] = 'NEW'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
add_cmake_opts['MAKE_POLICY_DEFAULT_CMP0094'] = 'NEW'
add_cmake_opts['CMAKE_POLICY_DEFAULT_CMP0094'] = 'NEW'

https://cmake.org/cmake/help/latest/variable/CMAKE_POLICY_DEFAULT_CMPNNNN.html

@@ -99,6 +100,10 @@ def configure_step(self):
add_cmake_opts = {}
if self.cfg['use_openssl']:
add_cmake_opts['CMAKE_USE_OPENSSL'] = 'ON'
# Use CMP0094 policy for python resolution. This is only available for CMake versions newer than 3.15 included
cmake_version = get_software_version('CMake')
if LooseVersion(cmake_version) >= LooseVersion('3.15'):
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this more readable:

Suggested change
if LooseVersion(cmake_version) >= LooseVersion('3.15'):
if LooseVersion(cmake_version) >= '3.15':

But just a suggestion

@casparvl
Copy link
Contributor

casparvl commented Jan 23, 2024

I was trying to test if this resolved the issue we originally saw in EESSI/software-layer#370 (comment) . For that particular build, it was solved by by specifying the Python executable explicitly through the configopts (see https://github.com/easybuilders/easybuild-easyconfigs/pull/19119/files#diff-dab20f8b8f1b541c80b11cd4ba163c8d9d8300c50c8d74e6ddd8b2d2eac1f098 ). I commented this line to undo that fix and was indeed able to reproduce the original. Then, I tried to use this easyblock: I downloaded it, made the typo-change suggested by @Flamefire and tested. Unfortunately, it still failed. After digging in further, I saw the configure line was NOT setting the policy, and I suddenly realized why: the wrong EasyBlock is being changed here. This EasyBlock is for building CMake itself. The problem occurs when building something else with CMake, i.e. when using the cmakemake.py EasyBlock.

In that EasyBlock, I added:

        # make sure that newer CMAKE picks python based on location, not just the newest python
        options['CMAKE_POLICY_DEFAULT_CMP0094'] = 'NEW'

to the configure_step. After that, casacore (the application I replicated the issue with) passed the configure phase correctly, and has now been building for 45 minutes. I'm assuming it will complete, but the most important thing in the context of solving this issue: the config log shows

-- PYTHON3_EXECUTABLE ......... = /cvmfs/software.eessi.io/versions/2023.06/software/linux/x86_64/amd/zen2/software/Python/3.11.3-GCCcore-12.3.0/bin/python3.11
-- PYTHON3_LIBRARIES .......... = /cvmfs/software.eessi.io/versions/2023.06/software/linux/x86_64/amd/zen2/software/Python/3.11.3-GCCcore-12.3.0/lib/libpython3
.11.so
-- PYTHON3_NUMPY_INCLUDE_DIRS . = /cvmfs/software.eessi.io/versions/2023.06/software/linux/x86_64/amd/zen2/software/SciPy-bundle/2023.07-gfbf-2023a/lib/python3
.11/site-packages/numpy/core/include

i.e. it is picking up the correct Python version.

Of course, we should make that policy setting conditional on the cmake version used; we can use the self.cmake_version variable that is already used in the configure step to do the version comparison.

In summary, I think we should close this PR and reimplement this fix in cmakemake.py.

@casparvl
Copy link
Contributor

Well, since I already had a working cmakemake.py, I just made a new PR right away: #3088
I think with that we can close this PR?

@MartinsNadia
Copy link
Author

Thank you @casparvl !!!
Sorry for this misunderstanding. I will close this PR

@casparvl
Copy link
Contributor

No worries, it took me a while to realize what was going on, it's an easy mistake to make and easy difference to overlook :) Hope I didn't steal your mojo by creating the other PR - After trying to figure out why yours wasn't working, I already had a working EasyBlock for cmakemake, it felt a bit silly to then ask you to (re)create it...

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.

5 participants