conan icon indicating copy to clipboard operation
conan copied to clipboard

Added support for tools.cmake.cmaketoolchain:extra_variables

Open perseoGI opened this issue 1 year ago • 4 comments

Changelog: Feature: add support for setting tools.cmake.cmaketoolchain:extra_variables Docs: https://github.com/conan-io/docs/pull/3719 Closes: https://github.com/conan-io/conan/issues/16222

  • [x] Refer to the issue that supports this Pull Request.
  • [x] If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • [x] I've read the Contributing guide.
  • [x] I've followed the PEP8 style guides for Python code.
  • [x] I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

perseoGI avatar May 13 '24 07:05 perseoGI

UPDATE https://docs.conan.io/2/reference/config_files/global_conf.html#configuration-data-operator Merge dictionaries is already implemented and can be achieved from CLI and profile with *= operator


@jcar87 wonder what would happen if there is a collision in defining tools.cmake.cmaketoolchain:extra_variables from different places. Will dictionaries merge? If there is a collision in a key in the dict, which will be the preference order? E.g.

profile

[settings]
...
[conf]
tools.cmake.cmaketoolchain:extra_variables={'A':'hello'}

CLI

conan install . --profile=profile -c tools.cmake.cmaketoolchain:extra_variables="{'B': 'world'}"

conanfile.py

    def generate(self):
        deps = CMakeDeps(self)
        deps.generate()
        tc = CMakeToolchain(self)
        tc.blocks["generic_system"].values["extra_variables"] = {'C': 'good day'}
        tc.generate()

This may be the desired behaviour:

set(A "hello")
set(B "world")
set(C "good day")

But actually, tc.blocks replace all the content of the dict. This can be addressed by indexing with the new key:

tc.blocks["generic_system"].values["extra_variables"]["C"] = "good day"

Also, from python things are easier, if we want to merge the whole dictionary from CLI or profile with the extra_variables defined in the recipe:

tc.blocks["generic_system"].values["extra_variables"] = tc.blocks["generic_system"].values["extra_variables"] | {'foo': 'bar'}

This will generate:

set(B "world")
set(C "good day")

But still have the problem of preference order: CLI will override the whole dictionary defined in the profile section. Do we want a merge mechanism?

perseoGI avatar May 13 '24 08:05 perseoGI

@jcar87 wonder what would happen if there is a collision in defining tools.cmake.cmaketoolchain:extra_variables from different places. Will dictionaries merge? If there is a collision in a key in the dict, which will be the preference order? E.g.

To clarify the merge behavior in the case of Python dicts, it's already explained in this section of the Conan docs. In summary, you should use -c "tools.cmake.cmaketoolchain:extra_variables*="{'B': 'world'}" to update the conf already defined in the profile.

franramirez688 avatar May 13 '24 09:05 franramirez688

Looks good.

The only thing I'd consider, but this is something we need to discuss with @jcar87 too, is if we wanted to move this much later in the toolchain, so it can be used to overwrite other existing variables (see for example #16240). If we do that, in the same spirit we would like to have a user_toolchain_append or similar that injects user files at the end of the toolchain, allowing users to overwrite other possible things that the toolchain is defining and doesn't have another way to fix them.

I've just pushed new changes which creates a new block to handle this new field. This block can be inserted later in order to inject variables later and override possible variables.

perseoGI avatar May 13 '24 15:05 perseoGI

Lets do the following:

  • Lets move the extra variables just before the try_compile block. This will allow users to re-define variables
  • Lets document it explicitly that this is a potential override of CMakeToolchain defined variables, users are at their own risk
  • It would be good to give it a bit more power:
    • Lets make sure that it allows users to prepend/append, maybe with the value of the variable being literal ${VAR} newvalue, and making sure that quotes are not breaking.
    • Lets make sure that users can define also CACHE variables, as sometimes it is necessary
    • If things fit nicely in the dict string values, fine, otherwise we might need to create some additional syntax to support it.

memsharded avatar May 17 '24 10:05 memsharded

I have test this feature with conan version 2.4.1 but i still have problem in overwrite variable in CMakeToolchain I set in profile

[conf]
tools.cmake.cmaketoolchain:user_toolchain=["{{SCE_CMAKE_DIR}}/PS5.cmake"]
tools.cmake.cmaketoolchain:generator=Visual Studio 17 2022
tools.cmake.cmaketoolchain:extra_variables*={'CMAKE_GENERATOR_TOOLSET': {'value': 'PS5Clang', 'cache': True,'type': 'STRING'}}
tools.cmake.cmaketoolchain:extra_variables*={'CMAKE_GENERATOR_PLATFORM': {'value': 'Prospero', 'cache': True,'type': 'STRING'}}
tools.cmake.cmaketoolchain:extra_variables*={'CMAKE_EXE_LINKER_FLAGS_INIT': ""}

Here is generated toolchain file

set(CMAKE_GENERATOR_PLATFORM "x64" CACHE STRING "" FORCE)

message(STATUS "Conan toolchain: CMAKE_GENERATOR_TOOLSET=ClangCL")
set(CMAKE_GENERATOR_TOOLSET "ClangCL" CACHE STRING "" FORCE)

...


set(CMAKE_GENERATOR_TOOLSET "PS5Clang" CACHE STRING "CMAKE_GENERATOR_TOOLSET")
set(CMAKE_GENERATOR_PLATFORM "Prospero" CACHE STRING "CMAKE_GENERATOR_PLATFORM")
set(CMAKE_EXE_LINKER_FLAGS_INIT "")

CMake actually recognizes using the above parameters I check the sourcecode ,it seem can not inject FORCE paramter

peterpuppy avatar Jun 14 '24 03:06 peterpuppy

Hi @peterpuppy first of all thank you for your feedback!

As you have seen, injecting "FORCE" parameter is still not available. We will add it ASAP!

Stay tuned!

perseoGI avatar Jun 14 '24 07:06 perseoGI