[bug] tools.build:skip_test and shared option behavior inconsistent

[aagor]

Describe the bug

Environment details: Linux, Conan 2.12.1

When testing conans tools.build:skip_test config option, I encountered that conans skip_test and the generated CMake cache variable BUILD_TESTING are diverting.

Preparation:

$ git clone https://github.com/conan-io/examples2.git
$ git clone https://github.com/conan-io/libhello.git
$ cd libhello
$ git switch with_tests
$ cp ../examples2/tutorial/creating_packages/build_method/conanfile.py .
$ conan build . --build=missing

Consider the following after the preparation:

• Modify tests/test.cpp.
  Execute conan build . -c tools.build:skip_test=True.
  No tests are build, no tests executed. Expected behavior.

• Modify tests/test.cpp again.
  Execute conan build ..
  No tests are build, old tests are executed. Not expected and inconsistent.

• Modify tests/test.cpp again.
  Execute conan build . -c tools.build:skip_test=False.
  No tests are build (not expected), old tests are executed (test execution itself is fine).

At least for the third case, when I explicitly set skip_test to false, I would expect the tests to build and the CMake cache variable BUILD_TESTING to be overriden to ON. As conan doesn't have something similar to CMake cache variables, I think it would be more consistent if BUILD_TESTING always follows conans tools.build:skip_test config option.

The exact same problem exists with conan packages shared option and BUILD_SHARED_LIBS CMake cache variable (and possibly others).

Is this intended? Is there a workaround?

How to reproduce it

No response

[memsharded]

Hi @aagor

Thanks for your feedback.

I think this is related to #12995 and this issue #12994 (could be a duplicated of the issue) that goes back to #7832.

In summary, the issue is that the toolchain/presets defines its values as cache variables, because there are too many existing situations with existing CMakeLists.txt defining those things as cache variables, and then the only way for things to work are defining them as cache variables. A explicit fix and a test capturing this behavior was added in #8124 as a response to #7832.

The downside is that when some configurations change in Conan and using the "local flow", that is the conan build command, or conan install + cmake commands, then it is necessary to clean the CMake cache. This cleaning cannot be done by default either, as that would remove the main advantage and use case of the "local flow" which is incremental builds.

So it is not possible to have everything, and the current proposal is the one that tries to minimize inconvenience:

• Incremental builds cannot be removed, that would be a disaster
• Issues because of the toolchain not correctly defining things because those are declared as cache variables slip into the package creation too, so it can have very harmful effects, with very difficult to debug issues in productions
• Some configurations changes in the "local flow" might work without a cache cleaning.
• So it leaves the necessity to force the cleaning of the cmake cache to the user to a few scenarios like changing some config like testing-enabling/shared-static, etc, that would require mostly the same cache cleaning even if not using Conan.

[aagor]

Hi, thanks for your explanation.

I think the problematic behavior may have been mitigated with CMake Policy CMP0126.

Nevertheless I don't understand why the FORCE option of set isn't used? Would that likely break something?

[memsharded]

Yes, but CMake Policy CMP0126 is CMake>3.21, and the basic behavior keeps working with CMake 3.15

Nevertheless I don't understand why the FORCE option of set isn't used? Would that likely break something?

I think that disables the possibility for users to dynamically define things. Consider that the most popular approach is to do conan install + cmake ..., not conan build, because calling cmake is the most native to developers. Users sometimes want to change values in command line with cmake -DVARIABLE=VALUE, and as commented above, this not always require a cache clean/regenerate, so it can be a good and natural flow for them.

[aagor]

I understand that workflow and it's true that changing things via CMake CLI wouldn't work if FORCE gets simply added (as this command gets written into the generated toolchain and is always executed).
For me it seems like it would be beneficial if the CMake variables are forced during the generate() method of the conanfile.py.

This way the variables would be set to the ones defined by conan on the first invocation and when using conan install or conan build again, and still allows to override values by the user when using only CMake after an initial conan install/build.

For the shared option, I agree it's probably not a use case to change that often (especially if using the same build directory, incremental builds would most likely not work anyway)

But for BUILD_TESTING it's different. It makes sense to switch that off temporarily (want to check that main code compiles before adapting tests, skip test compilation and execution for performance reasons, ...)

[memsharded]

This way the variables would be set to the ones defined by conan on the first invocation and when using conan install or conan build again, and still allows to override values by the user when using only CMake after an initial conan install/build

But generate() doesn't really call CMake, it just writes the conan_toolchain.cmake file. And conan install and conan build always call the generate() method, so that would overwrite the toolchain file always.

But for BUILD_TESTING it's different. It makes sense to switch that off temporarily (want to check that main code compiles before adapting tests, skip test compilation and execution for performance reasons, ...)

But isn't this more like a building and IDE things? Like this enabled define certain targets in the IDE, and developers just build the targets they want, without really building or running tests. This is true also for other languages and technologies, the environment is setup, including the tests (when working as developer, tests typically disabled in CI), and then the developer has everything in the IDE and works from there, not really going to the terminal to "reinstall" or reconfigure the project to drop or enable testing.

The skip_tests Conan conf (as well as most confs) is not intended as a developer toggle to control tests. It is intended as a CI optimization, to be able to quickly build packages in CI disabling tests, for example when a rebuild is being done as a part of a dependency graph chained cascade re-build. The ideal flow for a developer is conan install ... + cmake ... + IDE/editor work. Going back to conan install should ideally be done only if new dependencies are being added/removed/updated, but not as part of regular development work, and building with/without tests shouldn't require a conan install in the first place.

Note also that BUILD_TESTING cmake variable is not even defined in the conan_toolchain.cmake file. It is directly defined as cache variable in the generated CMake presets file, as the presets are the ones managing the different configure-build-test stages, which is aligned with the above mentioned expectations too.

Finally, it is relevant to understand that before we introduced the tools.graph:skip_test conf, there were users using the tools.build:skip_test conf as a conditional over test_requires, something like:

def build_requirements(self):
    if not self.conf.get("tools.build:skip_test"):
         self.test_requires("mytestdep/version")

That means that enabling or disabling tests also implied relevant changes in the dependency graph (as mytestdep/version can also have other transitive dependencies, often having some overlap with other regular dependencies in the graph). In practice that means that dependencies are being changed, and the amount of things cached in the CMake cache regarding dependencies means in practice that a reset of the CMake cache is necessary.

I am pinging @jcar87 for extra feedback on this matter.

[aagor]

Hi, very thanks for your detailed explanation.

But generate() doesn't really call CMake, it just writes the conan_toolchain.cmake file. And conan install and conan build always call the generate() method, so that would overwrite the toolchain file always.

Yeah, I know, it's at least not trivial to solve.

... then the developer has everything in the IDE and works from there, not really going to the terminal to "reinstall" or reconfigure the project to drop or enable testing.

I think, workflows differ, some developers do everything in IDE, some uses terminal a lot.

Note also that BUILD_TESTING cmake variable is not even defined in the conan_toolchain.cmake file. It is directly defined as cache variable in the generated CMake presets file, as the presets are the ones managing the different configure-build-test stages, which is aligned with the above mentioned expectations too.

I checked conans source and don't understand why BUILD_TESTING is only ever set in one direction.
Why not set it unconditionally?

diff --git a/conan/tools/cmake/presets.py b/conan/tools/cmake/presets.py
index cd7e3db27..cfcb5965c 100644
--- a/conan/tools/cmake/presets.py
+++ b/conan/tools/cmake/presets.py
@@ -51,8 +51,7 @@ class _CMakePresets:
             cache_variables["CMAKE_POLICY_DEFAULT_CMP0091"] = "NEW"

         if "BUILD_TESTING" not in cache_variables:
-            if conanfile.conf.get("tools.build:skip_test", check_type=bool):
-                cache_variables["BUILD_TESTING"] = "OFF"
+            cache_variables["BUILD_TESTING"] = "OFF" if conanfile.conf.get("tools.build:skip_test", check_type=bool) else "ON"

         preset_path = os.path.join(conanfile.generators_folder, "CMakePresets.json")
         multiconfig = is_multi_configuration(generator)

With this change, a conan build . -c tools.build:skip_test=False would always include tests, which sounds more intuitive to me instead of (silently!) don't build tests even if the user explicitly specified otherwise.
In addition, cmake.build() inside the build() method of a Conanfile, will execute tests (as requested), but the old ones. This can lead to the fact, that conan reports all green tests, even if they shouldn't be green anymore due to executing old outdated test binaries.