Map MinSizeRel and RelWithDebInfo correctly

[Neumann-A]

Unfortunately CMakes default is to map to debug.

To be more exact. It defaults to anything which is first in CMAKE_CONFIGURATION_TYPES

[zhihaoy]

This is the toolchain file, which will affect user projects' default setting (for example, a correct setup to use RelWithDebInfo for profiling, which only requires mapping specific packages in users' cmake files). For short, the fact that vcpkg doesn't provide RelWithDebInfo configuration doesn't imply that user's RelWithDebInfo configuration is broken, therefore this change is not acceptable to me.

[Neumann-A]

@zhihaoy Short Answer: Think again.
Middle Long Answer: Google the problem
Example:
https://www.google.com/search?q=RelWithDebInfo+links+debug+libraries&rlz=1C1CHBF_deDE733DE733&oq=RelWithDebInfo+links+debug+libraries&aqs=chrome..69i57.9687j1j7&sourceid=chrome&ie=UTF-8
or
https://stackoverflow.com/questions/24262081/cmake-relwithdebinfo-links-to-debug-libs

Long Answer:
The problem is the default CMake behavior is broken!
Why?

• vcpkg only builds debug und release libraries by default (and does not support other configurations currently)
• external projects uses vcpkg and tries to build MinSizeRel or RelWithDebInfo
• User will experience that his MinSizeRel or RelWithDebInfo are broken because CMake defaults to linking against the debug libraries. This is unexpected behavior from the users perpective. Without any active intervention his build broke. So this PR just fixes the default CMake behavior to do the correct thing.

doesn't imply that user's RelWithDebInfo configuration is broken,

It does because CMake will link the debug libraries provided by vcpkg in these cases.

After reading those two:
https://cmake.org/cmake/help/v3.14/variable/CMAKE_MAP_IMPORTED_CONFIG_CONFIG.html?highlight=cmake_map_imported#variable:CMAKE_MAP_IMPORTED_CONFIG_%3CCONFIG%3E

https://cmake.org/cmake/help/v3.14/prop_tgt/MAP_IMPORTED_CONFIG_CONFIG.html#prop_tgt:MAP_IMPORTED_CONFIG_%3CCONFIG%3E

I will make a slight change to the PR because the value can actually be a list!

[zhihaoy]

@zhihaoy Short Answer: Think again.
Middle Long Answer: Google the problem
Example:
https://www.google.com/search?q=RelWithDebInfo+links+debug+libraries&rlz=1C1CHBF_deDE733DE733&oq=RelWithDebInfo+links+debug+libraries&aqs=chrome..69i57.9687j1j7&sourceid=chrome&ie=UTF-8
or
https://stackoverflow.com/questions/24262081/cmake-relwithdebinfo-links-to-debug-libs

Long Answer:
The problem is the default CMake behavior is broken!
Why?

* only builds debug und release libraries by default (and does not support other configurations currently)

* external projects uses vcpkg and tries to build MinSizeRel or RelWithDebInfo

* User will experience that his MinSizeRel or RelWithDebInfo are broken because CMake defaults to linking against the debug libraries. This is unexpected behavior from the users perpective. Without any active intervention his build broke. So this PR just fixes the default CMake behavior to do the correct thing.

doesn't imply that user's RelWithDebInfo configuration is broken,

It does because CMake will link the debug libraries provided by vcpkg in these cases.

Let me put it in this way: it breaks my company's build. The reason is that overwriting CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO overwrites all user targets' default, and we have working RelWithDebInfo build. How it works? Only one of the dependency coming from vcpkg needs to be remapped, and we remapped it. I fully understand that the default setting does not work out of box, but your proposed change breaks the working builds, and users will suffer from it because they will have to learn a new default.

[Neumann-A]

@zhihaoy Have you actually tested it with the latest commit? The PR is changed to:

 set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL MinSizeRel Release)
 set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO RelWithDebInfo Release)

so it should not change the mapping if MinSizeRel and RelWithDebInfo are available.

[zhihaoy]

@zhihaoy Have you actually tested it with the latest commit? The PR is changed to:

 set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL MinSizeRel Release)
 set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO RelWithDebInfo Release)

so it should not change the mapping if MinSizeRel and RelWithDebInfo are available.

Just tried (which triggered a full rebuild...), and it's worse than I thought. I used to think that it probably builds, but shows no symbol in profiler, but what I actually observed is:

-- VCPKG: Multi configuration generator detected! Mapping MinSizeRel and RelWithDebInfo correctly!

And finally

LINK : fatal error LNK1104: cannot open file 'MKL::MKL-NOTFOUND.obj'

From some Intel MKL lookup script. Exact reason needs more investigation, but you see the problem:
When a user did not define CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO, changing this changes all targets' default (because this is a toolchain file! I don't mind if it only affects vcpkg ports). My project does not define CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO, it only has this:

set_target_properties(unofficial::mp::mp PROPERTIES
    MAP_IMPORTED_CONFIG_MINSIZEREL Release
    MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release)

A per-target based remapping.

[Neumann-A]

changes all targets' default

all imported targets.

but shows no symbol in profiler

thats strange but you were probably wrongly relying on the debug configuration here for imported targets (that would be a hidden bug in your build). Symbols for normal targets should be visible.

and this:

 set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL MinSizeRel Release)
 set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO RelWithDebInfo Release)

should not change anything expect for CMake to use Release instead of Debug in the case that a imported target does not provide MinSizeRel or RelWithDebInfo in IMPORTED_CONFIGURATIONS

[zhihaoy]

changes all targets' default

all imported targets.

I see.

but shows no symbol in profiler

I haven't observed this because the executable doesn't built yet.

and this:

 set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL MinSizeRel Release)
 set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO RelWithDebInfo Release)

should not change anything expect for CMake to use Release instead of Debug in the case that a imported target does not provide MinSizeRel or RelWithDebInfo in IMPORTED_CONFIGURATIONS

The current issue I saw is in a third-party script.

[zhihaoy]

 set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL MinSizeRel Release)
 set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO RelWithDebInfo Release)

should not change anything expect for CMake to use Release instead of Debug in the case that a imported target does not provide MinSizeRel or RelWithDebInfo in IMPORTED_CONFIGURATIONS

The current issue I saw is in a third-party script.

So, although I don't know why, but here is the issue. This script https://github.com/arrayfire/arrayfire/blob/master/CMakeModules/FindMKL.cmake is configuration agnostic. It only sets IMPORTED_LOCATION rather than IMPORTED_LOCATION_DEBUG for example. It works with multi-configuration generator. But with CMAKE_MAP_IMPORTED_CONFIG set in the project, it goes MKL-NOTFOUND. So I acknowledged that CMAKE_MAP_IMPORTED does not change all targets, but the set of the targets it changes is not only vcpkg ports either.

[Neumann-A]

Ah I see after rereading the doc your linker problem should be solved be adding an empty element to the list.

[Neumann-A]

@zhihaoy: my latest commit should take care of your linker problem with MKL. Please retry.

The relevant section from the cmake docs:

The first configuration in the list found to be provided by the imported target (i.e. via IMPORTED_LOCATION_ for the mapped-to ) is selected. As a special case, an empty list element refers to the configuration-less imported target location (i.e. IMPORTED_LOCATION).

[zhihaoy]

OK, it works at least.
However, I will probably just set the two mappings in my project to silence the message if this change is merged, which comes to the next question: would it be better to document and recommend this setting?

[cenit]

I’d just remove the message, since it would appear for every build, even when not necessary. Am I right?

[Neumann-A]

@zhihaoy @cenit: I am always for more information then less information. I added an option to turn the messages on and off (off by default). So I hope you two are fine with it now ;)

[Neumann-A]

Side note: One could alternativly use "MinSizeRel;Release;;${CMAKE_CONFIGURATION_TYPES}" as a mapping to have a fallback to the CMake default behavior. The question is, if probably wrong CMake behavior should be promoted.

[seanyen]

@ras0219-msft @dan-shaw I'd like to share that we came to the same issues for ROS on Windows community. If someone uses RelWithDebInfo and tries to make use of the stock ports from vcpkg, some ports fall into this kind and CMake fails to discover the right configurations (For example, realsense2 and sdl2).

I saw this pull request is attempting to fix this problem universally, which is good and I love to see it is moving forward. And I am also wondering, before the final solution is concluded, what's the recommended mitigation for who consuming vcpkg by this way?

[dan-shaw]

@Neumann-A @seanyen Thanks for the feedback everyone. After some discussion, we've decided that it makes sense to merge this. After the MacOS CI pipeline is back up, I'll merge this.

[silverqx]

@Neumann-A I'm solving exactly this in my cmake build, I want to ask, why you set it like this:

set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL "MinSizeRel;Release;")

and didn't also add RelWithDebInfo like this:

set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL "MinSizeRel;Release;RelWithDebInfo;")

The same for:

set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;")
set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO "RelWithDebInfo;Release;MinSizeRel;")

I wonder if you had a good reason for that? Thank you for your clarification? 🙏