-
Notifications
You must be signed in to change notification settings - Fork 55
Introduce JRL CMake Modules v2 #798
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
base: master
Are you sure you want to change the base?
Conversation
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.
We must add copyright on all new files:
# Copyright 2025 INRIAThere 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.
Done ✅
| @@ -0,0 +1,58 @@ | |||
| # jrl_boostpy_add_module(name [sources...]) | |||
| function(jrl_boostpy_add_module name) | |||
| python_add_library(${name} MODULE WITH_SOABI ${ARGN}) | |||
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.
This function depend of find_package(Python) call. We need to add guards to print a nice error if python_add_library is not defined.
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.
Done for the Boost::python target as well
| mgr = DllDirectoryManager() | ||
| module_path = os.path.dirname(__file__) | ||
| mgr.safe_add_dll_directory(os.path.join(module_path, '..', '..', '..', 'bin')) | ||
| dll_dirs = @__DLL_DIRS__@ | ||
| for dll_dir in dll_dirs: | ||
| mgr.safe_add_dll_directory(os.path.join(module_path, dll_dir)) |
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 DllDirectoryManager.__del__ will never be called here.
mgr will never be delete when we import @__MODULE_NAME__@.
You can check it with toto.py:
class Toto:
def __init__(self):
print("toto.__init__")
def __del__(self):
print("toto.__del__")
t = Toto()If you import toto Only enter will be called.
You need to use contextlib to handle it well.
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.
fixed, thank for the insights. ✅
|
|
||
| include(${CMAKE_CURRENT_SOURCE_DIR}/modules/jrl.cmake) | ||
|
|
||
| if(CMAKE_UTILS_BUILD_TESTS) |
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.
JRL_CMAKEMODULES_BUILD_TESTS is more appropriate.
Also, this should be an option.
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.
oops I forgot that one, done ✅
| @@ -1,5 +1,10 @@ | |||
| cmake_minimum_required(VERSION 3.22) | |||
|
|
|||
| if(JRL_CMAKEMODULES_USE_V2) | |||
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.
JRL_CMAKEMODULES_USE_V2 should be an option
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.
done ✅
| endif() | ||
| endfunction() | ||
|
|
||
| function(jrl_python_generate_init_py name) |
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.
Add some documentation
| if(NOT Python_EXECUTABLE) | ||
| message( | ||
| FATAL_ERROR | ||
| "Python_EXECUTABLE not defined. | ||
| Please use jrl_find_package(Python REQUIRED COMPONENT Interpreter)" | ||
| ) | ||
| endif() |
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.
Why not use jrl_check_var_defined ?
Also, in BoostPython.cmake you use $<TARGET_FILE:Python::Interpreter>. Which one is the best ?
v2/modules/jrl.cmake
Outdated
| if(NOT Python_EXECUTABLE) | ||
| message( | ||
| FATAL_ERROR | ||
| "Python_EXECUTABLE not defined. | ||
| Please use jrl_find_package(Python REQUIRED COMPONENT Interpreter)" | ||
| ) | ||
| endif() |
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.
Why not use jrl_check_var_defined ?
Also, in BoostPython.cmake you use $<TARGET_FILE:Python::Interpreter>. Which one is the best ?
| endif() | ||
| endfunction() | ||
|
|
||
| function(jrl_python_compute_install_dir output) |
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.
Add some documentation
| if(NOT PROJECT_IS_TOP_LEVEL) | ||
| return() | ||
| endif() | ||
|
|
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 don't get it. We will return if included from the main CMakeLists.txt. Then we will not generate the configuration file.
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.
It's maybe clearer to directly include v2/modules/jrl.cmake in the PACKAGE_TOP_MACROS.
the dll needs to be closed after the import coal call
otherwise, the dll are already close():28
also fixes an error when running in script mode
…ke the installed version
remove the test for now
…on dir
This commit might be reverted as we already have ${PROJECT_NAME}_PYTHON_INSTALL_DIR to override the installation path
This reverts commit 8148456.
this is a proper way to enfore the policy CMP0057, required to NEW on CMake 3.22
…build in build dir
…ve error messaging
In this PR we introduce the
JRL CMake Modules v2, extracted from the repo: https://github.com/ahoarau/jrl-cmakemodules-v2The integration on the user-side can be done as follows:
We can still use the repo
jrl-cmakemodulesas usual, i.e. withpixior manually installing it.Either set an env variable
JRL_CMAKEMODULES_SOURCE_DIRpointing to the jrl repo source location or use the installed version (find_package(...)).Then define
JRL_CMAKEMODULES_USE_V2 ONto bypass thev1and use exclusively thev2.We do not intend to use both
v1andv2in projects.TODO:
COMPATIBLITY_OPTIONforjrl_cmake_dependent_option()release.cmakein python (help wanted)