CMakeDeps: Fix imported library config suffix

[Untzelmann]

Currently the imported libraries are set without config suffix first. At the end there is one additional set using the config suffix, which overwrites the already set libraries.

Fixes #13504

Changelog: Fix: Fix imported library config suffix.
Docs: Omit

• 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.

Note: I think no documentation changes are required. Should I still make a change in the documentation?

[CLAassistant]

All committers have signed the CLA.

[memsharded]

Thanks for your contribution!

A couple of suggestions:

• If we want this to be part of the 2.0.X patches, it needs to be targeted to the release/2.0 branch. The develop2 branch is intended for 2.1
• Some tests in the test suite that proves that this is solving an issue would be necessary, also to guarantee that this doesn't break in the future. Doing tests might be a bit more difficult, we can try to help contributing them or guiding.
• Signing the CLA is necessary for accepting contributions, please have a look to it when possible.

[Untzelmann]

Ok,

• I changed this to release/2.0. Will it still be in 2.1, or do I need to open a second pull request to get this into both versions?
• I signed the cla.
• I am unsure how to write the test: The issue we wanted to fix is: We have a dependency to a shared library on windows. We want to copy all required DLL files of an exectuable by cmake code like this

add_custom_command(TARGET exe POST_BUILD COMMAND ${CMAKE_COMMAND} -E copy $<TARGET_RUNTIME_DLLS:exe> ${OUTPUT_DIRECTORY} COMMAND_EXPAND_LISTS )

Before the fix, this did not work. After the fix it does. In general, the issue is only observed in Windows, when the if-clause at the top of the function sets IMPORTED_IMPLIB and IMPORTED_LOCATION.

How to write a test for this?

[AbrilRBS]

I changed this to release/2.0. Will it still be in 2.1, or do I need to open a second pull request to get this into both versions?

It will be available in 2.1 too yes :)

How to write a test for this?

The functional tests look like the place to do this, with an decorator guard to only execute on Windows (There are some examples already there), but let us know if you need extra help with that!

[jcar87]

Thanks @Untzelmann - indeed this fixes a bug that existed with two different properties conflicting when on Windows with shared libraries, and the .lib file was being copied instead.

I'll add the pertinent tests ahead of the merge, thanks so much for your contribution!

[Untzelmann]

@jcar87 Thanks for adding the tests for me. I actually intended to have a look at implementing the a test for this, but I will probably not find time for this in the next days. So I am very thankful, if you add the test, and I will still have a look how to implement tests, for my next contributions.

[jcar87]

Hi @Untzelmann - thanks! Just pushed the test which replicates the problem, and tests that the DLL from a dependency is copied next to the executable (also tests that the executable actually runs: if the DLLs isn't there, on the command line it will produce no visible output).

This was indeed a bug, where the IMPORTED_LOCATION property was being overwritten. Since these targets would now only have the IMPORTED_LOCATION_<CONFIG> property, it does make me wonder if there would be any side effects, however the tests to cover a wide range of cases, and if they pass I'm confident that's not the case. If there were any issues in the future, I suspect we would need to set the IMPORTED_CONFIGURATIONS property - I'm hesitant to do so in this PR since it's not directly related to the issue being fixed.

Once again, thanks for contributing and for fixing this! And this is a very good example of using the TARGET_RUNTIME_DLLS generator expression :D

[Untzelmann]

I think it should not have any side effects. We usually only set the _RELEASE / _DEBUG versions in our CMake files and works fine. I think the only reason why the setting of the IMPORTED_LOCATION${config_suffix} was made is, to make sure that the other values are not overwritten, if release and debug is generated for a project. However the IMPORTED_IMPLIB was not considered. It would probably also have been fine to just set the IMPORTED_IMPLIB${config_suffix} at the end. However, then the value of the variables without config suffix would have the values of debug or release at random. So I preferred the solution were we do not set these variables at all.

Thank you for your assistance. By looking at your change for the tests, I think I already understood how that works. My next contributions will contain a unit test directly, then.