Skip to content
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

Conversation

VincentRouvreau
Copy link
Contributor

@VincentRouvreau VincentRouvreau commented Sep 24, 2024

Fix #580

git checkout master
time cmake .
# real	0m10.676s
# user	0m13.474s
# sys	0m13.880s

git checkout cmake_improvement_with_importlib_metadata
time cmake .
# real	0m2.100s
# user	0m1.833s
# sys	0m2.096s

@VincentRouvreau VincentRouvreau added enhancement New feature or request build The build system (CMake, etc) python3.8 Requires Python >= 3.8 labels Sep 24, 2024
@mglisse
Copy link
Member

mglisse commented Sep 24, 2024

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 import just for this package.

@VincentRouvreau
Copy link
Contributor Author

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 import just for this package.

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)
Copy link
Member

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)?

Copy link
Contributor Author

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)
Copy link
Member

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`.
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@VincentRouvreau VincentRouvreau merged commit 7c3e328 into GUDHI:master Sep 26, 2024
4 checks passed
@VincentRouvreau VincentRouvreau deleted the cmake_improvement_with_importlib_metadata branch September 26, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build system (CMake, etc) enhancement New feature or request python3.8 Requires Python >= 3.8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Python] Enhance third parties version detection
2 participants