ceres-solver icon indicating copy to clipboard operation
ceres-solver copied to clipboard

Modernize the way we use googletest.

Open sandwichmaker opened this issue 2 years ago • 9 comments

Currently we package googletest and gmock using a now obsolete method which generated single headers from the repo. This is not supported anymore. The modern instructions for using googletest are described at

https://google.github.io/googletest/quickstart-cmake.html

sandwichmaker avatar Aug 17 '22 18:08 sandwichmaker

One thing which is not clear is if we should be fetching inside the cmake file, or using googletest as a submodule the same way abseil recommends that it be used.

https://abseil.io/docs/cpp/quickstart-cmake

sandwichmaker avatar Aug 17 '22 18:08 sandwichmaker

@sergiud wdyt?

sandwichmaker avatar Aug 17 '22 18:08 sandwichmaker

I'm all for it. I'd rather integrate googletest through find_package such that distribution specific packages can be used without hassle. This is also consistent to how every other dependency is included in Ceres.

sergiud avatar Aug 17 '22 18:08 sergiud

googletest is not supposed to be used as a library, they explicitly tell you not to do it, but rather have a copy of the source as part of your own project. So find_package is not the right approach here IMO.

sandwichmaker avatar Aug 17 '22 18:08 sandwichmaker

My question is, should we be including googletest as a git submodule or pull it using cmake's FetchContent.

include(FetchContent)
FetchContent_Declare(
  googletest
  GIT_REPOSITORY https://github.com/google/googletest.git
  GIT_TAG release-1.12.1
)
# For Windows: Prevent overriding the parent project's compiler/linker settings
set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
FetchContent_MakeAvailable(googletest)

sandwichmaker avatar Aug 17 '22 18:08 sandwichmaker

Whilst it's true that they advise against this, it's also true that an awful lot of people do exactly this, and they even provide an option: INSTALL_GTEST in their own CMake to support this. Hence why there are googletest packages in Ubuntu, Homebrew et al. Their concerns around ensuring the compiler flags are identical are not wrong, but they are also an unlikely failure mode for a lot of users in practice for whom a package is more appropriate/convenient.

There are a number of use-cases where FetchContent is a particular pain when you need to internally (i.e. privately) manage and audit/track all dependencies, e.g. using a tool such as Conan. Whilst FetchContent directives such as the above may 'work' they backdoor whether a uniform version of googletest (say) was used across all dependencies that may depend on it across a larger project. For this reason, IMO I prefer using the packages of it that are already available.

alexsmac avatar Aug 17 '22 18:08 alexsmac

Thanks @alexsmac I am not terribly well informed about the world outside google, so its good for me to learn the considerations here. I think the ODR violation concern for googletest -- since they are primarily used for test binaries inside ceres solver are not really a problem but they are an issue for something like abseil where other libraries may be using different compiler options/defines.

sandwichmaker avatar Aug 17 '22 19:08 sandwichmaker

So for googletest fetchcontent/find_package/including the source in ceres are all mostly non-issues IMO. So I am good with using find_package. Though googletest is going to take on abseil as a dependency soonish, and then if we are using find_package for that, then I suppose our hand will be forced and we will be using find_package for abseil also? The problem we will have then is that given abseil's pace of development we would have to tie ourselves to a particular version of abseil which may make its use impractical.

sandwichmaker avatar Aug 17 '22 19:08 sandwichmaker

cc: @keir WDYT is the right way to depend on googletest and abseil going forward?

sandwichmaker avatar Aug 17 '22 19:08 sandwichmaker