Skip to content

enhance CMakeMake easyblock to check whether correct Python installation was picked up by cmake #3233

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

Closed
MartinsNadia wants to merge 15 commits into from
Closed

enhance CMakeMake easyblock to check whether correct Python installation was picked up by cmake #3233

Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7e3b26a
Sanity check for python configuration
MartinsNadia Feb 19, 2024
40986c8
Sanity check for Python
MartinsNadia Feb 26, 2024
133e3b8
Remove legacy code, integrate python sanity check into configure section
dagonzalezfo Feb 27, 2024
775dde0
Update cmakemake.py
MartinsNadia Feb 27, 2024
067c4cd
teste
MartinsNadia Mar 18, 2024
16b6465
Implementation of path validation for sanitycheck
MartinsNadia Mar 18, 2024
cfbe5e5
Fix identation
MartinsNadia Mar 22, 2024
3d82787
Formatting improvements
MartinsNadia Mar 22, 2024
c0d5e86
Fixing whitespace
MartinsNadia Mar 25, 2024
f260f73
Revised version
MartinsNadia Apr 30, 2024
b60ecd6
Revised version - whitespace
MartinsNadia Apr 30, 2024
77884b9
Improve Python check in cmakemake.py
Flamefire Apr 30, 2024
492c330
Merge pull request #2 from Flamefire/patch-1
MartinsNadia Jun 3, 2024
c1f005a
Address review
Flamefire Jun 25, 2024
4b3653b
Merge pull request #3 from Flamefire/20240625102843_new_pr_cmakemake
MartinsNadia Jul 3, 2024
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
25 changes: 23 additions & 2 deletions easybuild/easyblocks/generic/cmakemake.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,29 @@ def configure_step(self, srcdir=None, builddir=None):

(out, _) = run_cmd(command, log_all=True, simple=False)

return out

# sanitycheck for the configuration step
self.log.info("Checking python paths")
with open('CMakeCache.txt', 'r') as file:
Copy link
Contributor

@Flamefire Flamefire Mar 15, 2024

Choose a reason for hiding this comment

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

This should handle the case that this file may not exist. E.g. when configure_cmd != DEFAULT_CONFIGURE_CMD anything can happen.

lines = file.readlines()
Copy link
Contributor

@Flamefire Flamefire Mar 15, 2024

Choose a reason for hiding this comment

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

Maybe use readfile(...).split('\n')?


# Search for Python paths in each line
for line in lines:
if line.startswith('Python3_EXECUTABLE:FILEPATH='):
Copy link
Member

@ocaisa ocaisa Feb 27, 2024

Choose a reason for hiding this comment

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

It looks like these are only guaranteed for CMake 3.16+ so should probably make this check conditional on that version

Copy link
Member

@ocaisa ocaisa Feb 27, 2024

Choose a reason for hiding this comment

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

They also don't cover the FindPython case: https://cmake.org/cmake/help/latest/module/FindPython.html#artifacts-specification

Copy link
Contributor

@Flamefire Flamefire Mar 15, 2024

Choose a reason for hiding this comment

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

Yes maybe use a regex? Python[2-3]?_EXECUTABLE?

pythonExecPath = line.split('=')[1].strip()
Copy link
Member

@boegel boegel Feb 28, 2024

Choose a reason for hiding this comment

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

Small nitpicking suggestion: let's not start using CamelCase for variable names (we only use that for class names), so please use python_exec_path, python_include_path, python_library_path

elif line.startswith('Python3_INCLUDE_DIR:FILEPATH='):
pythonIncludePath = line.split('=')[1].strip()
elif line.startswith('Python3_LIBRARY:FILEPATH='):
pythonLibraryPath = line.split('=')[1].strip()
self.log.info("Python executable path: %s", pythonExecPath)
self.log.info("Python include path: %s", pythonIncludePath)
self.log.info("Python library path: %s", pythonLibraryPath)
Copy link
Member

@ocaisa ocaisa Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would only print these if they have a value (so move them up into your if/elif loop)

Copy link
Contributor

@Flamefire Flamefire Mar 15, 2024

Choose a reason for hiding this comment

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

Not only that: You could have them multiple times when you consider Python_EXECUTABLE and Python3_EXECUTABLE or the variables may be completely unset if the CMake file didn't use Python at all

Maybe create thos variables as empty lists?

# Check if paths include EBROOTPYTHON
ebrootpython_path = os.getenv('EBROOTPYTHON')
Copy link
Member

@ocaisa ocaisa Feb 27, 2024

Choose a reason for hiding this comment

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

This assumes that EBROOTPYTHON is set, what happens if it isn't?

Copy link
Member

@ocaisa ocaisa Feb 27, 2024

Choose a reason for hiding this comment

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

I imagine that what you want to do is complain if pythonExecPath (and friends) are set but no Python dependency is found

Copy link
Member

@ocaisa ocaisa Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
ebrootpython_path = os.getenv('EBROOTPYTHON')
ebrootpython_path = get_software_root("Python")
if pythonExecPath and not ebrootpython_path:
raise EasyBuildError("Python related path (%s) found but no Python dependency included, check log" % pythonExecPath)

if all(path and ebrootpython_path in path for path in [pythonExecPath, pythonIncludePath, pythonLibraryPath]):
Copy link
Contributor

@Flamefire Flamefire Mar 15, 2024

Choose a reason for hiding this comment

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

See above: pythonExecPath & co might not have been set at all. And maybe path.startswith(ebrootpython_path)? We possibly need to take symlinks into account too: either of the 2 might be a resolved symlink or not.

self.log.info("Python related paths configured correctly.")
return out
Copy link
Member

@ocaisa ocaisa Feb 27, 2024

Choose a reason for hiding this comment

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

Move the return statement out of the if/else block, it is hidden here.

else:
raise EasyBuildError("Python related paths do not include EBROOTPYTHON, check log")
def test_step(self):
"""CMake specific test setup"""
# When using ctest for tests (default) then show verbose output if a test fails
Expand Down