-
Notifications
You must be signed in to change notification settings - Fork 283
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
#3399
Conversation
Revised version
- 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
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.
lgtm
One check is just stuck and I see no other way to restart it. |
Test report by @Micket Overview of tested easyconfigs (in order)
Build succeeded for 7 out of 11 (11 easyconfigs in total) |
for
for
@Micket So in both cases we're just missing |
Yes, although: "The following OPTIONAL packages have not been found: Doxygen", might still be better to have it? Actual error is related to this PR though:
I'll have to check the CMakeCache.txt why the "1" is there which doesn't seem to make sense. |
"include_dir": [], | ||
"library": [], | ||
} | ||
python_regex = re.compile(r"_?(?:Python|PYTHON)\d?_(EXECUTABLE|INCLUDE_DIR|LIBRARY)[^=]*=(.*)") |
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.
The [^=]* part matches to much. From my test builds, I have things like
PYTHON_EXECUTABLE-ADVANCED:INTERNAL=1
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.
maybe sufficient to just add a : ?
python_regex = re.compile(r"_?(?:Python|PYTHON)\d?_(EXECUTABLE|INCLUDE_DIR|LIBRARY):[^=]*=(.*)")
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.
That is not enough to match variables with a potential suffixes like "_DIR" or "_DEBUG", like PYTHON_LIBRARY_DEBUG
. However limiting to word characters should work. See the where I also added some examples in the commit message
Variables we want to match include: - PYTHON_EXECUTABLE:FILEPATH=/usr/bin/python3 - PYTHON_INCLUDE_DIR:PATH= - PYTHON_LIBRARY:FILEPATH=/usr/lib/x86_64-linux-gnu/libpython3.6m.so - PYTHON_LIBRARY_DEBUG:FILEPATH=PYTHON_LIBRARY_DEBUG-NOTFOUND - PYTHON_LIBRARY_RELEASE:FILEPATH=PYTHON_LIBRARY_RELEASE-NOTFOUND But not: - PYTHON_EXECUTABLE-ADVANCED:INTERNAL=1 - PYTHON_INCLUDE_DIR-ADVANCED:INTERNAL=1 The type (after colon) is optional. Hence only match word characters but not dashes and explicitely exclude the colon-type combination.
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 0 out of 2 (2 easyconfigs in total) |
@Flamefire Still not there yet, the two easyconfigs in the test report fail with errors like:
|
I added an extra capture group with the last change, so it picked up the type instead of the value... |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 50 out of 52 (52 easyconfigs in total) |
For
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
Various expressions are considered "False" by CMake (matching case insensitively) including especially the empty string which is matched by the `.*` value capture group. Exclude all such values when checking paths. Do the exclusion during the collection which avoids reporting those paths. Use named capture groups for easier understanding which group is considered.
I didn't account for empty cache entries as they usually won't appear. The find-modules would set them to It seems that OpenCV sets those cache entries even if they are empty. I went full way and excluded any value considered "false" by CMake which should make it reasonably robust. We could be more restrictive and use |
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 52 out of 52 (52 easyconfigs in total) |
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.
lgtm
This is #3233 squashed and rebased to 5.0x
There was 1 commit by Danilo Gonzalez which mostly moved and removed code introduced earlier by @MartinsNadia which then was removed/changed again in the "revised version".
To make the rebase possible I squashed that together with all commits from @MartinsNadia into a single one which should be fine as the "revised version" was almost a complete rewrite.