cmake: Controlling Abseil's tests with BUILD_TESTING is inconvenient

[matta]

Describe the bug

Abseil arranges for its own tests to be built and run whenever BUILD_TESTING is set, and then Abseil turns it off by default. This has been the case since the fix for #901.

If an enclosing project enables testing and then incorporates Abseil as a subproject, Abseil's tests will be built and configured to run. There is no convenient way for an enclosing project to both turn on testing but also disable Abseil's tests.

Steps to reproduce the bug

1. Incorporate Abseil into another CMake project. In my case, I'm doing it this way in the root CMakeLists.txt of my project:

FetchContent_Declare(
  googletest
  GIT_REPOSITORY https://github.com/google/googletest.git
  # We YOLO it and fetch from head
  # GIT_TAG        703bd9caab50b139428cea1aaff9974ebee5742e # release-1.10.0
)
FetchContent_MakeAvailable(googletest)

set(ABSL_PROPAGATE_CXX_STD ON)
set(ABSL_FIND_GOOGLETEST OFF)
set(ABSL_USE_EXTERNAL_GOOGLETEST ON)
FetchContent_Declare(
  abseil
  GIT_REPOSITORY https://github.com/abseil/abseil-cpp.git
  # We YOLO it and fetch from head
  # GIT_TAG        703bd9caab50b139428cea1aaff9974ebee5742e # release-1.10.0
)
FetchContent_MakeAvailable(abseil)

2. configure my project as usual: cmake -S . -B _builds
3. cd _builds
4. make tests

Expected: my project's tests are run
Actual: both my project's tests and Abseil's tests are run

What version of Abseil are you using?

https://youtu.be/tISy7EJQPzI

What operating system and version are you using

n/a

What compiler and version are you using?

n/a

What build system are you using?

cmake version 3.21.4

Additional context

Other projects (grpc, googletest, etc.) expose an option named like `${PROJECT_NAME}_BUILD_TESTING that allows the project's tests to be turned on. They typically default off. That's what I'd suggest here.

[derekmauro]

Hey Matt! The last time someone brought this up there was a long discussion about this that was really never resolved. @coryan had some opinions on sticking with BUILD_TESTING. @haberman was in favor of introducing a separate flag.

[matta]

I'm curious to learn if there is something I'm missing on the technical level here.

From https://discourse.cmake.org/t/how-to-use-fetchcontent-correctly-for-building-external-dependencies/3686, Craig Scott, one of the cmake maintainers, says:

Dealing with tests is harder. [...] The dependency’s tests will still show up and be included in the main projects tests, but that is hard to avoid without the dependency providing some sort of switch to turn off adding tests. A well-structured project should ideally provide a project-specific CMake variable that can be passed down to turn tests on or off, and it should default to true if the dependency is being built on its own and false if it is not the top level project.

[derekmauro]

There was a doc written that says "Prefer using standard CMake flags" but doesn't give any rationale. If I recall correctly, when asked, the author of the doc said that not using BUILD_TESTING would be surprising, but didn't explain why.

It seems like we have examples of projects using a project specific flag, so it can't be that surprising.

Unless I hear otherwise, I'm inclined to accept #1057.

[matta]

I can see the appeal of using BUILD_TESTING directly when Abseil is built as a top level project.

Derek, I'm asking about this at https://discourse.cmake.org/t/fetchcontent-vs-build-testing/4477 so you might wait for a resolution there before accepting my PR.

[derekmauro]

Hi Matt,

Did anything change your mind on this one?

An example of using project structure (as suggested in one of the comments in your cmake.org question) can be found here:
https://github.com/google/or-tools/blob/b37d9c786b69128f3505f15beca09e89bf078a89/cmake/dependencies/CMakeLists.txt#L36-L41

I think I'm still inclined to accept your PR though.

[matta]

Hi Derek, thanks for following up.

I still think this PR is an improvement and could be merged as-is, but I think the PR could be better, and I'll explain why below. As I've switched back to bazel, and am not doing anything serious with Abseil anyway, I'm not personally motivated to work on this any longer.

One reason I think this PR is an improvement as-is because I think not providing a project specific variable to turn tests on or off is inconvenient. Unlike bazel, there is no consistently applied convention for running tests in "your" project vs those of your dependencies. Where bazel forces a path prefix based convention, CMake projects aren't always set up well to conveniently build a subset of tests. The way to do it often relies on certain naming conventions for test targets, which I found were not consistent across projects, and nothing concrete about this is suggested in CMake documentation. So providing a way to easily configure tests on/off for individual project dependencies might be convenient. Imagine having 10 dependent projects and you normally want to run tests for none of them. But you suspect a bug in Abseil. If all 10 used BUILD_TESTING as their single point of config, there would be no way to turn just Abseil's tests on (apart from the cumbersome approach I'll describe below).

The example you gave from the the https://github.com/google/or-tools project requires a particular project structure. It requires having a subdirectory for all dependencies and in the dependencies/CMakeLists.txt having something like this before fetching the dependencies:

include(FetchContent)
set(BUILD_TESTING OFF)

Then to use it you must call add_subdirectory from a CMakeLists.txt in a parent directory:

add_subdirectory(dependencies)

I see or-tools doing this here: https://github.com/google/or-tools/blob/9e820b180402c051caef5e25eb250d9da587e40b/CMakeLists.txt#L216

This is consistent with what I found looking at this problem. The only ways to have a scope in CMake are functions and separate CMakeLists.txt files, so the convenient way to turn BUILD_TESTING off for a subset of dependencies is to put them in a subdirectory like this. There is no convenient way to set a variable/option for just a portion of a CMakeLists.txt file. I think there were some hacky approaches I found on the forums that involved setting and then re-setting options, but I saw them all as "fighting" the way CMake wants things to be structured.

This, of course, has the drawback of forcing that kind of directory structure on the user. One can no longer conveniently have a simple project that uses a single CMakeLists.txt and turns Abseil tests off while keeping project tests on. I think it is a non-obvious and unfortunate restriction, especially for a non-expert CMake user (i.e. me).

In #1056 (comment) above I quoted some advice from an active CMake maintainer:

A well-structured project should ideally provide a project-specific CMake variable that can be passed down to turn tests on or off, and it should default to true if the dependency is being built on its own and false if it is not the top level project.

A member on the CMake form suggested GLFW as an example of this best practice (https://discourse.cmake.org/t/fetchcontent-vs-build-testing/4477/5). In GLFW it is done like this:

https://github.com/glfw/glfw/blob/53d86c64d709ff52886580d338d9b3b2b1f27266/CMakeLists.txt#L21-L27

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
    set(GLFW_STANDALONE TRUE)
endif()

option(GLFW_BUILD_TESTS "Build the GLFW test programs" ${GLFW_STANDALONE})

So GLFW_STANDALONE is set TRUE only when its own CMakeLists.txt is the root one, and the GLFW_BUILD_TEST option defaults to TRUE only when that is the case. Note that GLFW_BUILD_TESTS is an option, so it can be set differently prior to this file being processed.

You could get fancier and default GLFW_BUILD_EXAMPLES to the current value of BUILD_TESTS if standalone and always default it false otherwise, or some such, but this is probably overkill. I suspect the above scheme is the right default for most cases.

So, this approach seen in GLFW seems to be best practice. This PR moves things in that direction but doesn't quite get there.