[POC][CMakeConfigDeps] Create CONFIG files per component#19681
[POC][CMakeConfigDeps] Create CONFIG files per component#19681franramirez688 wants to merge 33 commits into
Conversation
memsharded
left a comment
There was a problem hiding this comment.
Looking better, good job!
I still think we might want to do it in 2 stages, and even also some previous refactor over the complicated parts of CMakeConfigDeps core class before the introduction, but lets talk about it.
| foo-config-version.cmake | ||
| """ | ||
| def __init__(self, cmakedeps, conanfile): | ||
| def __init__(self, cmakedeps, conanfile, config_comp_name, cmake_file_name): |
There was a problem hiding this comment.
Fantastic, I think this is really great
memsharded
left a comment
There was a problem hiding this comment.
I am now struggling a bit to understand the changes and risks. Maybe now a previous PR that is a pure refactor would make change.
ef83505 to
95ca476
Compare
| components belonging to it. The first one mentioned in the | ||
| list will act as the root one. For example: | ||
| self.cpp_info.set_property("cmake_file_component_names", { | ||
| "CMAKE_FILE_NAME1": ["COMP1", "COMP2"], |
There was a problem hiding this comment.
Should this be the component name, and let the component define the cmake file name via a property? (Or the default one if the component name can be used as a config name)
| Get the name of the files for the find_package(XXX) for the root and the rest of | ||
| the components. | ||
| This is used by CMakeConfigDeps to determine: | ||
| - The filename to generate (XXX-config.cmake or FindXXX.cmake) |
There was a problem hiding this comment.
| - The filename to generate (XXX-config.cmake or FindXXX.cmake) | |
| - The filename to generate (XXX-config.cmake) |
no module mode
ed62c75 to
3d83384
Compare
| def package_info(self): | ||
| self.cpp_info.components["core"].libs = ["core"] | ||
| self.cpp_info.components["core"].type = "static-library" | ||
| self.cpp_info.components["core"].location = "lib/libcore.a" |
There was a problem hiding this comment.
Could you add a requires to extra here and check that the find_dependency appears in pkg-config?
| self._cmake_file_name = cmake_file_name | ||
| self._cmake_file_info = cmake_file_info | ||
| self._full_cpp_info = cmake_file_info["full_cpp_info"] |
There was a problem hiding this comment.
This is nice for the POC, but it seems a bit of hiper-parametrization, passing too many different and interconnected objects.
But most of it can be abstracted to:
self._cmakedepsis needed for:- A set of properties for this
xxx-config.cmakefile - The filename computation
- Access to the consumer conanfile
- A set of properties for this
self._conanfileis needed for:- The package name and version
- The cpp_info.components (not sure why)
So it seems that the first thing we want to do is to clearly parameterize the ConfigTemplate2 class (and the other helpers classes) to provide a more direct and explicit parameterization of what is needed. Then the CMakeConfigDeps is free to pass it the information for 1 file or multiple files.
There was a problem hiding this comment.
I have done a quick POC of this refactor in #19880.
I know it is not that simple for the other files, but I think the idea is still valid
There was a problem hiding this comment.
That sounds fine.
The cpp_info.components (not sure why)
It's not really the cpp_info.components object; they are the components belonging to each CMake file.
|
This is looking really good in my opinion. About the naming discussion, maybe instead of About how the root config is resolved, maybe it could be determined from the effective For example, if self.cpp_info.set_property("cmake_file_name", "CoreLib")
self.cpp_info.set_property("cmake_config_files", {
"CoreLib": {
"components": ["core", "core-utils"],
},
"CoreLibExtra": {
"components": ["extra"],
},
})Then If name = "mypackage"
...
self.cpp_info.set_property("cmake_config_files", {
"mypackage": {
"components": ["core"],
},
"mypackage-tools": {
"components": ["tools"],
},
})Then What do you think? |
memsharded
left a comment
There was a problem hiding this comment.
I have reviewed now only the tests, and I think from the UX/UI point of view, it is looking good.
|
|
||
| def package_info(self): | ||
| # Split into separate config files per CMake package name | ||
| self.cpp_info.set_property("cmake_file_component_names", { |
There was a problem hiding this comment.
Maybe the cmake_file_name could be reused, and if it is a dict then the meaning is this one?
That would make the recipe doing that breaking for previous Conan versions, but I also think this property won't work for consumers that use an older Conan version, so a required_conan_version would still be necessary?
There was a problem hiding this comment.
Overall, I like the approach of centralizing the definition of the different files and components in one place.
Changelog: Feature: CMakeConfigDeps can create complete CONFIG files per component.
Docs: https://github.com/conan-io/docs/pull/XXXX
Closes: #19407
Checked on CCI repo
spirv-tools
Applying this diff, it should work locally on your native env: