Add IMPORTED_CONFIGURATIONS property for CMake targets

[marlamb]

Some frameworks use the property in order to find libraries, e.g. like https://github.com/ros/catkin/blob/noetic-devel/cmake/catkin_libraries.cmake#L150

In order to support these kind of usages, this commit adds the property to all targets.

This should solve #14606 and #16688.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ marlamb
✅ memsharded
❌ Martin Lambertsen

---

Martin Lambertsen seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[marlamb]

Here the checklist that was initially in the PR description:

• Refer to the issue that supports this Pull Request.
• 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.

[memsharded]

Thanks for your contribution @marlamb

This change might have some risk, so it might not be possible to move it forward (we don't know yet, we need some time to investigate it).

The CI is reporting errors now, you can do as you wish, try to fix them or wait for our investigation. If you want to check the tests, a bunch of tests are broken now in the "functional" folder. I know it is not easy to run them, as they need to have some tools installed and configuring the conftest.py with a conftest_user.py user file, and CI is not visible because of security, so I will try to help reporting the broken tests, and helping if you have any question for running the tests.

[memsharded]

This is broken:

string does not recognize sub-command REGEX_REPLACE

Note that support for CMake 3.15 is necessary.

[marlamb]

Thanks for the feedback, I appretiate your help forwarding the errors from the CI (as I don't have the local setup...) . In this case it was a simple typo (underscore instead of space). Should be fixed by the latest update, lets see what the next CI run says. The used CMake function should be supported at least since 3.0 (never used 2.x before, probably it even existed there).

Do you see any risks apart from the failing CI on my first trial? Setting the property looks to me like a low-risk operation.

[marlamb]

Looks like at least the CI is now happy 😄 .

Regarding the CMake support I just noticed that the same function is already used a few lines above.

[Stadik]

I can verify this modification fixes #16688

In one of our latest pipelines the number of these warnings due to missing IMPORTED_CONFIGURATIONS even filled the gitlab log size limit 😅 ... we hope this can be merged soon

[memsharded]

I am reviewing this to try to merge it for next Conan 2.8.

What would be the best test to cover this functionality? Ideally, a test that is red/broken without this change, and green with it, and that the test is as realistic as possible.

Is it just the iteration over the imported configurations in the consumer side the way to go? I'd prefer if there was something built-in in CMake that fails without this.

[marlamb]

Great to hear that this diff will make it into the next version! But I am sorry, I don't know a better way to test it. The way I understand the CMake documentation, the main benefit is having the list, as it requires that other properties on the target have to be set as well (but not vice versa). And the benefit of having a list is usually to be able to iterate over it. If I don't miss something (and I am far from an CMake expert, so that might the case ;-) ) looping and checking for the other properties to apply them if present is an appropriate test.

[memsharded]

I have added a test for the PR, please review it.

The most important part is that consumers cannot iterate the package pkgname::pkgname target, because that is just an interface to the real libs, which has an internal name like CONAN_LIB::pkgname_libname_RELEASE, which seems to defeat the original purpose of the change.

[marlamb]

The comment is pointing to another issue? Shouldn't this be #14606 and #16688?

[memsharded]

Yes, copy & paste, thanks for pointing this out, I'll fix it in next commit.

[memsharded]

@marlamb any feedback about this:

The most important part is that consumers cannot iterate the package pkgname::pkgname target, because that is just an interface to the real libs, which has an internal name like CONAN_LIB::pkgname_libname_RELEASE, which seems to defeat the original purpose of the change.

I think that having to use the CONAN_LIB::pkgname_libname_RELEASE target name in order to retrieve the RELEASE configuration is kind of pointless already.
If I understood correctly @Stadik in #14606 (comment), the addition might avoid the warnings, but that would be the only benefit at the moment.

[marlamb]

Hi and sorry for the late reply. Unfortunately I will be very busy the next weeks and cannot foresee when I will be able to have a closer look at your observation. Just a few spontaneous thoughts on it:

• I could imagine that frameworks trying to find targets in a more generic way than human users (which would clearly use the pkgname::pkgname target) tend to come across the aliases and finally work on the "real" targets. Also they cannot assume (or know) that by chance the build type is already part of the target name.
• The newly added property naturally belongs to other properties that are set for the internal targets. Are these other properties available on the public targets? At least I don't remember explicit code in the generated CMake files setting the properties for the public targets, but I have to admit I didn't have a thorough look at it now. In case they are not available as well, the similar question would arise: why are they set at all?

Perhaps it is just a combination of both points that might make the addition useful, which might be quite difficult to test in a reasonably small unit test, but that has to be verified.

[Stadik]

I think that having to use the CONAN_LIB::pkgname_libname_RELEASE target name in order to retrieve the RELEASE configuration is kind of pointless already.

As I see it the creation of an internal library target instead of the pure config specific properties already kind of blocks the intended usage of CONFIG for multi-config generators in cmake, since it creates of map of a (multi-)target with CONFIG specific properties (pkgname::pkgname) against multiple single-config targets (CONAN_LIB::pkgname_libname_CONFIG)

The problem I see is it is must from cmake side to either define config-less properties like LOCATION together with config specific ones or to define available configs in IMPORTED_CONFIGURATIONS

As a consumer for us it currently looks like this:

• pkgname::pkgname is linked, it is only alias so nothing special to do
• we walk its INTERFACE_LINK_LIBRARIES and find CONAN_LIB::pkgname_libname_CONFIG which is an IMPORTED target so it should define a physical object
• we check IMPORTED_LOCATION -> not defined + warning since cmake thinks is MUST be defined
• we check LOCATION also not defined
• so we must check config specific IMPORTED_LOCATION_CONFIG (where CONFIG is either CMAKE_BUILD_TYPE or the current pass of CMAKE_CONFIGURATION_TYPES)
• we cannot assume a proper MAP between configs in case some are not defined, and also not that the CONFIG matching CMAKE_BUILD_TYPE exists at all, so we first need to check the available CONFIGS ... which is empty since IMPORTED_CONFIGURATIONS is not set
• so the conclusion is, even though it is an IMPORTED target, it seems to not specify any LOCATION where we can find the object(s) it defines

So cmake is kind of correct with its warning.

[marlamb]

What @Stadik describes suits my expectation of human written consumer CMake vs. a generic framework trying to get information from the targets, which goes beyond the alias. Looks to me like a totally valid use-case, which unfortunately is not easy to reflect in a simple unit test.

[Stadik]

Not sure if it still helps, but considering the targets CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix} that are generated by cmakedeps_macros.cmake

Just checking for LOCATION property is enough to test this fix

# Gives the warning about IMPORTED_LOCATION not set  
get_target_property(LOC CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix} LOCATION)

# LOC is CONAN_LIB::<...>-NOTFOUND if IMPORTED_CONFIGURATIONS are not defined, otherwise the physical location
if(LOC STREQUAL "CONAN_LIB::${package_name}_${_LIBRARY_NAME}${config_suffix}-NOTFOUND")
 -> FAIL
endif()

[memsharded]

We are finally moving forward the new CMakeDeps generator, which produces the right IMPORTED targets, with locations and IMPORTED_CONFIGURATIONS.

So we have considered that it is better to go for the full correct solution, rather than trying to apply this partial patch that doesn't really solve the main issues.

I am closing this PR then in favor of #16964, feedback very welcome!