-
Notifications
You must be signed in to change notification settings - Fork 67
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
Cmake improvement with importlib metadata #1135
Cmake improvement with importlib metadata #1135
Conversation
As expected, we need a special case for pykeops, which can be installed on windows even if it cannot be imported. I don't know if we should always disable it on windows, or if we should try the |
I did so on 848d853 |
COMMAND ${Python_EXECUTABLE} -c "import fcntl" | ||
RESULT_VARIABLE FCNTL_IMPORT_MODULE_RESULT | ||
OUTPUT_VARIABLE FCNTL_IMPORT_MODULE_OUPUT) | ||
if(FCNTL_IMPORT_MODULE_RESULT EQUAL 0) |
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.
Is that better than if (NOT WIN32)
?
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, you are right. I wanted to do something smarter, like "the day fcntl will be available on windows, pykeops will work on windows", but that was not that smart...
if (NOT WIN32)
on b4694d9
find_python_module("networkx") | ||
# Specific case for PyKeops that can be imported on Windows, but fails because it uses fcntl (not available on Windows) |
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 it can be installed (pip install pykeops
) but not imported (import pykeops
). That's a detail though.
@@ -20,6 +20,7 @@ Below is a list of changes: | |||
|
|||
- Installation | |||
- CMake ≥ 3.15 is now required (was ≥ 3.8). | |||
- Python ≥ 3.8 is now required (was ≥ 3.5), because of `importlib.metadata`. |
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 the use of type hints also requires 3.8? I don't remember.
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.
In Python 3.5 for type hints
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.
Ok. Note that they added new type hinting possibilities with almost every release, so it is hard to be sure we aren't using modern things that were absent in older releases we are not testing anymore. Anyway, let's ignore this.
Fix #580