Skip to content

Conversation

AbrilRBS
Copy link
Member

@AbrilRBS AbrilRBS commented Aug 25, 2025

Changelog: Feature: Add cmake_extra_variables property for CMakeConfigDeps
Changelog: Feature: Add cmake_extra_variables property for CMakeDeps
Docs: TODO

Closes #18683, this comes from a discussion with the team where it could help around some shortcomings in a few CCI recipes


def is_multi_configuration(generator):
if not generator:
return False
return "Visual" in generator or "Xcode" in generator or "Multi-Config" in generator


def parse_extra_variable(source, key, value):
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a fan of moving this to a utils file, but didn't find a better suited place to share between CMakeToolchain and CMakeConfigDeps, happy to hear alternatives

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 to me.

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.

Maybe we need to provide this for CMakeDeps as well? As CMakeConfigDeps will still take a bit to be production ready, do we want to have this available for ConanCenter sooner?

@@ -28,6 +28,19 @@ def additional_variables_prefixes(self):
self.cmakedeps.get_property("cmake_additional_variables_prefixes", self.conanfile, check_type=list) or [])
return list(set([self.file_name] + prefix_list))

@property
def parsed_extra_variables(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

Note quite sure if the config is the best place for this, I write them here for symmetry with CMakeConfigDeps where they are written next to the cmake_additional_variables_prefixes too, maybe we want to move them elsewhere?

@AbrilRBS AbrilRBS marked this pull request as ready for review August 25, 2025 18:00
@AbrilRBS AbrilRBS changed the title Sketch cmake_extra_variables property for CMakeConfigDeps Add cmake_extra_variables property for CMakeConfigDeps Aug 25, 2025
@AbrilRBS AbrilRBS changed the title Add cmake_extra_variables property for CMakeConfigDeps Add cmake_extra_variables property for CMakeConfigDeps and CMakeDeps Aug 25, 2025
@AbrilRBS AbrilRBS added this to the 2.20 milestone Aug 25, 2025
@jcar87
Copy link
Contributor

jcar87 commented Aug 28, 2025

I would probably suggest:

  • cmake_config_variable_definitions to reflect what it actually is

And my only comment is to make sure that we generate these variables (the set(....) invocations) in the -config.cmake rather than the files included by it. I'm not sure I would bother with the find modules in the legacy CMakeDeps

@memsharded
Copy link
Member

I would probably suggest:
cmake_config_variable_definitions to reflect what it actually is

It was named cmake_extra_variables to align with the conf tools.cmake.cmaketoolchain:extra_variables, because it has a very similar meaning, from the consumer point of view is mostly the same.
But then it is true that it could create some expectations about priority/precedence, overwriting from profiles, etc., which is not happening, it is processed separately.
I think that we should better analyze these possible collisions and expectations, might need some reworking because I think it can create user frustration if users define tools.cmake.cmaketoolchain:extra_variables, but that is ignored because some dependency is defining the same variables and they take precedence, contradicting the Conan principle that the "downstream user" should have control.
I am moving this to next release to analyze this with @AbrilRBS

And my only comment is to make sure that we generate these variables (the set(....) invocations) in the -config.cmake rather than the files included by it. I'm not sure I would bother with the find modules in the legacy CMakeDeps

Yes ,this is already this way in CMakeDeps, and there is the ticket #18836 to do it for CMakeConfigDeps (but this goes together with moving the legacy xxx_INCLUDEDIRS variables, so better do it separately.

@memsharded memsharded modified the milestones: 2.20, 2.21 Aug 28, 2025
@jcar87
Copy link
Contributor

jcar87 commented Aug 28, 2025

tools.cmake.cmaketoolchain:extra_variables cause variables to be defined in the Toolchain, not the config file. It should very very rarely and exceptionally be used in the package_info() of a recipe

the recommendation for what variables should be covered by tools.cmake.cmaketoolchain:extra_variables should be: "whatever cmake recommends or requires to be defined in a toolchain file", and nothing else.

so it's probably okay for a toolchain related recipe (e.g. if I have a recipe for a compiler, or a build tool, and there's something for convenience that I'd like users to generate in a toolchain file), but less okay to use in a recipe for a library (our goal for this is to match upstream config files generated by the project, and those cannot define variables for a toolchain (obviously, as by the time you reach find_package, project() has already run).

There are no uses of tools.cmake.cmaketoolchain:extra_variables in Conan Center.

The only concern would be, users relying on tools.cmake.cmaketoolchain:extra_variables in their own recipes because the feature in this PR does not exist yet.

@memsharded
Copy link
Member

tools.cmake.cmaketoolchain:extra_variables cause variables to be defined in the Toolchain, not the config file. It should very very rarely and exceptionally be used in the package_info() of a recipe

Yes, it is very rarely used there, it is not the intention. But it is widely used by users defining it in profile files.
I am not concerned about recipes defining tools.cmake.cmaketoolchain:extra_variables in its conf_info for their consumers.

My concern is a user defining tools.cmake.cmaketoolchain:extra_variables in their profile for example CMAKE_SOME_VAR, and their provided value being ignored because there is a package up the graph that defines the new cmake_extra_variables property in its package_info, and it defines the same CMAKE_SOME_VAR variable, but with a different value.

If this is the case, at this moment, the dependency value will have priority over the profile value, which is unexpected and can be frustrating for users.

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] Think about adding a way for packages to generate extra cmake variables
3 participants