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

Added support for tools.cmake.cmaketoolchain:extra_variables #16242

Open
wants to merge 5 commits into
base: develop2
Choose a base branch
from

Conversation

perseoGI
Copy link
Contributor

@perseoGI perseoGI commented May 13, 2024

Changelog: Feature: add support for setting tools.cmake.cmaketoolchain:extra_variables
Docs: conan-io/docs#3719
Closes: #16222

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

@perseoGI perseoGI changed the title Added support for tools.cmake.cmaketoolchain:extra_variables (#16222) Added support for tools.cmake.cmaketoolchain:extra_variables May 13, 2024
@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from 033e2e0 to d0491bd Compare May 13, 2024 08:07
@perseoGI
Copy link
Contributor Author

perseoGI commented May 13, 2024

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?

@franramirez688
Copy link
Contributor

franramirez688 commented May 13, 2024

@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.

@memsharded memsharded added this to the 2.4.0 milestone May 13, 2024
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@memsharded memsharded self-assigned this May 13, 2024
@perseoGI
Copy link
Contributor Author

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.

@memsharded
Copy link
Member

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.

@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from 593881a to aa753af Compare May 21, 2024 10:23
@memsharded memsharded requested a review from jcar87 May 21, 2024 11:57
os=Windows
arch=x86_64
[conf]
tools.cmake.cmaketoolchain:extra_variables={'CMAKE_GENERATOR_INSTANCE': '"${GENERATOR_INSTANCE}/buildTools/"', 'FOO': '42 CACHE' }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the feature, but I'm not too sure about defining cache variables this way? having the spaces be separators for cmake syntax I'm not too sure.

set(FOO 42 CACHE)

IIRC, this is not valid CMake syntax:

CMake Error at CMakeLists.txt:5 (set):
  set given invalid arguments for CACHE mode.

(I think the type and dosctring are missing, see docs)

I wonder if we could have something like what CMake uses for presets:

      "cacheVariables": {
        "FIRST_CACHE_VARIABLE": {
          "type": "BOOL",
          "value": "OFF"
        },
        "SECOND_CACHE_VARIABLE": "ON"
      },

we could maybe have something like this, where we either take the value (which shouldn't allow spaces unless they are inside double quotes?) or a dictionary with the value, whether it is cache or not, and if it is cache, the other needed info (like the type)

tools.cmake.cmaketoolchain:extra_variables={'CMAKE_GENERATOR_INSTANCE': '"${GENERATOR_INSTANCE}/buildTools/"', 'FOO': {value:'42', cache: true, type: 'bool'}} }

Copy link
Contributor Author

@perseoGI perseoGI May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, take a look at the new design. It is now fully working and tested.
Now we are supporting:

{
'A': 'string value',
'B': 42, 
'C': 1.3,
'D': '${ENV}/test',
'DICT': {'value': '42 sense', 'cache': true, 'type': 'BOOL', 'docstring': 'test cache var'} 
}

Dict values includes:

  • Support for pythonic boolean inputs or string inputs (in cache variable)
  • Descriptive error if cache variable and not type or docstring defined
  • Descriptive error if cache type not in BOOL, FILEPATH, PATH, STRING, INTERNAL list

Generating following CMake output:

set(A 'string value')
set(B 42)
set(C 1.3)
set(D "${ENV}/test")
set(value "42 sense" CACHE BOOL "test cache var")

@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from 3ffb1f8 to aa753af Compare May 21, 2024 15:58
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
perseoGI and others added 4 commits May 29, 2024 09:23
This will allow users to re-define variables
Now it admits not only plain key:value pairs but dictionary values in
the form:
{'value': <value>, 'cache': <true/false>, 'type': <cache_type>, 'docstring': <cache docstring> }
@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from 3f209d7 to a302915 Compare May 29, 2024 07:54
Copy link
Contributor Author

@perseoGI perseoGI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code

conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Show resolved Hide resolved
conan/tools/cmake/toolchain/blocks.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
test/integration/toolchains/cmake/test_cmaketoolchain.py Outdated Show resolved Hide resolved
@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from a302915 to 79bb96f Compare May 29, 2024 09:05
@perseoGI perseoGI force-pushed the perseoGI/16222-feature-cmaketoolchain-extra-variables branch from 79bb96f to ab9487f Compare May 30, 2024 09:52
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

@memsharded memsharded requested a review from jcar87 May 30, 2024 16:00
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

Successfully merging this pull request may close these issues.

[feature] expose CMakeToolchain conf to be able to set CMAKE_GENERATOR_INSTANCE
5 participants