Don't Force set BUILD_TESTING Variable

[Leon0402]

Hi,

I noticed you force overwrite the default CMake BUILD_TESTING Variable here: https://github.com/whoshuu/cpr/blob/master/CMakeLists.txt#L156

This breaks projects depending on the lib. If I want to run my tests I need to force overwrite BUILD_TESTING as well now after I included your lib (with FetchContent).

Cache Variables are usually not supposed to be set in a CMakeListst.txt, but provided via command line. So you would run cmake --build build -DBUILD_TESTING=OFF Only need that once, becauses it's cached (at least until you delete the build folder).

If you don't like specifing the variable every time (as it defaults to true), use CMakePresets to define a shortcut for it. Although it is very unusual for a library to set the variable by default to OFF. Why wouldn't you want to build your tests? And personally I also find it more convenient to always set this via the command line, so you don't run into issues with git and furthermore it's easier to change in say CI Pipelines

If you really want to set this to OFF without specifying it via the command line, you can set a local variable with the same name. So just do

set(BUILD_TESTING OFF) 

This works, because it's possible in CMake to have both local and cache variables with the same name and local variables take presedence over cache variables. But the important difference is that local variables are scoped! So if you do it like this, BUILD_TESTING is just off for your library and not my project using your library.

Thanks! :)

[COM8]

Aha! Yes, you are right. The reason we are setting BUILD_TESTING here to OFF is to prevent curl from building its test cases. @Leon0402 would you like to create a PR for this some time soon since then I could include this in the 1.6.1 release I'M preparing right now and wanted to release tomorrow.

[COM8]

CPR itself uses the CPR_BUILD_TESTS variable to determine if we should build tests.

[Leon0402]

Sure :) https://github.com/whoshuu/cpr/pull/561

[Leon0402]

Just a small update: My PR unfortunately didn't fix it. The reason for this is curl or some bug in Cmake. Or a combination of both :-)

Just setting normal variables usually works for the reasons mentioned (local scope, preference over cache variables). But curl has such a line in it's cmake: cmake_dependent_option(BUILD_TESTING ...) Which is no problem afaik as this should be more or less the same as setting a cache variable, but without FORCE, which ist then just ignored.

But setting: both

set(BUILD_TESTING OFF)
cmake_dependent_option(BUILD_TESTING "" "" "" "")

BUILD_TESTING is somehow OFF everywhere, including the parent scope (where it's not set). I haven't figured out yet if there is some logical explanation for this, but it seems this could be a cmake bug.

Will post an update here f I found out anything new.

[Leon0402]

The issue was confirmed by cmake developers here: https://gitlab.kitware.com/cmake/cmake/-/issues/22909

It seems an fix wouldn't be too difficult, but I currently lack time and knowledge to fix it myself, so I need to wait until someone will fix it. Maybe upvoting it will help.

Currently I use the following workaround:

set(BUILD_TESTING_BEFORE ${BUILD_TESTING})

# FetchContentMakeAvailable(cpr) ... 

set(BUILD_TESTING ${BUILD_TESTING_BEFORE} CACHE INTERNAL "" FORCE)

This will reset BUILD_TESTING to its previous value.

Maybe a note should be added to the README or this issue pinned, to inform about this known bug.

[COM8]

Ping @KingKili ?

[COM8]

Any updates on that front?

[Leon0402]

Not from my side. The issue https://gitlab.kitware.com/cmake/cmake/-/issues/22909 is still open. It sounds easy to fix, but it doesn't seem like someone has the time to do it right now.