enhance CMakeMake easyblock to check whether correct Python installation was picked up by cmake
#3233
Conversation
| ebrootpython_path = os.getenv('EBROOTPYTHON') | ||
| if all(path and ebrootpython_path in path for path in [pythonExecPath, pythonIncludePath, pythonLibraryPath]): | ||
| self.log.info("Python related paths configured correctly.") | ||
| return out |
There was a problem hiding this comment.
Move the return statement out of the if/else block, it is hidden here.
| self.log.info("Python include path: %s", pythonIncludePath) | ||
| self.log.info("Python library path: %s", pythonLibraryPath) | ||
| # Check if paths include EBROOTPYTHON | ||
| ebrootpython_path = os.getenv('EBROOTPYTHON') |
There was a problem hiding this comment.
This assumes that EBROOTPYTHON is set, what happens if it isn't?
There was a problem hiding this comment.
I imagine that what you want to do is complain if pythonExecPath (and friends) are set but no Python dependency is found
| self.log.info("Python executable path: %s", pythonExecPath) | ||
| self.log.info("Python include path: %s", pythonIncludePath) | ||
| self.log.info("Python library path: %s", pythonLibraryPath) |
There was a problem hiding this comment.
I would only print these if they have a value (so move them up into your if/elif loop)
There was a problem hiding this comment.
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?
|
|
||
| # Search for Python paths in each line | ||
| for line in lines: | ||
| if line.startswith('Python3_EXECUTABLE:FILEPATH='): |
There was a problem hiding this comment.
It looks like these are only guaranteed for CMake 3.16+ so should probably make this check conditional on that version
There was a problem hiding this comment.
They also don't cover the FindPython case: https://cmake.org/cmake/help/latest/module/FindPython.html#artifacts-specification
There was a problem hiding this comment.
Yes maybe use a regex? Python[2-3]?_EXECUTABLE?
| self.log.info("Python include path: %s", pythonIncludePath) | ||
| self.log.info("Python library path: %s", pythonLibraryPath) | ||
| # Check if paths include EBROOTPYTHON | ||
| ebrootpython_path = os.getenv('EBROOTPYTHON') |
There was a problem hiding this comment.
| 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) |
| # Search for Python paths in each line | ||
| for line in lines: | ||
| if line.startswith('Python3_EXECUTABLE:FILEPATH='): | ||
| pythonExecPath = line.split('=')[1].strip() |
There was a problem hiding this comment.
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
boegel
changed the title
CMakeMake easyblock to check whether correct Python installation was picked up by cmake
|
|
||
| # sanitycheck for the configuration step | ||
| self.log.info("Checking python paths") | ||
| with open('CMakeCache.txt', 'r') as file: |
There was a problem hiding this comment.
This should handle the case that this file may not exist. E.g. when configure_cmd != DEFAULT_CONFIGURE_CMD anything can happen.
| # sanitycheck for the configuration step | ||
| self.log.info("Checking python paths") | ||
| with open('CMakeCache.txt', 'r') as file: | ||
| lines = file.readlines() |
There was a problem hiding this comment.
Maybe use readfile(...).split('\n')?
|
|
||
| # Search for Python paths in each line | ||
| for line in lines: | ||
| if line.startswith('Python3_EXECUTABLE:FILEPATH='): |
There was a problem hiding this comment.
Yes maybe use a regex? Python[2-3]?_EXECUTABLE?
| self.log.info("Python executable path: %s", pythonExecPath) | ||
| self.log.info("Python include path: %s", pythonIncludePath) | ||
| self.log.info("Python library path: %s", pythonLibraryPath) |
There was a problem hiding this comment.
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?
| self.log.info("Python library path: %s", pythonLibraryPath) | ||
| # Check if paths include EBROOTPYTHON | ||
| ebrootpython_path = os.getenv('EBROOTPYTHON') | ||
| if all(path and ebrootpython_path in path for path in [pythonExecPath, pythonIncludePath, pythonLibraryPath]): |
There was a problem hiding this comment.
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.
| except FileNotFoundError: | ||
| self.log.warning("CMakeCache.txt not founf. Python paths checks skipped.") | ||
| return | ||
| # EBROOTPYTHON check |
There was a problem hiding this comment.
And please remove this comment. It doesn't add any information to the code below
There was a problem hiding this comment.
Please recheck also the use(s) of self.log.error. When raising an EasyBuildError you don't need to log the same because that will already be done.
I also thing that you don't need self.log.error but only EasyBuildError, i.e. fail if there is an error.
Another enhancement would be to gather all issues (e.g. each wrong path) in a list and then fail if that list is non-empty. This way ALL issues would be reported, not only the first found which might help in fixing this.
| python_prefixes = r"(Python|Python2|Python3)_" | ||
|
|
||
| for line in lines: | ||
| if line.startswith(python_prefixes + r"EXECUTABLE:FILEPATH="): |
There was a problem hiding this comment.
This doesn't work. python_prefixes is all lowercase and you likely intended to use something like re.match. Got to look up the docs but from the top of my head: python_prefixes = "PYTHON[23]?_" above and re.match(python_prefixes + "EXECUTABLE(:FILEPATH)?=", line) here would be needed
| python_library_path.append(line.split('=')[1].strip()) | ||
| self.log.info("Python library path: %s", python_library_path[-1]) | ||
|
|
||
| # Check if paths are found and validate paths for symlinks |
There was a problem hiding this comment.
Everything from here should be outside the loop shouldn't it?
|
|
||
| # Check if paths are found and handle EBROOTPYTHON cases | ||
| if any(path for path in [python_exec_path, python_include_path, python_library_path]): | ||
| if not os.getenv('EBROOTPYTHON'): |
There was a problem hiding this comment.
This looks like a duplication to the below (ebrootpython_path etc). Maybe at least put them together? Are both even required? I.e. isn't ebrootpython_path enough/better than EBROOTPYTHON?
| ) | ||
| elif not all( | ||
| (path and os.getenv('EBROOTPYTHON') in path) | ||
| for path in [python_exec_path, python_include_path, python_library_path] |
There was a problem hiding this comment.
You are checking only for $EBROOTPYTHON being in a list of paths, which likely is always false. Maybe rename the variables to be able to spot that: python_exec_path -> python_exec_paths and so on.
It might even make sense to show which path is "wrong". I.e. which of the 3 lists and which exact path is outside EB.
- Reintroduce function such that missing early returns can be used - Fix regexp (Superflous group, optional stuff) - Handle the case where the cache path is not found - Handle $EBROOTPYTHON not set and comparing it in the presence of symlinks
Improve Python check in cmakemake.py
|
|
||
| return out | ||
|
|
||
| def check_python_paths(self): | ||
| """Check that there are no detected Python paths outside the EB installed PythonF""" |
There was a problem hiding this comment.
| """Check that there are no detected Python paths outside the EB installed PythonF""" | |
| """Check that there are no detected Python paths outside the Python dependency provided by EasyBuild""" |
| self.log.warning("CMakeCache.txt not found. Python paths checks skipped.") | ||
| return | ||
| cmake_cache = read_file('CMakeCache.txt') | ||
| if not cmake_cache: |
There was a problem hiding this comment.
Hmm, I don't see how this can happen, since at this point you know that the file exists?
There was a problem hiding this comment.
Yes, but it could be empty for some reason
| "include_dir": [], | ||
| "library": [], | ||
| } | ||
| PYTHON_RE = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") |
There was a problem hiding this comment.
Don't use "constants" inline:
| PYTHON_RE = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") | |
| python_regex = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") |
| } | ||
| PYTHON_RE = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") | ||
| for line in cmake_cache.splitlines(): | ||
| match = PYTHON_RE.match(line) |
There was a problem hiding this comment.
| match = PYTHON_RE.match(line) | |
| match = python_regex.match(line) |
| ebrootpython_path = get_software_root("Python") | ||
| if not ebrootpython_path: | ||
| if any(python_paths.values()) and not self.toolchain.comp_family() == toolchain.SYSTEM: | ||
| self.log.warning("Found Python paths in CMake cache but Python is not a dependency") |
There was a problem hiding this comment.
I wouldn't make this a warning, just info message?
| self.log.warning("Found Python paths in CMake cache but Python is not a dependency") | |
| self.log.info("Found Python paths in CMake cache but Python is not a dependency") |
There was a problem hiding this comment.
I think not, it is likely an error. #3233 (comment) requested to "complain" ;-)
| if path.endswith('-NOTFOUND'): | ||
| continue | ||
| if not os.path.exists(path): | ||
| errors.append(f"Python {path_type} does not exist: {path}") |
There was a problem hiding this comment.
Since this is a PR to develop, we need to be careful with f-strings, since EasyBuild 4.x is still supposed to support Python 2.7:
| errors.append(f"Python {path_type} does not exist: {path}") | |
| errors.append("Python %s does not exist: %s" % (path_type, path)) |
| if match: | ||
| path_type = match[1].lower() | ||
| path = match[2].strip() | ||
| self.log.info(f"Python {path_type} path: {path}") |
There was a problem hiding this comment.
Since this is a PR to develop, we need to be careful with f-strings, since EasyBuild 4.x is still supposed to support Python 2.7:
| self.log.info(f"Python {path_type} path: {path}") | |
| self.log.info("Python %s path: %s" % (path_type, path)) |
There was a problem hiding this comment.
No need to format before logging:
| self.log.info(f"Python {path_type} path: {path}") | |
| self.log.info("Python %s path: %s", path_type, path) |
| if not os.path.exists(path): | ||
| errors.append(f"Python {path_type} does not exist: {path}") | ||
| elif not os.path.realpath(path).startswith(ebrootpython_path): | ||
| errors.append(f"Python {path_type} path '{path}' is outside EBROOTPYTHON ({ebrootpython_path})") |
There was a problem hiding this comment.
Since this is a PR to develop, we need to be careful with f-strings, since EasyBuild 4.x is still supposed to support Python 2.7:
| errors.append(f"Python {path_type} path '{path}' is outside EBROOTPYTHON ({ebrootpython_path})") | |
| errors.append("Python %s path '%s' is outside EBROOTPYTHON (%s)" % (path_type, path, ebrootpython_path)) |
| if errors: | ||
| # Combine all errors into a single message | ||
| error_message = "\n".join(errors) | ||
| raise EasyBuildError(f"Python path errors:\n{error_message}") |
There was a problem hiding this comment.
Since this is a PR to develop, we need to be careful with f-strings, since EasyBuild 4.x is still supposed to support Python 2.7:
| raise EasyBuildError(f"Python path errors:\n{error_message}") | |
| raise EasyBuildError("Python path errors:\n" + error_message) |
|
@boegelbot please test @ generoso |
|
@boegel: Request for testing this PR well received on login1 PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2188166362 processed Message to humans: this is just bookkeeping information for me, |
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 4 out of 4 (4 easyconfigs in total) |
|
I added MartinsNadia#3 to get this done fast. It addresses the review and handles the "FindPython" (w/o version number) case. |
|
@MartinsNadia ping on reviewing/merging MartinsNadia#3 to update this PR? |
Sanity check to verify if Cmake apply the new policy CMP0094