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

[question] CMakeToolchain: When use cached_variables vs variables #11937

Open
1 task done
uilianries opened this issue Aug 23, 2022 · 7 comments
Open
1 task done

[question] CMakeToolchain: When use cached_variables vs variables #11937

uilianries opened this issue Aug 23, 2022 · 7 comments

Comments

@uilianries
Copy link
Member

uilianries commented Aug 23, 2022

The documentation only explains CMakeToolchain.variables and CMakeToolchain.cached_variables features, one is better for multiple configuration and another is single configuration:

https://docs.conan.io/en/latest/reference/conanfile/tools/cmake/cmaketoolchain.html#variables

But it's not clear when each one should be used, based on real examples.

So far, we are migrating recipes in CCI and there is a doubt about which one should be adopted by default when using CMakeToolchain.

@memsharded
Copy link
Member

So far, we are migrating recipes in CCI and there is a doubt about which one should be adopted by default when using CMakeToolchain.

Do you have some examples of such variables? that would help also to learn about the use cases. I ask this question to myself when implementing things, and it typically depends on the variable itself.

@jcar87
Copy link
Contributor

jcar87 commented Aug 23, 2022

This has come up as well in other issues so I'll try to summarise.

  • CMakeToolchain.variables are variables that are defined directly in the generated conan_toolchain.cmake file (with set(VAR_NAME "var_value")
  • CMakeToolchain.cache_variables are defined in the generated CMakePresets file, and when Conan drives the build, are passed via -D variable definitions to the cmake command. Additionally, if users are in a local workflow and are using a recent version of CMake, they can rely on the CMake presets files generated by Conan too (but they don't have to).

There are practical differences:

  • -D variables are defined as cache variables from the beginning of the run, and the value is respected by other re-definitions as cache variables (e.g. set(VAR_NAME CACHE "" "var_value")
  • Variables set in the toolchain file are not defined from the beginning of the run, the toolchain file is loaded at some point during the first call to project(), and then it is re-loaded multiple times in different contexts (like Cmake invoking itself to test the capabilities of the toolchain).

This second behaviour is where we may find issues in the following scenarios:

  • If a project's CMakeLists defines cache variables before the call to project(), that is, before the toolchain is loaded, and gives them values. If the toolchain defines them as regular (non-cache) variables, references to that variable will pick up that value. If the toolchain were to define them as cache variables, the original value would be respected.
  • If the user provides values directly via the command line with (-D) and the Conan generated toolchain gives the variables a different value (that may or may not be a cache variable) - CMake will respect the value set by the toolchain at least from that point (when it is loaded via project()), but it may be confusing user experience.
    For example, if I have a recipe that I may build locally (with the local workflow, rather than via create), and the toolchain generated by Conan sets BUILD_SHARED_LIBS, and then the user wants to override that in cmake via -DBUILD_SHARED_LIBS (because they're trying something), this is what's going to happen:
Build shared libs: ON
-- Using Conan toolchain: /Users/luisc/.conan/data/hello/0.1/_/_/build/f7023de70d0bfc63077b5172ade69e4423b9047c/build/generators/conan_toolchain.cmake
-- Conan toolchain: Setting CMAKE_POSITION_INDEPENDENT_CODE=ON (options.fPIC)
-- Conan toolchain: Setting BUILD_SHARED_LIBS = OFF
Build shared libs: OFF

There may be more cases that we haven't seen because this hasn't been used more widely.

The recommended approach (at least in Craig Scott's book) is for CMake toolchain to define as few variables as possible. Some variables are intended to be defined by the toolchain rather than other means, but for everything else we need to consider that the project may have its own defaults that a user may override via command line -D switches (= cache variables from the get go), which is not something that is fully emulated by defining the variables in the toolchains (whether as cache variables or not).

We are dealing with a tradeoff: we also don't want users in local workflows to have to define variables manually when they invoke CMake themselves, so the "single" -DCMAKE_TOOLCHAIN_FILE=conan_toolchain.cmake is a very convenient user experience. We could rely on presets, but that would restrict the convenience to users using the most recent versions of CMake - and we have committed to supporting CMake 3.15 in Conan 2.

As for recipes in Conan Center Index, in the interim, and under an assumption that those are built by CI in Conan Center, or built with create or --build=missing or similar (that is, Conan drives the build itself), I would opt for tc.cache_variables to cause Conan to pass them as -D variables and avoid unwanted side effects.

@uilianries
Copy link
Member Author

As for recipes in Conan Center Index, in the interim, and under an assumption that those are built by CI in Conan Center, or built with create or --build=missing or similar (that is, Conan drives the build itself), I would opt for tc.cache_variables to cause Conan to pass them as -D variables and avoid unwanted side effects.

Okay, we have an answer. I'll use it as recommendation for future PRs.

/cc @SpaceIm @jwillikers

@SpaceIm
Copy link
Contributor

SpaceIm commented Aug 23, 2022

  • CMakeToolchain.variables are variables that are defined directly in the generated conan_toolchain.cmake file (with set(VAR_NAME "var_value")

From what I've seen, they are passed as set(VAR_NAME "var_value" CACHE <type> "Variable VAR_NAME conan-toolchain defined") (conan-io/docs#2689). So the new type cache_variables is quite misleading (it's cache_variables in CMakePresets terminology AFAIK). If they were not passed as CACHE variables, they couldn't override options until CMake >=3.13 policies. It's what happens to BUILD_SHARED_LIBS: #11840

From my experimentations:

  • CMAKE_POLICY_DEFAULT_CMPxxxx must be passed as cache_variables.
  • Anything else can be passed as variables.
  • if BUILD_SHARED_LIBS or CMAKE_POSITION_INDEPENDENT_CODE are turned as option() in upstream CMakeLists and cmake_minimum_required() is lower than 3.13 , you have to either:
  • if BUILD_SHARED_LIBS or CMAKE_POSITION_INDEPENDENT_CODE are turned as CACHE variables in upstream CMakeLists, you have to pass BUILD_SHARED_LIBS/CMAKE_POSITION_INDEPENDENT_CODE as variables (or cache_variables).
  • if upstream CMakeLists is illformed, things can go wrong, specially for variables (and it may not be obvious to see it), for example:
    • cmake_minimum_required() missing.
    • project() before cmake_minimum_required().
    • option() or CACHE variables between cmake_minimum_required() & project() (pernicious, you may not see that recipe options injected through conan toolchain are not honored).

@jwillikers
Copy link
Contributor

Yeah, that fits with why I was using them in the first place. @uilianries I'll update my PR's to use cache variables. Thanks for opening the issue.

@uilianries
Copy link
Member Author

@jwillikers Sorry! 😓

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

No branches or pull requests

5 participants