MujocoDependencies: Add CMake options to use system versions of dependencies #937
Conversation
|
This changes are tested extensively in traversaro/mujoco-conda-forge-ci#2, a CI job in which all dependencies are installed via conda-forge, and not managed by CMake's FetchContent. |
traversaro
force-pushed
the
usesystemdepsoptions
branch
from
a5ee46e
to
b79dde6
Compare
traversaro
changed the title
|
This patch would be useful in NixOS where packages are built in isolated environments (offline with explicit dependencies). The current approach depends on big patches and some messy |
| set(ENABLE_DOUBLE_PRECISION ON) | ||
| set(CCD_HIDE_ALL_SYMBOLS ON) | ||
| findorfetch( | ||
| USE_SYSTEM_PACKAGE | ||
| OFF | ||
| ${MUJOCO_USE_SYSTEM_ccd} |
There was a problem hiding this comment.
I suggest capital letters when naming the options, not only here, but everywhere in this file
There was a problem hiding this comment.
For consistency I used exactly the style used by the MUJOCO_DEP_VERSION_<pkg> variables defined in this file. Anyhow, I think anything is ok to mujoco's mantainers is ok for me (cc @nimrod-gileadi).
There was a problem hiding this comment.
I was inspired by the other calls of findorfetch, for example in the SampleDependencies and SimulateDependencies, where it is made with capital letters
| ) | ||
| target_compile_options(qhullstatic_r PRIVATE ${MUJOCO_MACOS_COMPILE_OPTIONS}) | ||
| target_link_options(qhullstatic_r PRIVATE ${MUJOCO_MACOS_LINK_OPTIONS}) | ||
| else() |
There was a problem hiding this comment.
TARGETS Qhull::qhullstatic_r in the findorfetch call will simplify this change
There was a problem hiding this comment.
I am not sure of the modification/simplification you are proposing here. Feel free to suggest the specific code modification, thanks!
There was a problem hiding this comment.
The only place we use qhull is: user_mesh.cc
#include <libqhull_r/qhull_ra.h>
So I thought Qhull::qhullstatic_r should be a better target:
findorfetch(
USE_SYSTEM_PACKAGE
${MUJOCO_LIB_USE_SYSTEM_QHULL}
PACKAGE_NAME
qhull
LIBRARY_NAME
qhull
GIT_REPO
https://github.com/qhull/qhull.git
GIT_TAG
${MUJOCO_DEP_VERSION_qhull}
TARGETS
--- qhull
+++ Qhull::qhullstatic_r
EXCLUDE_FROM_ALL
)
But but after a little experimenting I am not sure how it will work.
| @@ -59,20 +59,20 @@ target_link_libraries( | |||
| PUBLIC absl::core_headers | |||
There was a problem hiding this comment.
PUBLIC is probably not needed in the tests, I suggest to make it PRIVATE here and everywhere in this file
There was a problem hiding this comment.
Unless I am missing something, that line of code is not modified by this PR. As you mentioned, this PR is already quite big, and the change you mention seems to be a bit out of scope for this PR.
| GIT_REPOSITORY https://gitlab.com/libeigen/eigen.git | ||
| GIT_TAG ${MUJOCO_DEP_VERSION_Eigen3} | ||
| ) | ||
| FetchContent_Declare( |
There was a problem hiding this comment.
findorfetch can be used for eigen3 as in python/mujoco/CMakeLists.txt
There was a problem hiding this comment.
That is probably possible, but we need to make sure that it works fine with the present workaround for https://gitlab.kitware.com/cmake/cmake/-/issues/15415 (in python/mujoco/CMakeLists.txt Eigen3::Eigen is linked to a shared library, not to a static one). I am not sure when I will be able to do (and extensively test) this modification, so feel free to propose it yourself, thanks!
This PR adds options for using the system versions of MuJoCo dependencies. This options are
OFFby default, but they can be useful for anyone packaging MuJoCo as part of a software distribution.For each MuJoCo dependencies
<dep>, an advanced option (by default OFF) calledMUJOCO_USE_SYSTEM_<dep>is created.To ensure that the
MUJOCO_USE_SYSTEM_<dep>option work as intended, the following additional changes were done:tinyxml2::tinyxml2target to link tinyxml2 : thetinyxml2::tinyxml2target works well both when tinyxml2 is found via find_package or via add_subdirectory/FetchContent, as opposed to thetinyxml2target that only works iftinyxml2is added with add_subdirectory/FetchContent.GTest::gtest,GTest::gtest_mainandGTest::gmockimported targets instead ofgtest,gtest_mainandgmock. As n the case oftinyxml2, the targetgtest,gtest_mainandgmockar defined only when gtest is included viaadd_subdirectory.CCD, it is necessary to ensure thatCCD_STATIC_DEFINEis defined only if the consumed ccd is static, not it if is dynamic (otherwise this will result in Windows linking errors such as Downstream projects on Windows can't link conda-forge/libccd-feedstock#8)