-
Notifications
You must be signed in to change notification settings - Fork 308
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.
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
enhance CMakeMake easyblock to check whether correct Python installation was picked up by cmake
#3233
Changes from 13 commits
7e3b26a
40986c8
133e3b8
775dde0
067c4cd
16b6465
cfbe5e5
3d82787
c0d5e86
f260f73
b60ecd6
77884b9
492c330
c1f005a
4b3653b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,13 +42,13 @@ | |||||||||
| from easybuild.framework.easyconfig import BUILD, CUSTOM | ||||||||||
| from easybuild.tools.build_log import EasyBuildError, print_warning | ||||||||||
| from easybuild.tools.config import build_option | ||||||||||
| from easybuild.tools.filetools import change_dir, create_unused_dir, mkdir, which | ||||||||||
| from easybuild.tools.filetools import change_dir, create_unused_dir, mkdir, read_file, which | ||||||||||
| from easybuild.tools.environment import setvar | ||||||||||
| from easybuild.tools.modules import get_software_root, get_software_version | ||||||||||
| from easybuild.tools.run import run_cmd | ||||||||||
| from easybuild.tools.systemtools import get_shared_lib_ext | ||||||||||
| from easybuild.tools.utilities import nub | ||||||||||
|
|
||||||||||
| import easybuild.tools.toolchain as toolchain | ||||||||||
|
|
||||||||||
| DEFAULT_CONFIGURE_CMD = 'cmake' | ||||||||||
|
|
||||||||||
|
|
@@ -317,9 +317,60 @@ def configure_step(self, srcdir=None, builddir=None): | |||||||||
| self.cfg['configopts']]) | ||||||||||
|
|
||||||||||
| (out, _) = run_cmd(command, log_all=True, simple=False) | ||||||||||
| self.check_python_paths() | ||||||||||
|
|
||||||||||
| return out | ||||||||||
|
|
||||||||||
| def check_python_paths(self): | ||||||||||
| """Check that there are no detected Python paths outside the EB installed PythonF""" | ||||||||||
| if not os.path.exists('CMakeCache.txt'): | ||||||||||
| self.log.warning("CMakeCache.txt not found. Python paths checks skipped.") | ||||||||||
| return | ||||||||||
| cmake_cache = read_file('CMakeCache.txt') | ||||||||||
| if not cmake_cache: | ||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I don't see how this can happen, since at this point you know that the file exists?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but it could be empty for some reason |
||||||||||
| self.log.warning("CMake Cache could not be read. Python paths checks skipped.") | ||||||||||
| return | ||||||||||
|
|
||||||||||
| self.log.info("Checking Python paths") | ||||||||||
|
|
||||||||||
| python_paths = { | ||||||||||
| "executable": [], | ||||||||||
| "include_dir": [], | ||||||||||
| "library": [], | ||||||||||
| } | ||||||||||
| PYTHON_RE = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") | ||||||||||
|
||||||||||
| PYTHON_RE = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") | |
| python_regex = re.compile(r"_?(?:Python|PYTHON)\d_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| match = PYTHON_RE.match(line) | |
| match = python_regex.match(line) |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not, it is likely an error. #3233 (comment) requested to "complain" ;-)
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.