use CMAKE_CONFIGURATION_TYPES instead of explicitly defining some

[JulianH1989]

I started using this wrapper.
In Our CMake project we are using quite a big list of compilers and additional tools.

Some of these tools (like clang-tidy) rely on a correct (cmake generated) compile_commands.json.
Since this wrapper only invokes conan for debug and release the generated compile_commands.json is malformed which breaks external tools like clang_tidy.

It's fine to exclude unneeded configs for example by overwriting the CMAKE_CONFIGURATION_TYPES list or specifying CONAN_INSTALL_BUILD_CONFIGURATIONS but this should not be done be default.

[CLAassistant]

All committers have signed the CLA.

[geertbleyen]

By coincidence, I was doing the exact same thing and was thinking, oh #707 can be used for this.
But now I'm wondering if CONAN_INSTALL_BUILD_CONFIGURATIONS should actually be the same as CMAKE_CONFIGURATION_TYPES ? Does it make sense to use either?

[JulianH1989]

By coincidence, I was doing the exact same thing and was thinking, oh #707 can be used for this. But now I'm wondering if CONAN_INSTALL_BUILD_CONFIGURATIONS should actually be the same as CMAKE_CONFIGURATION_TYPES ? Does it make sense to use either?

I'd actually recommend to overwrite CMAKE_CONFIGURATION_TYPES in the actual project (not the conan provider) if needed. If you do so, you will not break any downstream tools

[geertbleyen]

Yes, that's also what I'd do. I'm just wondering, what is the sense in both being able to define CMAKE_CONFIGURATION_TYPES and CONAN_INSTALL_BUILD_CONFIGURATIONS, as they sort of achieve the same thing in conan_provider.cmake, no? I'd prefer to only have CMAKE_CONFIGURATION_TYPES as CONAN_INSTALL_BUILD_CONFIGURATIONS tries to achieve the same thing but in a more convoluted manner unless I'm missing something

[jcar87]

CMAKE_CONFIGURATION_TYPES is used to tell CMake which build types are available on multi-config generators for your project and CONAN_INSTALL_BUILD_CONFIGURATIONS is to tell Conan which configurations to use for the dependencies.

Since the default value for the former is the four configurations, it would be overkill to require the 4 configurations for all dependencies. For example, on Linux (where Debug and Release tend to be binary compatible) - users are typically okay using Debug for their own project, but Release for all dependencies (for performance purposes).

So I'd say these serve two different purposes. In my experience, I'd say devs who override CMAKE_CONFIGURATION_TYPES are in the minority, and most just run with CMake defaults and only build what they need.

Some of these tools (like clang-tidy) rely on a correct (cmake generated) compile_commands.json.
Since this wrapper only invokes conan for debug and release the generated compile_commands.json is malformed which breaks external tools like clang_tidy.

It would be useful to know what exactly is broken, preferably with a reproducible example. My intuition tells me that this is either an issue with the CMakeDeps generator, or it could easily be solved by mapping configurations so that CMake does the right thing.

I'm not convinced it is a good idea to switching the defaults to require dependencies to be installed with all 4 configurations unless users explicitly opt-out.

[JulianH1989]

consider this simple example:

//file fmt_dummy.cpp
#include "fmt_dummy.h"
#include <fmt/format.h>
#include <cstdlib>
#include <string>

void foo()
{
  const std::string str = fmt::format("Int format {}", 42);
}

if you are nod modifying CMAKE_CONFIGURATION_TYPES the generated compile_commands.json will contain 4 entries for the file fmt_dummy.cpp

For Debug and Release you will get something like this

{
  "directory": "/home/user/dummyproject/out/linux_gcc13_analyzer/build",

  "command": "/usr/bin/g++-13 -DBUILD_FMT_MODULE -DENABLE_RUNTIME_ANALYZER -DENABLE_STATIC_ANALYZER -DUSED_CXX_STANDARD=20 -DUSED_C_STANDARD=17 -DCMAKE_INTDIR=\\\"Release\\\" -I/home/user/dummyproject/modules -isystem /home/user/.conan2/p/b/fmt8410f4dc26532/p/include -O3 -DNDEBUG -std=gnu++20 -fsanitize=address,undefined,leak -o modules/FMT_Module/CMakeFiles/fmt_dummy.dir/Release/fmt_dummy.cpp.o -c home/user/dummyproject/modules/FMT_Module/fmt_dummy.cpp",

  "file": "home/user/dummyproject/modules/FMT_Module/fmt_dummy.cpp",

  "output": "home/user/dummyproject/out/linux_gcc13_analyzer/build/modules/FMT_Module/CMakeFiles/fmt_dummy.dir/Release/fmt_dummy.cpp.o"
},

for MinSIzeRel and RelWithDbgInfo you will get something very similar.

but without the -isystem /home/user/.conan2/p/b/fmt8410f4dc26532/p/include
Unfortunately MinSIzeRel and RelWIthDbgInfo are listed after the Debug and Release Config in the compile_commands.json.
CLang-tidy scans the compile_commands.json and removes all duplicated entries. In My case: Only the last entry MinSizeRel was used to analyse the file which did not contain the conan include paths.
This causes clang-tidy to fail.

The same issue occurs when feeding cppcheck the compile_commands.json. Luckily you can tell cppcheck to discard unknown headers, but this did not work for clang tidy.

I get your point: If you are bothered with the increased build time i can only think of using these 2 options

1. overwriting CMAKE_CONFIGURATION_TYPES (this must be done in the actual project though)
2. use a single config generator
3. per default use all 4 config types: Optionally specify via Cache variable Debug and Release if needed

I think you can just take the example from this repo and configure it with ninja multi config. Check your compile_commands.json and you will see that this contains for each file 4 versions. Only debug and release contain the correct conan include paths

[geertbleyen]

Since the default value for the former is the four configurations, it would be overkill to require the 4 configurations for all dependencies. For example, on Linux (where Debug and Release tend to be binary compatible) - users are typically okay using Debug for their own project, but Release for all dependencies (for performance purposes).

For C++, Debug and Release are most definitely not compatible as many std containers have different layouts depending on build configuration. Which is why, for most people that use dependencies with C++ interfaces and that are not header-only, I'd expect the CMAKE_CONFIGURATION_TYPES need to match what conan builds for those configurations?

I guess, it could be fine to take the PR as suggested, as when not defining CONAN_INSTALL_BUILD_CONFIGURATIONS would then result conan's install configs to match with CMAKE_CONFIGURATION_TYPES set by the project.

[jcar87]

Does it work if the project defines the variables

CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL=Release
CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release

?

[geertbleyen]

Does it work if the project defines the variables

CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL=Release
CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release

?

Regarding the CompileCommands.json case that @JulianH1989 mentions, I'm not sure.
But regarding having a separate set of configs for cmake vs conan dependencies, it indeed could still make sense to have both options?

[jcar87]

Debug and Release are most definitely not compatible as many std containers have different layouts depending on build configuration.

This is the case with MSVC STL, but not other vendors.

Which is why, for most people that use dependencies with C++ interfaces and that are not header-only, I'd expect the CMAKE_CONFIGURATION_TYPES need to match what conan builds for those configurations?

As I mentioned earlier, on Linux and macOS is fairly standard to have your own project built on Debug, and your dependencies in Release. In fact, that's how it works when one uses dependencies provided by the system - they are optimized builds.

I'd say - most people don't change the CMAKE_CONFIGURATION_TYPES - they just dont bother using the ones they don't need.

I guess, it could be fine to take the PR as suggested, as when not defining CONAN_INSTALL_BUILD_CONFIGURATIONS would then result conan's install configs to match with CMAKE_CONFIGURATION_TYPES set by the project.

They are different things. Both can be handled by the consumer project in a presets file without much hassle. Merging this PR will cause all users using multi-config generators (Ninja MultiConfig or Visual Studio) to have to build all dependencies in all 4 configurations due to CMake's default behaviour - when it most cases they won't need them. So we're inclined to not change the current behaviour in a way that would cause unnecessary build times for the vast majority of users.

[geertbleyen]

Ok, I concede the point. For my use case I can just define CONAN_INSTALL_BUILD_CONFIGURATIONS to be the same as CMAKE_CONFIGURATION_TYPES and go that route. Since we do build for Windows with MSVC, we do need separate debug and release conan installs anyways.

[JulianH1989]

Hey,
I just tested this

set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL Release)
set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release)

If I add this to my project BEFORE any target is created the compile_commands.json looks good and clangh tidy works.

Regarding this PR:
IMHO most users simply want to consume this provider. They don't want to bother about it's contents.
As a matter of fact the current implementation generates a malformed compile_commands.json. Any down stream tool which consumes the compile_commands.json will possibly break.

So I'd prefer to just use the full CMAKE_CONFIGURATION_TYPES because breaking downstream tools is much worse than only increasing the build time. People can still optimize their build by specifying CONAN_INSTALL_BUILD_CONFIGURATIONS .
(Besides: Once the conan cache is initialized and everything was build it should not take much longer with 4 configuration types for subsequent builds)

If you decide to keeps things as they are, at least put a big warning in the readme that users have to set CONAN_INSTALL_BUILD_CONFIGURATIONS to ${CMAKE_CONFIGURATION_TYPES} or set

set(CMAKE_MAP_IMPORTED_CONFIG_MINSIZEREL Release)
set(CMAKE_MAP_IMPORTED_CONFIG_RELWITHDEBINFO Release)

in their project. Otherwise the compile_commands.json is malformed and downstream tools will break if they do not add this to their project.