Skip to content

Fix proxQP support#50

Merged
olivier-roussel merged 1 commit intoSofaDefrost:mainfrom
olivier-roussel:fix-proxsuite-support
Jan 20, 2025
Merged

Fix proxQP support#50
olivier-roussel merged 1 commit intoSofaDefrost:mainfrom
olivier-roussel:fix-proxsuite-support

Conversation

@olivier-roussel
Copy link
Contributor

@olivier-roussel olivier-roussel commented Jan 7, 2025

  • Remove Fetch_content support for proxsuite (see comment below). Restrict usage as external dependency only, with a cmake option to enable proxQP support.
  • Enable proxQP support on CI : Requires conda-based CI first (see Adds a conda based CI workflow #53). Adding a CI with proxQP solver enabled will be handled in another PR.

@olivier-roussel olivier-roussel changed the title Fix proxsuite support Fix proxQP support Jan 10, 2025
@olivier-roussel
Copy link
Contributor Author

olivier-roussel commented Jan 10, 2025

As raised by the CI, the Fetch_Content mechanism won't work for compiling proxQP with our windows toolchain.
If I understood correctly, it seems there is a compilator bug with MSVC 2022 and C++20 support with some proxsuite support:
Simple-Robotics/proxsuite#357
It has been fixed in clang-cl v19, but our CI on windows uses cl which is still bugged when enabling C++20 support on proxsuite.
Thus, we have to remove the Fetch_Content mechanism for proxsuite in this plugin, which is in any case a practice I would not recommend, at least for this reason (build chains compatibilities issues), but also for more complex cases where extra dependencies are required. In my opinion, dependencies management should not be handled by cmake, but by one of the tool dedicated to it chosen by the user.

Copy link
Member

@alxbilger alxbilger left a comment

Choose a reason for hiding this comment

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

I don't understand how the CI installs proxqp...

if [[ "${{ matrix.qp_solver }}" == "proxqp" ]]; then
cmake_options="$cmake_options \
-DSOFTROBOTSINVERSE_ENABLE_PROXQP=ON \
-DSOFA_ALLOW_FETCH_DEPENDENCIES=ON \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-DSOFA_ALLOW_FETCH_DEPENDENCIES=ON \
-DSOFA_ALLOW_FETCH_DEPENDENCIES=ON \

Still needed?

@olivier-roussel
Copy link
Contributor Author

I don't understand how the CI installs proxqp...

It does not yet, still working on the PR as it is a draft

@olivier-roussel olivier-roussel force-pushed the fix-proxsuite-support branch 3 times, most recently from 9728e6c to c493597 Compare January 17, 2025 10:11
@olivier-roussel olivier-roussel marked this pull request as ready for review January 17, 2025 10:31
@olivier-roussel
Copy link
Contributor Author

@alxbilger @EulalieCoevoet Does the PR looks OK to you for merging ?

message(FATAL_ERROR "Sofa.SoftRobots.Inverse: DEPENDENCY proxsuite NOT FOUND. SOFA_ALLOW_FETCH_DEPENDENCIES is OFF and thus cannot be fetched. Install proxsuite, or enable SOFA_ALLOW_FETCH_DEPENDENCIES to fix this issue.")
endif()
find_package(proxsuite REQUIRED)
message("Sofa.SoftRobots.Inverse: proxsuite FOUND. Will build with proxsuite support.")
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should keep:

if(proxsuite_FOUND)
        message("Sofa.SoftRobots.Inverse: proxsuite FOUND. Will build with proxsuite support.")
else
        message("Sofa.SoftRobots.Inverse: DEPENDENCY proxsuite NOT FOUND.")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To have a verbose output if proxsuite not found ?
Because that was the purpose of having REQUIRED dependency here: if not found, it fails with error complaining about missing proxsuite dependency. So I think this message would be redundant here, no ?

Copy link
Member

Choose a reason for hiding this comment

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

Okay I see. Good for me then.

@olivier-roussel olivier-roussel merged commit 57c84b0 into SofaDefrost:main Jan 20, 2025
4 checks passed
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