Refactor CMake with JRL CMake Modules v2 (v2) - #40
Conversation
ManifoldFR
left a comment
There was a problem hiding this comment.
Seems good to me overall 😄
Pull request was converted to draft
5cedb44 to
099bd00
Compare
05572ce to
fcf926d
Compare
|
@nim65s we will need a followup PR for Nix in the future 😃 |
31fceae to
e300dd5
Compare
5b8813f to
d8ac8b7
Compare
7b4631c to
a747b22
Compare
|
|
||
| # Stub generation requires typing-extensions | ||
| # ROS Humble ships an incompatible typing-extensions with python3.10 | ||
| if(Python_VERSION VERSION_GREATER_EQUAL 3.11.0) |
There was a problem hiding this comment.
This mean we don't generate stub on python 3.10 even on non ROS system ?
Do we have another way to detect being build on ROS with python 3.10 ?
There was a problem hiding this comment.
replaced by checking the typing_extension that needs to be >=4.1
There was a problem hiding this comment.
With jrl-umi3218/jrl-cmakemodules#866, we can do a better handling of this version check like this:
if(NANOEIGENPY_GENERATE_STUBS)
if(Python_VERSION VERSION_LESS 3.11)
jrl_check_python_module(typing_extensions 4.1.0 REQUIRED)
endif()
nanobind_add_stub(
nanoeigenpy_stub
VERBOSE
MODULE nanoeigenpy
OUTPUT ${CMAKE_BINARY_DIR}/lib/site-packages/nanoeigenpy.pyi
PYTHON_PATH $<TARGET_FILE_DIR:nanoeigenpy>
DEPENDS nanoeigenpy
)
endif()For the Ubuntu 22 CI, its up to them to manually set -DNANOEIGENPY_GENERATE_STUBS=OFF, otherwise they will have a FATAL_ERROR.
This is more aligned with the "if user wants a feature by setting an option, REQUIRE the dependency".
15991d9 to
740f19f
Compare
eigen does not initialize matrices at construction by default
|
|
||
| jrl_option(BUILD_TESTING "Build the tests" OFF) | ||
|
|
||
| jrl_option(INSTALL_DOCUMENTATION "Generate and install the documentation" OFF) |
There was a problem hiding this comment.
It miss BUILD_DOCUMENTATION and the documentation subdirectory (it was done by default by legacy jrl cmake module)
| ${nanoeigenpy_HEADERS} | ||
| ) | ||
| jrl_target_set_output_directory(nanoeigenpy OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib/site-packages) | ||
| jrl_target_enforce_msvc_conformance(nanoeigenpy PRIVATE) |
There was a problem hiding this comment.
It miss jrl_target_set_default_compile_options
There was a problem hiding this comment.
You put this on the nanoeigenpy_headers as interface. This will be forwarded to all consumer. Better only apply it on tests and binding.
There was a problem hiding this comment.
indeed. Now it's on the python bindings in PRIVATE. (there are no cpp tests)
This PR is a full rewrite of the CMake files with the JRL CMake Modules v2.
pytestNOTE: This the commit are cherry-picked and squashed from the previously accidentitaly merged #28