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

CMakeDeps managing CMAKE_MAP_IMPORTED_CONFIG_XXX #12049

Merged
merged 9 commits into from
Nov 22, 2022

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Sep 7, 2022

Changelog: Bugfix: The CMakeDeps generator was not working properly when the consumer maps configurations of the dependencies using CMAKE_MAP_IMPORTED_CONFIG_XXX.
Docs: conan-io/docs#2826

Close #12041

@lasote lasote added this to the 1.53 milestone Sep 7, 2022
@lasote lasote requested review from jcar87 and memsharded September 7, 2022 08:04
Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, a bit risky, but IMPORTED is what should be used, and config suffix too

@czoido czoido modified the milestones: 1.53, 1.54 Oct 4, 2022
@memsharded memsharded modified the milestones: 1.54, 1.55 Nov 2, 2022
Copy link
Contributor

@jcar87 jcar87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me with two caveats:

  • Without this fix, a situation where a consumer project as "Debug" as the build type, and the dependencies where installed as "Release" - CMake gives you a warning (or an error dependency on policy). The warning is indicative of the mismatch and the potential problems. With this fix in the same situation, there's no longer a CMake warning, but compilation will fail. - this may make the user experience confusing.
    If the consumer project does not explicitly do mapping, the observed behaviour is the same with and without this PR.

  • The CMAKE_MAP_IMPORTED_CONFIG_ in the consumers side accept, as value, a list, not just a single config. If we have a situation where a consumer runs conan install for multiple build types, and the CMake side has a configuration that can be satisfied by "any" of those - they could express something like set(CMAKE_MAP_IMPORTED_CONFIG_RELEASE Release RelWithDebInfo MinSizeRel Debug ""). I'd say this is very unusual, but as explained in this CMake issue, that would cause multiple generator expressions to be matched, likely resulting in libraries from different build times to be simultaneously linked...

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

Successfully merging this pull request may close these issues.

[bug] MAP_IMPORTED_CONFIG_* does not work in CMakeDeps
4 participants