-
Notifications
You must be signed in to change notification settings - Fork 1k
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 components to cmake_find_package generator (jinja2) #7108
Add components to cmake_find_package generator (jinja2) #7108
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Quick review, I'll have a look to the tests and play with the feature a little bit now.
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.
I need to run some tests locally, I'm worried about some of the lists. I'll come back with a review soon.
if({{ pkg_name }}_FIND_COMPONENTS) | ||
foreach(_FIND_COMPONENT {{ '${'+pkg_name+'_FIND_COMPONENTS}' }}) | ||
list(FIND {{ pkg_name }}_COMPONENTS "{{ pkg_name }}::${_FIND_COMPONENT}" _index) | ||
if(${_index} EQUAL -1) |
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.
Do we want to consider here the inputs QUIET
and REQUIRED
?
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.
Aren't those variables managed by CMake itself? From https://cmake.org/cmake/help/v3.0/command/find_package.html
The QUIET option disables messages if the package cannot be found.
The MODULE option disables the second signature documented below.
The REQUIRED option stops processing with an error message if the package cannot be found.
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.
Oh! Maybe it uses the variable <PKG>_FOUND
?
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.
I've checked my previous comments about CMake lists. It looks like all the combinations (mixin quotes with ;
-separated lists work... at least in my CMake version).
I've left some comments that can improved later, IMO they won't break.
"" | ||
"{{ pkg_name }}_{{ comp_name }}") | ||
|
||
set({{ pkg_name }}_{{ comp_name }}_LINK_LIBS {{ '${'+pkg_name+'_'+comp_name+'_LIB_TARGETS}' }} {{ '${'+pkg_name+'_'+comp_name+'_DEPENDENCIES}' }}) |
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.
The variable {{pkg_name}}_{{compo_name}}_LIB_TARGETS
has already been linked with everything listed in {{ pkg_name }}_{{ comp_name }}_LIBS_FRAMEWORKS_DEPS
(and also with system libraries that are wrongly listed in libs
). So, {{ pkg_name }}_{{ comp_name }}_LIB_TARGETS
should be all be need in line L177. Am I right?
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.
Still, we need to populate the variables with the proper values... and we can remove those that are not documented.
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.
I'm really surprised and really happy to see that this can be implemented so easily (kudos!). I'm sad the resulting FindXXX.cmake
is like a patchwork but from the source POV it is cleaner.
I'm running this branch with some of the existing recipes in Conan Center that declare components 🧑🏭
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.
Now we can use components!
Changelog: Feature: Add components to
cmake_find_package
generator.Docs: conan-io/docs#1722
Close #7116
#tags: slow
Uses a jinja2 template.
Leaves old implementation of
cmake_find_package
generator in recipes without components.Refer to the issue that supports this Pull Request: closes [feature] Components: POC components in the cmake generator #6718 closes Support components on cmake_find_package generator #7084
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.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.