Skip to content
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

Include ceres-solver as fetchcontent directory #451

Closed
Jerry-Ma opened this issue Jan 14, 2019 · 21 comments
Closed

Include ceres-solver as fetchcontent directory #451

Jerry-Ma opened this issue Jan 14, 2019 · 21 comments
Assignees
Labels
cmake CMake build related issues
Milestone

Comments

@Jerry-Ma
Copy link

Jerry-Ma commented Jan 14, 2019

Hi,

I am trying to use ceres-solver (the master branch)with the development HEAD of Eigen, both are included as FetchContent projects:

include(FetchContent)
function(FetchContentHelper name vc url tag)
    set(options ADD_SUBDIR)
    set(mvargs CONFIG_SUBDIR)
    cmake_parse_arguments(PARSE_ARGV 4 FCH "${options}" "" "${mvargs}")

    FetchContent_Declare(
        ${name}
        ${vc}_REPOSITORY ${url}
        ${vc}_TAG        ${tag}
        GIT_PROGRESS     ON
    )
    FetchContent_GetProperties(${name})
    if(NOT ${name}_POPULATED)
        message("Setting up ${name} from ${url}")
        FetchContent_Populate(${name})
        if(FCH_ADD_SUBDIR)
            foreach(config ${FCH_CONFIG_SUBDIR})
                string(REPLACE "=" ";" configkeyval ${config})
                list(LENGTH configkeyval len)
                if (len GREATER_EQUAL 2)
                    list(GET configkeyval 0 configkey)
                    list(SUBLIST configkeyval 1 -1 configvals)
                    string(REPLACE ";" "=" configval "${configvals}")
                else()
                    message(FATAL_ERROR "Invalid config: ${configkeyval}")
                endif()
                message("Set ${configkey} = ${configval}")
                set(${configkey} ${configval} CACHE INTERNAL "" FORCE)
            endforeach()
            set(${name}_SOURCE_DIR ${${name}_SOURCE_DIR} PARENT_SCOPE)
            set(${name}_BINARY_DIR ${${name}_BINARY_DIR} PARENT_SCOPE)
            add_subdirectory(${${name}_SOURCE_DIR} ${${name}_BINARY_DIR} EXCLUDE_FROM_ALL)
        endif()
    endif()
endfunction(FetchContentHelper)
FetchContentHelper(spdlog GIT "https://github.com/gabime/spdlog.git" v1.x ADD_SUBDIR)
FetchContentHelper(fmt GIT "https://github.com/fmtlib/fmt.git" 5.2.1 ADD_SUBDIR)
FetchContentHelper(eigen HG "https://bitbucket.org/eigen/eigen" default
    ADD_SUBDIR CONFIG_SUBDIR
        BUILD_TESTING=OFF
        )
# !1
set(Eigen3_DIR ${eigen_BINARY_DIR})
find_package(Eigen3 REQUIRED NO_MODULE NO_DEFAULT_PATH)
# get_target_property(eigen_include_dir Eigen3::Eigen INTERFACE_INCLUDE_DIRECTORIES)  # !2
FetchContentHelper(ceres GIT "https://github.com/ceres-solver/ceres-solver.git" master
    ADD_SUBDIR CONFIG_SUBDIR
      EIGEN_INCLUDE_DIR=${eigen_SOURCE_DIR}  # !3
      EIGENSPARSE=ON
      SUITESPARSE=ON
      CXSPARSE=OFF
      GFLAGS=OFF
      MINIGLOG=ON
      CXX11=ON
      OPENMP=ON
      EXPORT_BUILD_DIR=ON
    )
set(Ceres_DIR ${ceres_BINARY_DIR})
find_package(Ceres REQUIRED NO_DEFAULT_PATH)  # !4

In general, this works, but there are places that do not work and/or confusing to me.

  • !1: This is to import Eigen3::Eigen target.
  • !2: This attempts to read the include dir property of Eigen3::Eigen, which I though I could use as EIGEN_INCLUDE_DIR in configuring ceres, but it turn out that the property from get_property is a generator expression, so it causes an error in the ceres cmake file.
  • !3: Since !2 does not work, I use ${eigen_SOURCE_DIR} (populated by the fetchcontent_polulate command) to instruct the cere cmake file to find eigen, and it does work. However, this breaks when exporting the ceres target, due to the fact that this EIGEN_INCLUDE_DIR is in the build tree and cannot be set as one of the directories in INTERFACE_INCLUDE_DIR property. I can work around the problem by making the following change in ceres-src/internal/ceres/CMakeLists.txt:
# target_include_directories(ceres SYSTEM PUBLIC ${EIGEN_INCLUDE_DIRS})
target_include_directories(ceres SYSTEM PUBLIC
    $<BUILD_INTERFACE:${EIGEN_INCLUDE_DIRS}>
  $<INSTALL_INTERFACE:include>)
  • !4: Finally, I can import the ceres target, but not sure if this is the right way of doing it.

Any help is greatly appreciated. Thank you!

@alexsmac
Copy link
Contributor

You do not need to use EIGEN_INCLUDE_DIR to specify the location of Eigen for Ceres, setting Eigen3_DIR will have the desired effect, Ceres' FindEigen.cmake script prefers an exported package if it is available (controlled via: EIGEN_PREFER_EXPORTED_EIGEN_CMAKE_CONFIGURATION) - fixing that should resolve your other issues.

@Jerry-Ma
Copy link
Author

I read through the FindEigen.cmake module in ceres, and was able to found a solution as follows:

FetchContentHelper(eigen HG "https://bitbucket.org/eigen/eigen" default
    ADD_SUBDIR CONFIG_SUBDIR
        BUILD_TESTING=OFF
        )
FetchContentHelper(ceres GIT "https://github.com/ceres-solver/ceres-solver.git" master
    ADD_SUBDIR CONFIG_SUBDIR
        EIGEN_PREFER_EXPORTED_EIGEN_CMAKE_CONFIGURATION=OFF
        EIGEN_INCLUDE_DIR_HINTS=${eigen_SOURCE_DIR}
        EIGENSPARSE=ON
        SUITESPARSE=ON
        CXSPARSE=OFF
        GFLAGS=OFF
        MINIGLOG=ON
        CXX11=ON
        OPENMP=ON
        TBB=OFF
        CXX11_THREADS=OFF
        EXPORT_BUILD_DIR=ON
    )

The thing here is that for !1 and !4, I do not need them because when including as subdir, the eigen and ceres CMakeLists.txt will create correct targets already, i.e., Eigen3::Eigen and ceres, no needing of the extra find_package call. The key to the success is that I have to set EIGEN_PREFER_EXPORTED_EIGEN_CMAKE_CONFIGURATION=OFF to skip the check of system Eigen, then the flow will enter into the block of line 180 find_path call where EIGEN_INCLUDE_DIR_HINTS is used, which I set to the fetchcontent source dir.

Setting Eigen3_DIR as you suggested (while setting EIGEN_PREFER_EXPORTED_EIGEN_CMAKE_CONFIGURATION=OFF), instead of setting EIGEN_INCLUDE_DIR_HINTS, does not work here, because the find_path call at line 180 does not honor that variable, instead it tries to find the header files from EIGEN_CHECK_INCLUDE_DIRS.

To sum up, I've found the solution to the question that I raised this issue for, which is to fetchcontent eigen and ceres, and build ceres using the fetched eigen build. In the mean time, I believe the FindEigen.cmake module could be better tweaked to allow this to be achieved more intuitively.

One possible change would be adding some logic involves Eigen3_DIR or exported target (e.g., if (TARGET Eigen3::Eigen) before the find_path call.

@alexsmac
Copy link
Contributor

You should not need to do anything beyond setting Eigen3_DIR when configuring Ceres. The find_path() call on line 180 is only relevant if Eigen is not found as a package. You are explicitly creating it as a package. There should not be any issues with a conflicting system package of Eigen provided that you have passed Eigen3_DIR pointing at the local configured Eigen directory.

Your mistake appears to be that you are assuming that you do not need to set Eigen3_DIR. The targets will be present if you use add_subdirectory(), but as per the docs of FetchContent you are not expecting to modify the fetched projects which means they will still invoke find_package() calls, which you will need to correctly configure the variables for in order that they detect the versions that you want.

@Jerry-Ma
Copy link
Author

Upon closer investigate, I found that the reason that Eigen3_DIR did not work for me is due to a bug from Eigen: http://eigen.tuxfamily.org/bz/show_bug.cgi?id=1386 .

In short, I am setting up the Eigen package via FetchContent, which basically first clones the eigen source code to build/_deps/eigen-src, then add_subdirectory(eigen-src) with the builddir set to build/_deps/eigen-build. Upon finishing, the Eigen3Config.cmake.in in eigen-src is configured to Eigen3Config.cmake in eigen-build.

However, as described in the bug report above, the configured Eigen3Config.cmake assumes that this file is to be used for installed Eigen3, not the exported Eigen3 build tree. Setting Eigen3_DIR to build/_deps/eigen-build does instruct ceres correctly locate this Eigen3Config.cmake file, but the EIGEN_INCLUDE_DIR is set to a wrong value, which does not point to the correct directory, in this case, build/_deps/eigen-src, instead, it points to ${CURRENT_LIST_DIR}../../../ which would be correct if the Eigen is installed to some prefix, but is actually wrong in my case since it is just a build tree.

In the bug report, it points to a pull request that suppose to fix this issue, but the pull request is not merged for some reason yet. Although I haven't check if it does fix my problem.

I am not sure about the exact meaning of your second point, but I agree with you that, if the XXXConfig.cmake file is correctly written to handle both case of exported build tree and installed package, then setting XXX_DIR and use find_package(XXX NO_MODULE NO_DEFAULT_PATH) would be the safest bet.

Back to this issue, I think for now, I am OK with my current solution (or hack) of bypassing find_package(Eigen) in ceres CMakeLists.txt by setting EIGEN_INCLUDE_DIR_HINT. After Eigen could fix this issue from their side, I can switch to setting Eigen3_DIR and using the find_package mechanism in ceres.

@Jerry-Ma
Copy link
Author

Forgot to mention, for the manual solution to work, I still need to make the change to ceres-src/internal/ceres/CMakeLists.txt .

- target_include_directories(ceres SYSTEM PUBLIC ${EIGEN_INCLUDE_DIRS})
+target_include_directories(ceres SYSTEM PUBLIC
+   $<BUILD_INTERFACE:${EIGEN_INCLUDE_DIRS}>
+  $<INSTALL_INTERFACE:include>)

I think this change is not bounded to the find_package stuff we discussed, but rather a generic treatment of both exported build tree and installed package, just the same as what is done to minglog.
I'd be happy if you can fix this bit unless there is other side effects that I do not know of. Thank you.

@Jerry-Ma
Copy link
Author

Jerry-Ma commented Jan 31, 2019

OK, after some more fiddling, I guess the thing to fix here in ceres is that it uses the legacy EIGEN_FOUND and EIGEN_INCLUDE_DIR machinery even when find_package(Eigen3) runs correctly and provide the imported target Eigen3::Eigen.

In fact, if you look into Eigen source code: https://bitbucket.org/eigen/eigen/src/24d29254152d3b0818a05073f456898b39963fa0/cmake/Eigen3Config.cmake.in?at=default&fileviewer=file-view-default It is advertised that those variables should not be used.

If using the Eigen3::Eigen target, we can completely remove that target_include_directories block, but instead use target_link_libraries(Eigen3::Eigen).

Any possibility that this got fixed soon?

@sandwichmaker sandwichmaker added this to the 2.0 milestone Mar 3, 2019
@sandwichmaker sandwichmaker added the cmake CMake build related issues label Mar 28, 2019
@sandwichmaker
Copy link
Contributor

@alexsmac ?

@Ram-Z
Copy link

Ram-Z commented Dec 13, 2019

This has come quite a long way now since 33dd469 has been merged. Thanks @NeroBurner.

With a preinstalled version Eigen, this should now work. The last hitch is when you try to use FetchContent with both Eigen and ceres-solver. Both define an uninstall target and CMake doesn't like that.

CMake Error at build-Linux-x86_64-Debug/_deps/ceres-src/CMakeLists.txt:769 (add_custom_target):
  add_custom_target cannot create target "uninstall" because another target
  with the same name already exists.  The existing target is a custom target
  created in source directory
  "build-Linux-x86_64-Debug/_deps/eigen-src".  See
  documentation for policy CMP0002 for more details.

This minimal CMakeLists.txt should be able to reproduce the issue.

cmake_minimum_required(VERSION 3.5)
project(FetchCeres)
include(FetchContent)
FetchContent_Declare(eigen
  GIT_REPOSITORY "https://github.com/eigenteam/eigen-git-mirror.git"
  GIT_TAG cf794d3b741a6278df169e58461f8529f43bce5d  # tag: 3.3.7
)
FetchContent_MakeAvailable(eigen)

FetchContent_Declare(ceres
  URL           https://github.com/ceres-solver/ceres-solver/archive/edb8322bdabef336db290be1cc557145b6d4bf80.tar.gz
  URL_HASH      SHA256=94de33593ec654481f29e74691373229f72e538ae69794aece2c6875678fbbc4
)
set(Eigen3_DIR ${eigen_BINARY_DIR})
# if ceres would set CMP0077 to NEW this could be a simple set(MINIGLOG ON) and not be overridden
set(MINIGLOG ON CACHE INTERNAL "" FORCE)  
FetchContent_MakeAvailable(ceres)

The above snippet also brings up the following error, but I think this needs to be addressed in Eigen.

CMake Error at build/_deps/eigen-build/Eigen3Config.cmake:20 (include):
  The file

    /tmp/tmp.RyOIZUuoJR/build/_deps/eigen-build/Eigen3Targets.cmake

  was generated by the export() command.  It may not be used as the argument
  to the include() command.  Use ALIAS targets instead to refer to targets by
  alternative names.

Call Stack (most recent call first):
  build/_deps/ceres-src/CMakeLists.txt:242 (find_package)

@NeroBurner
Copy link
Contributor

NeroBurner commented Dec 14, 2019

Some shameless self promotion: using Hunter as package manager one just needs CMake and a compiler to create a project using ceres-solver. I've created a (nearly) minimal example to show how to add ceres-solver (even with SuiteSparse) with Hunter

Edit: link to minimal example project: https://github.com/NeroBurner/hunter_ceres_example

@Ram-Z
Copy link

Ram-Z commented Dec 16, 2019

That example requires Eigen to be found in the system though doesn't? Have you tried getting Eigen with Hunter too?

@NeroBurner
Copy link
Contributor

Eigen is also provided by Hunter. Hunter uses a modified ceres-solver project. Currently on the hunter-1.14.0 branch is used by Hunter.

There all dependencies are required from Hunter, see this commit for the actual Eigen package request

Some of the changes (Eigen3Config, gflagsConfig, explicit PRIVATE/PUBLIC linking) are already upstream, so less modifications for me to keep track of starting with ceres-solver v2.0.0 :)

@Ram-Z
Copy link

Ram-Z commented Dec 16, 2019

Ah... I see. I guess that works because Hunter will build and "install" Eigen and forget about any of the target it created. Whereas FetchContent is still aware of any targets Eigen created.

NeroBurner added a commit to NeroBurner/ceres-solver that referenced this issue Dec 16, 2019
Ceres-solver provides a custom uninstall target to ease the removal of
installed files from the system. This clashes with other projects, that
too provide an uninstall target (like Eigen3).

Related issue: ceres-solver#451 (comment)

Change-Id: Id153830ae20a880d23c7468acb39f55f48a2129a
@NeroBurner
Copy link
Contributor

@Ram-Z Could you try if this commit would fix the last roadblock for FetchContent?
https://github.com/NeroBurner/ceres-solver/tree/optional_uninstall_target

just set the cmake-arg PROVIDE_UNINSTALL_TARGET=NO to fetch-content of ceres-solver

@Ram-Z
Copy link

Ram-Z commented Dec 16, 2019

This does indeed fix the duplicate target name issue. Had to add the following patch to Eigen to fix the file was generated by the export() command. It may not be used as the argument to the include() command..

diff --git a/cmake/Eigen3Config.cmake.in b/cmake/Eigen3Config.cmake.in
index c5c546887..a79b31ce5 100644
--- a/cmake/Eigen3Config.cmake.in
+++ b/cmake/Eigen3Config.cmake.in
@@ -3,6 +3,10 @@
 
 @PACKAGE_INIT@
 
+if(Eigen3_FOUND OR EIGEN3_FOUND)
+  return()
+endif()
+
 include ("${CMAKE_CURRENT_LIST_DIR}/Eigen3Targets.cmake")
 
 # Legacy variables, do *not* use. May be removed in the future.

Then I also had to add_library(Eigen3::Eigen ALIAS eigen) because that alias is not provided upstream.

But with these changes I managed to use FetchContent on both Eigen and ceres successfully.

I'm not entirely sure what the best practices for CMakeConfig files are. Should they return early if the target they are providing has already been found?

@NeroBurner
Copy link
Contributor

I think both changes would be valuable contributions to upstream Eigen3.

The Eigen team recently moved to gitlab: https://gitlab.com/libeigen/eigen

@Ram-Z
Copy link

Ram-Z commented Dec 16, 2019 via email

@Ram-Z
Copy link

Ram-Z commented Dec 16, 2019

Already patched upstream, just not released yet.

cmake_minimum_required(VERSION 3.5)
project(FetchCeres)
include(FetchContent)

file(DOWNLOAD
  https://gitlab.com/libeigen/eigen/commit/2cbd9dd49806d686a40841b6d888a83c816efccf.patch
  ${CMAKE_CURRENT_BINARY_DIR}/support-include.patch
)
FetchContent_Declare(eigen
  GIT_REPOSITORY https://gitlab.com/libeigen/eigen
  GIT_TAG        21ae2afd4edaa1b69782c67a54182d34efe43f9c  # tag: 3.3.7
  PATCH_COMMAND  patch -p1 -i ${CMAKE_CURRENT_BINARY_DIR}/support-include.patch
)
FetchContent_MakeAvailable(eigen)

file(DOWNLOAD
  https://github.com/NeroBurner/ceres-solver/commit/8bcf51f1e00e36293aa4f7fb8eac8ffbb0933457.patch
  ${CMAKE_CURRENT_BINARY_DIR}/no-uninstall.patch
)

FetchContent_Declare(ceres
  URL           https://github.com/ceres-solver/ceres-solver/archive/edb8322bdabef336db290be1cc557145b6d4bf80.tar.gz
  URL_HASH      SHA256=94de33593ec654481f29e74691373229f72e538ae69794aece2c6875678fbbc4
  PATCH_COMMAND patch -p1 -i ${CMAKE_CURRENT_BINARY_DIR}/no-uninstall.patch
)

# if ceres would set CMP0077 to NEW this could be a simple set(MINIGLOG ON) and not be overridden
set(PROVIDE_UNINSTALL_TARGET OFF CACHE INTERNAL "" FORCE)
set(MINIGLOG                 ON  CACHE INTERNAL "" FORCE)
FetchContent_MakeAvailable(ceres)

This works now. Thanks.

keir pushed a commit that referenced this issue Dec 16, 2019
Ceres-solver provides a custom uninstall target to ease the removal of
installed files from the system. This clashes with other projects, that
too provide an uninstall target (like Eigen3).

Related issue: #451 (comment)

Change-Id: Id153830ae20a880d23c7468acb39f55f48a2129a
@Ram-Z
Copy link

Ram-Z commented Jan 8, 2020

Finally got back to this again and with the patch to ceres below, I'm happy that ceres can be integrated well using FetchContent.

--- CMakeLists.txt	2020-01-08 18:25:03.380697208 +0000
+++ CMakeLists.txt.new	2020-01-08 18:24:56.050634096 +0000
@@ -31,6 +31,10 @@
 
 cmake_minimum_required(VERSION 3.5)
 cmake_policy(VERSION 3.5)
+if(POLICY CMP0077)
+  # do not overwrite normal variables with option()
+  cmake_policy(SET CMP0077 NEW)
+endif()
 
 # Set the C++ version (must be >= C++11) when compiling Ceres.
 #

The CMP0077 policy allows option() calls to not override normal variables that are already set.

cmake_minimum_required(VERSION 3.5)
project(FetchCeres)
include(FetchContent)

FetchContent_Declare(eigen
  GIT_REPOSITORY https://gitlab.com/libeigen/eigen
  GIT_TAG        9623c0c4b9f40e74ba6ba551f86831616bdd488c
)
FetchContent_MakeAvailable(eigen)

FetchContent_Declare(ceres
  URL           https://github.com/ceres-solver/ceres-solver/archive/a60136b7aa2c1ab97558a74b271eee16ca1365c4.tar.gz
  URL_HASH      SHA256=94de33593ec654481f29e74691373229f72e538ae69794aece2c6875678fbbc4
  # patch from above
  PATCH_COMMAND patch -p0 -i ${CMAKE_CURRENT_BINARY_DIR}/ceres-cmp0077.patch 
)

set(PROVIDE_UNINSTALL_TARGET OFF)
set(MINIGLOG ON)
FetchContent_MakeAvailable(ceres)

@Ram-Z
Copy link

Ram-Z commented Jan 8, 2020

I guess it's worth mentionning that instead of setting CMP0077 NEW, you could also increase cmake_mininum_required(VERSION 3.13) but I assume that would be less acceptable.

@Ram-Z
Copy link

Ram-Z commented Jan 10, 2020

I've figured out that you can set(CMAKE_POLICY_DEFAULT_CMP0077 NEW) and that would enforce the NEW policy in sub directories.

So no changes are required anymore in Eigen or Ceres.

cmake_minimum_required(VERSION 3.5)
project(FetchCeres)
include(FetchContent)

FetchContent_Declare(eigen
  GIT_REPOSITORY https://gitlab.com/libeigen/eigen
  GIT_TAG        9623c0c4b9f40e74ba6ba551f86831616bdd488c
)
FetchContent_MakeAvailable(eigen)

FetchContent_Declare(ceres
  URL           https://github.com/ceres-solver/ceres-solver/archive/a60136b7aa2c1ab97558a74b271eee16ca1365c4.tar.gz
  URL_HASH      SHA256=94de33593ec654481f29e74691373229f72e538ae69794aece2c6875678fbbc4
)

set(CMAKE_POLICY_DEFAULT_CMP0077 NEW)
set(PROVIDE_UNINSTALL_TARGET OFF)
set(MINIGLOG ON)
FetchContent_MakeAvailable(ceres)

I'm not OP, but in my opinion this can now be closed. Thank you all very much.

@sandwichmaker
Copy link
Contributor

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmake CMake build related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants