Skip to content

Conversation

@ahoarau
Copy link
Collaborator

@ahoarau ahoarau commented Dec 11, 2025

⚠️DO NOT MERGE UNTIL jrl-umi3218/jrl-cmakemodules#798 is merged ⚠️

This PR is a full rewrite of the CMake files with the JRL CMake Modules v2.

  • Full rewrite of the CMake Files in modern CMake
  • Remove submodules: archives can now be used
  • c++17 minimum required
  • reorder tests to separate python and cpp tests
  • 🚧 NIX CI is temporarily disabled

@ahoarau ahoarau marked this pull request as draft December 11, 2025 16:38
@ManifoldFR ManifoldFR marked this pull request as ready for review December 17, 2025 15:02
@ManifoldFR ManifoldFR marked this pull request as draft December 17, 2025 15:02
@ahoarau ahoarau force-pushed the jrl-next branch 2 times, most recently from e114b10 to 7d7943a Compare December 22, 2025 13:28
Copy link
Contributor

@jorisv jorisv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, the CMakeLists are way clearer.


set(TEST_TYPE "boost::optional")
set(MODNAME boost_optional)
configure_file(optional.cpp.in ${CMAKE_CURRENT_SOURCE_DIR}/boost_optional.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a huge fan of generating file in the source directory. It can be commited by mistake.
Maybe allow eigenpy_add_test to have a SOURCE_FILE optional argument that can take absolute path source file as argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was intentional as configure_file checks for the content before writing the file. I reverted to generating in the build dir. That's OK as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commited by mistake

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To remove when generated in binary dir

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commited by mistake

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commited by mistake

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix all options with EIGENPY_ when jrl_option support back compatibility.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do that in another PR.

CMakeLists.txt Outdated
export_boost_default_options()
find_package(Boost REQUIRED)
search_for_boost_python(REQUIRED)
jrl_find_package(Python 3.8 REQUIRED COMPONENTS Interpreter Development.Module NumPy)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you don't use jrl_find_python ?

TEST ${test_name}
PROPERTY
ENVIRONMENT_MODIFICATION
"PYTHONPATH=path_list_prepend:$<TARGET_FILE_DIR:eigenpy>/.."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this one is useful.
This give the build/lib/.. directory where no imported lib can be found.

include/eigenpy/decompositions/SVDBase.hpp
include/eigenpy/decompositions/BDCSVD.hpp
include/eigenpy/decompositions/JacobiSVD.hpp
include/eigenpy/decompositions/minres.hpp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
include/eigenpy/decompositions/minres.hpp
include/eigenpy/decompositions/minres.hpp
# Eigen
include/eigenpy/eigen/EigenBase.hpp

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add ament support before we allow to merge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants