-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add cmake_extra_variables
property for CMakeConfigDeps
and CMakeDeps
#18822
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
Add cmake_extra_variables
property for CMakeConfigDeps
and CMakeDeps
#18822
Conversation
There was a problem hiding this 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.
test/functional/toolchains/cmake/cmakedeps2/test_cmakeconfigdeps_new.py
Outdated
Show resolved
Hide resolved
test/functional/toolchains/cmake/cmakedeps2/test_cmakeconfigdeps_new.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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?
return list(set([self.file_name] + prefix_list)) | ||
|
||
@property | ||
def parsed_extra_variables(self): |
There was a problem hiding this comment.
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?
cmake_extra_variables
property for CMakeConfigDeps
cmake_extra_variables
property for CMakeConfigDeps
cmake_extra_variables
property for CMakeConfigDeps
cmake_extra_variables
property for CMakeConfigDeps
and CMakeDeps
I would probably suggest:
And my only comment is to make sure that we generate these variables (the |
It was named
Yes ,this is already this way in |
the recommendation for what variables should be covered by 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 The only concern would be, users relying on |
Yes, it is very rarely used there, it is not the intention. But it is widely used by users defining it in profile files. My concern is a user defining 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. |
…rilRBS/conan into ar/cmake-extra-variables-package_info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this approach is a bit more understandable than the other with CMake vars 👍
Changelog: Feature: Add
cmake_extra_variables
property forCMakeConfigDeps
.Changelog: Feature: Add
cmake_extra_variables
property forCMakeDeps
.Docs: conan-io/docs#4257
Closes #18683, this comes from a discussion with the team where it could help around some shortcomings in a few CCI recipes