-
Notifications
You must be signed in to change notification settings - Fork 840
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CMake: target_link_libraries() the native exported targets of Eigen and Ceres #872
Conversation
target_link_libaries(... exportedTarget) instead of the hard-coded Find*.cmake files and variables
8f7a4c8
to
a76671d
Compare
# - if there's no such target, cmake will convert it into -lceres | ||
# and the linker will fail | ||
if(Ceres_ceres_FOUND) | ||
set(CERES_LIBRARIES Ceres::ceres) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still using (but also explicitly defining) the CERES_LIBRARIES
variable because one needs to target_link_libraries
different targets, conditionally on the version of ceres
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done in https://github.com/mapillary/OpenSfM/blob/main/opensfm/src/cmake/FindCeres.cmake ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I'm not sure if it's possible: if we keep FindCeres.cmake
, then find_package(ceres ...)
a few lines above would probably always encounter the in-tree FindCeres.cmake
before it checks the other directories in CMAKE_PREFIX_PATH
. I'll try to research that
if (LAPACK_FOUND) | ||
target_include_directories(bundle PRIVATE ${LAPACK_INCLUDE_DIRS}) | ||
endif() | ||
if (SUITESPARSE_FOUND) | ||
target_include_directories(bundle PRIVATE ${SUITESPARSE_INCLUDE_DIRS}) | ||
endif() | ||
target_include_directories(bundle PRIVATE ${CERES_INCLUDE_DIR} ${CMAKE_SOURCE_DIR}) | ||
target_include_directories(bundle PRIVATE ${CMAKE_SOURCE_DIR}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But otherwise preferring to reference only target names (Ceres::ceres
in target_link_libraries
), instead of lists of paths (CERES_INCLUDE_DIR
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it no longer needed to use the CERES_INCLUDE_DIR? Would this work for someone that has ceres in a different location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! It would because we keep target_link_libraries(... ${CERES_LIBRARIES})
, where ${CERES_LIBRARIES}
refer to an exported Ceres cmake target (it propagates the include directories). The ceres/Ceres::ceres target would be discovered by find_package
through the CMAKE_PREFIX_PATH
environment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, Nix puts ceres in a "custom" location at /nix/store/${hash}-ceres-solver/
, but cmake is able to discover it, because with ceres in "inputs" Nix adds this path to CMAKE_PREFIX_PATH
list
target_link_libraries(bundle_test | ||
PUBLIC | ||
bundle | ||
geometry | ||
Eigen3::Eigen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's target_link_libraries
but it's exported like:
add_library(Eigen3::Eigen INTERFACE IMPORTED)
@fabianschenk has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Eigen3::Eigen
target: https://eigen.tuxfamily.org/dox/TopicCMakeGuide.htmldocker build -f Dockerfile.ceres2 .
andnix build
target_link_libraries
vstarget_include_directories
propagation rules: cf. bottom of the section https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#transitive-usage-requirementsI needed these changes in the process of packaging OpenSfM for nixpkgs, which significantly simplifies the building and deployment compared to plain docker: NixOS/nixpkgs#152957
You can see the build status, including the tests (currently failing to build for macos) at: https://github.com/SomeoneSerge/pkgs/actions
I'd hope to follow up with a few more changes that I found I needed, but I'd need your advice on how to go about them:
Thank you!