Skip to content

Refactor CMake with JRL CMake Modules v2#437

Draft
ahoarau wants to merge 210 commits into
Simple-Robotics:develfrom
ahoarau:jrl-next
Draft

Refactor CMake with JRL CMake Modules v2#437
ahoarau wants to merge 210 commits into
Simple-Robotics:develfrom
ahoarau:jrl-next

Conversation

@ahoarau

@ahoarau ahoarau commented Dec 11, 2025

Copy link
Copy Markdown
Collaborator

⚠️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
  • Remove external libraries
  • c++17 minimum required
  • pixi support
  • 🚧 NIX CI is temporarily disabled
  • Fix Documentation links
  • Support for find_package components
find_package(proxsuite 0.7.1 CONFIG REQUIRED COMPONENTS proxsuite vectorized)

@ahoarau ahoarau marked this pull request as draft December 11, 2025 16:39
@ahoarau ahoarau force-pushed the jrl-next branch 2 times, most recently from bd8de6d to c36d61e Compare December 22, 2025 14:54

@jorisv jorisv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work

Comment thread .github/workflows/conda/environment.yml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This can be removed. Pixi CI replace the conda-ci.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be removed. The Pixi CI will replace this workflow.

Comment thread bindings/python/proxsuite/__init__.py
"${PROJECT_NAME}-example-py-${EXAMPLE_NAME}"
"${EXAMPLE_FILE}"
"bindings/python"
function(proxsuite_add_python_example name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be added as a test

Comment thread examples/cpp/CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
@ahoarau ahoarau force-pushed the jrl-next branch 6 times, most recently from 930496b to e4a6dd2 Compare January 9, 2026 07:33
@ahoarau ahoarau force-pushed the jrl-next branch 3 times, most recently from 9920160 to 7ce7cbf Compare January 23, 2026 16:09
@ahoarau ahoarau force-pushed the jrl-next branch 2 times, most recently from f85e65e to e238efe Compare February 11, 2026 18:32
@ahoarau ahoarau force-pushed the jrl-next branch 7 times, most recently from b2617af to b0388c0 Compare February 20, 2026 09:26
@ahoarau ahoarau force-pushed the jrl-next branch 2 times, most recently from 7f6f202 to 3d05dbd Compare March 24, 2026 08:52
@ahoarau ahoarau force-pushed the jrl-next branch 2 times, most recently from 28e0fd5 to b09f8a3 Compare April 16, 2026 11:14
ahoarau added 3 commits May 11, 2026 22:35
remove custom functions as they format in a strange way when favour-inlining (the default) is used.

@jorisv jorisv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

When running tests, following tests are failing on Linux with pixi:

         33 - proxsuite-test-cpp.dense_maros_meszaros (SEGFAULT)
         34 - proxsuite-test-cpp.vectorized.dense_maros_meszaros (SEGFAULT)
         35 - proxsuite-test-cpp.sparse_maros_meszaros (SEGFAULT)
         36 - proxsuite-test-cpp.vectorized.sparse_maros_meszaros (SEGFAULT)

Comment on lines +37 to +38
key: ccache-${{ runner.os }}-${{ matrix.os }}-${{ github.sha }}
restore-keys: ccache-${{ runner.os }}-${{ matrix.os }}-

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replace the prefix ccache by an unique one.
Replace runner.os by matrix.build_type.

name: CI - ${{ matrix.os }} - ${{ matrix.build_type }} (APT)
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Run on a docker image to ensure not using modified tools.

Comment on lines +42 to +43
key: ccache-pixi-${{ matrix.os }}-${{ matrix.build_type }}-${{ matrix.cxx_std }}-${{ github.sha }}
restore-keys: ccache-pixi-${{ matrix.os }}-${{ matrix.build_type }}-${{ matrix.cxx_std }}-

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add ${{ matrix.pixi_environment }} so we don't have cache issue if we add a new environment.

Comment on lines +48 to +70
environments: default

- name: Configure
run: pixi run -e default configure --fresh --log-level=DEBUG

- name: Build
run: pixi run -e default build --verbose --parallel 1

- name: Install
run: pixi run -e default install

- name: Test Python import
run: pixi run -e default test-import-python

- name: Run C++ tests
if: ${{ !(startsWith(matrix.os, 'windows-') && matrix.build_type == 'Debug') }}
run: pixi run -e default test

- name: Run packaging tests
run: pixi run -e default test-packaging

- name: Uninstall
run: pixi run -e default uninstall

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Replace default by ${{ matrix.pixi_environment }}

Comment on lines +44 to +45
key: ccache-${{ matrix.ROS_DISTRO }}-${{github.run_id}}
restore-keys: ccache-${{ matrix.ROS_DISTRO }}-

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
key: ccache-${{ matrix.ROS_DISTRO }}-${{github.run_id}}
restore-keys: ccache-${{ matrix.ROS_DISTRO }}-
key: ccache-ci-ros-${{ matrix.ROS_DISTRO }}-${{github.run_id}}
restore-keys: ccache-ci-ros-${{ matrix.ROS_DISTRO }}-

Comment thread bindings/python/proxsuite/__init__.py
Comment thread CMakeLists.txt

# cereal (Serialization)
# NOTE: We have to keep the FetchContent support because:
# - Cereal is not available via CMeel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comment thread CMakeLists.txt
jrl_option(TEST_JULIA_INTERFACE "Run the julia examples as unittest" OFF)
jrl_option(BUILD_WITH_OPENMP_SUPPORT "Build the library with the OpenMP support" OFF)
jrl_option(LINK_PYTHON_INTERFACE_TO_OPENMP "Link OpenMP to the Python interface" ON CONDITION "BUILD_WITH_OPENMP_SUPPORT" FALLBACK OFF)
jrl_option(BUILD_WITH_SERIALIZATION "Build with serialization support" ON CONDITION "BUILD_PYTHON_INTERFACE" FALLBACK OFF)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the link between serialization and python interface ?

Comment thread pixi.toml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Take a look at other pixi.toml file in other maestro project.
We create different feature per optional dependency (here: simde, openmp, cereal) to allow developers to choose on which one they want to know.
Try to mimic as much as possible the one in pinocchio to be standard.

Comment on lines +91 to +95
prek:
name: Prek CI
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please, use pre-commit and pre-commit.ci to be standard with other projects.

Comment thread CMakeLists.txt
jrl_option(BUILD_MAROS_MESZAROS_TESTS "Build the Maros-Meszaros tests, needs matio library" OFF)
jrl_option(ENABLE_WARNINGS "Enable warnings during compilation" OFF)
jrl_option(ENABLE_WARNINGS_AS_ERRORS "Treat all warnings as errors" OFF)
jrl_option(BUILD_PYTHON_INTERFACE "Build the Python bindings" OFF)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This option is set to ON by default on jrl-v1

)
set_tests_properties(
proxsuite-example-python.${name}
PROPERTIES ENVIRONMENT "PYTHONPATH=$<TARGET_FILE_DIR:proxsuite_pywrap>/.." LABELS python

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to use ENVIRONMENT_MODIFICATION to avoid issue with some package manager or custom installation path:

      ENVIRONMENT_MODIFICATION
        "PYTHONPATH=path_list_prepend:$<TARGET_FILE_DIR:proxsuite_pywrap>/.."

)
set_tests_properties(
proxsuite-test-python.${name}
PROPERTIES ENVIRONMENT "PYTHONPATH=$<TARGET_FILE_DIR:proxsuite_pywrap>/.." LABELS python

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to use ENVIRONMENT_MODIFICATION to avoid issue with some package manager or custom installation path:

      ENVIRONMENT_MODIFICATION
        "PYTHONPATH=path_list_prepend:$<TARGET_FILE_DIR:proxsuite_pywrap>/.."

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.

2 participants