-
Notifications
You must be signed in to change notification settings - Fork 981
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
Drop properties from legacy cmake generators #10098
Conversation
@@ -74,8 +74,8 @@ class CMakeFindPackageGenerator(GeneratorComponentsMixin, Generator): | |||
|
|||
set({{ pkg_name }}_COMPONENTS {{ pkg_components }}) | |||
|
|||
if({{ pkg_name }}_FIND_COMPONENTS) | |||
foreach(_FIND_COMPONENT {{ '${'+pkg_name+'_FIND_COMPONENTS}' }}) | |||
if({{ pkg_filename }}_FIND_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.
was this a bug? How is this connected to not listening to properties?
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 commented this here, as it's also happening for CMakeDeps, I have extended test_component_not_found_cmake
to cover this.
The thing is that CMake generates a <PackageName>_FIND_COMPONENTS
when COMPONENTS
are specified in the find_package
, but we are using pkg_name
here that may not be the same as the <PackageName>
CMake variable that is equivalent to our filename
.
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 was going the fix this in for CMakeDeps in the PR for CMakeDeps and absolute targets but I can fix all of them in another PR as well...
Please, lets make sure to validate this, the |
Does this not contradict the migration docs? If I read that correctly,
should be synonymous with
But as of this PR that appears to be no longer the case, and I need to set both if I want the recipe to be compatible with downstream recipes using the |
Yes, maybe the docs are not clear enough: they are completely different and exclusive mechanisms. If you want to keep your recipes with
Lets add a note for that clarifying in the docs. |
Changelog: Feature: Legacy cmake generators (
cmake_find_package
,cmake_find_package_multi
) don't listen to newset_properties
model anymore.Changelog: Fix: Fix
<PackageName>_FIND_COMPONENTS
CMake generated variable to the correct value associated with the filename, not the package name.Docs: conan-io/docs#2316
Related to: #10077 (comment)
#TAGS: slow