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

[bug] CMakeToolchain does not correctly handle tools.build:*flags for multi-config generators #13920

Closed
DoDoENT opened this issue May 18, 2023 · 3 comments · Fixed by #15654
Closed
Assignees
Milestone

Comments

@DoDoENT
Copy link
Contributor

DoDoENT commented May 18, 2023

Environment details

  • Operating System+version: MacOS Ventura 13.3.1
  • Compiler+version: XCode 14.3
  • Conan version: 2.0.5
  • Python version: 3.11.3

Steps to reproduce

Documentation for CMakeToolchain states that:

  • tools.build:cxxflags list of extra C++ flags that will be appended to CMAKE_CXX_FLAGS_INIT.
  • tools.build:cflags list of extra of pure C flags that will be appended to CMAKE_C_FLAGS_INIT.
  • tools.build:sharedlinkflags list of extra linker flags that will be appended to CMAKE_SHARED_LINKER_FLAGS_INIT.
  • tools.build:exelinkflags list of extra linker flags that will be appended to CMAKE_EXE_LINKER_FLAGS_INIT.

And this is also the observed behavior. However, this is wrong for multi-config generators, such as Xcode, VS, and Ninja-multi, this behavior is wrong. In those cases, variables CMAKE_<LANG>_FLAGS_<CONFIG>_INIT, CMAKE_SHARED_LINKER_FLAGS_<CONFIG>_INIT, and CMAKE_EXE_LINKER_FLAGS_<CONFIG>_INIT should be used instead.

This is problematic when you create a toolchain package that forces different compiler/linker flags depending on the build type (in my case, I enable -flto=thin for release builds and -fsanitize=address for debug builds - I can show you my full toolchain conanfile recipe if needed). In that case, the generated conan_toolchain.cmake contains either string(APPEND CONAN_CXX_FLAGS "-fsanitize=address") or string(APPEND CONAN_CXX_FLAGS "-flto=thin"), depending on whether I invoked conan install ... -s build_type=Debug first or last.

This behavior not only produces non-deterministic builds but, depending on the actual flags in question, results with a project that correctly builds only for configuration that was "last installed".

This is not a big deal when creating packages, but it's very problematic for local development.

I think that CMakeToolchain should in that case behave more similar to cmake_multi generator from V1: generate conan_toolchain_<build_type>.cmake for every conan install ... -s build_type=... invocation and top-level conan_toolchain.cmake should only include conan_toolchain_<build_type>.cmake that are installed (every conan install ... invocation just updates the conan_toolchain.cmake if it already exists - similarly to how it currently does for CMakePresets.json.

I can work around this issue in my own projects by simply clearing the CMAKE_<LANG>_FLAGS and similar variables and using my own cmake logic to reproduce the desired behavior. However, I would be happier if the built-in behavior is correct in the first place.

Logs

No response

@memsharded memsharded self-assigned this May 19, 2023
@memsharded memsharded added this to the 2.0.6 milestone May 19, 2023
@memsharded
Copy link
Member

Hi @DoDoENT

Thanks for your detailed report.
This is indeed an issue. I think we can try to address it in the same conan_toolchain.cmake (I am trying to avoid adding more files), because we already do these things dynamically for multi-config for the MSVC runtime, so the same approach could be done for flags, and append _CONFIG flags correctly. Let's try to improve this for next release 2.0.6 or 2.0.7 (2.0.6 might be soon, because of a regression in components+overrides)

@memsharded memsharded modified the milestones: 2.0.6, 2.0.7 May 24, 2023
@memsharded memsharded modified the milestones: 2.0.7, 2.0.8 Jun 20, 2023
@memsharded memsharded modified the milestones: 2.0.8, 2.0.9 Jul 11, 2023
@memsharded memsharded modified the milestones: 2.0.9, 2.0.10 Jul 19, 2023
@memsharded memsharded modified the milestones: 2.0.10, 2.0.11 Aug 25, 2023
@memsharded memsharded modified the milestones: 2.0.11, 2.0.12 Sep 14, 2023
@czoido czoido modified the milestones: 2.0.12, 2.0.13, 2.0.14 Sep 26, 2023
@czoido czoido modified the milestones: 2.0.14, 2.0.15 Nov 7, 2023
@memsharded memsharded modified the milestones: 2.0.15, 2.1 Dec 18, 2023
@memsharded
Copy link
Member

I am submitting #15654 to fix this, hopefully for next 2.1, feedback welcome!

@memsharded
Copy link
Member

Merged #15654 for next 2.1

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

Successfully merging a pull request may close this issue.

3 participants