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

Add components to cmake_find_package generator (jinja2) #7108

Merged
merged 36 commits into from
Jun 5, 2020

Conversation

danimtb
Copy link
Member

@danimtb danimtb commented May 27, 2020

Changelog: Feature: Add components to cmake_find_package generator.
Docs: conan-io/docs#1722

Close #7116

#tags: slow

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.

@danimtb danimtb added this to the 1.26 milestone May 27, 2020
@danimtb danimtb self-assigned this May 27, 2020
@danimtb danimtb changed the title Feature/components find package Add components to cmake_find_package generator (jinja2) May 27, 2020
@danimtb

This comment has been minimized.

@danimtb

This comment has been minimized.

@jgsogo

This comment has been minimized.

@danimtb danimtb marked this pull request as ready for review May 29, 2020 14:10
Copy link
Contributor

@jgsogo jgsogo left a 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.

@danimtb danimtb assigned memsharded and unassigned danimtb Jun 4, 2020
Copy link
Contributor

@jgsogo jgsogo left a 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)
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

@jgsogo jgsogo left a 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.


⚠️ We should keep providing variables listed here in the docs: https://docs.conan.io/en/latest/reference/generators/cmake_find_package.html#variables-in-find-pkg-name-cmake, they are common variables and many people use them, if we stop providing these vars we will break consumers just because the requirement has started to use components.

""
"{{ pkg_name }}_{{ comp_name }}")

set({{ pkg_name }}_{{ comp_name }}_LINK_LIBS {{ '${'+pkg_name+'_'+comp_name+'_LIB_TARGETS}' }} {{ '${'+pkg_name+'_'+comp_name+'_DEPENDENCIES}' }})
Copy link
Contributor

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?

Copy link
Contributor

@jgsogo jgsogo left a 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.

Copy link
Contributor

@jgsogo jgsogo left a 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 🧑‍🏭

Copy link
Contributor

@jgsogo jgsogo left a 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants