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

Fix cmake build --config XXXX for multi-config with hardcoded configurations #14780

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Sep 19, 2023

Changelog: Fix: Allow cmake.build(build_type="Release") for recipes built with multi-config systems but without build_type setting.
Docs: Omit

Investigation on #14349

@thorsten-klein
Copy link
Contributor

thorsten-klein commented Sep 26, 2023

👍🏻

Note 1:

It seems that in presets.py the CMAKE_BUILD_TYPE of the toolchain is always overwritten by the value of the profile setting.
Is this behavior intended?

Note 2:
There are currently 2 possible places within conanfile.py where the build_type / CMAKE_BUILD_TYPE can defined.

  1. tc.cache_variables["CMAKE_BUILD_TYPE"] = "MinSizeRel"
  2. cmake.build(build_type="Release")
    The second one only has some effect if is_multi_configuration. This might be confusing for users.

My expected behavior:
priorities for value of CMAKE_BUILD_TYPE:

  1. tc.cache_variables["CMAKE_BUILD_TYPE"]
  2. 'build_type' if build_type is considered under settings
  3. None (empty)

*Proposed changes:
I have extended your test in order to reproduce & align to my expected behavior:
memsharded/conan@fix/cmake_build_configs...thorsten-klein:conan:fix/cmake_build_configs

@memsharded
Copy link
Member Author

I have had a look to your code in your branch extending the test and your comment:

My expected behavior:
priorities for value of CMAKE_BUILD_TYPE:

tc.cache_variables["CMAKE_BUILD_TYPE"]
'build_type' if build_type is considered under settings

I am afraid this is against Conan design. The behavior for Conan is that downstream/consumer definitions should always have precedence over recipes definition. In this case, the profile build_type, if specified, should be applied to recipes, as long as recipes depend on it settings = ..., "build_type". What is not correct is that the profile defines build_type=Release, this is also stored in the conaninfo.txt and implies a package_id, but being changed under the hood so the package binary is actually using a different build-type like MinSizeRel without Conan being aware of it in the binary representation. This cannot happen.

@memsharded memsharded marked this pull request as ready for review November 5, 2023 23:44
@memsharded memsharded marked this pull request as draft November 6, 2023 09:14
@czoido czoido modified the milestones: 2.0.14, 2.0.15 Nov 7, 2023
@memsharded
Copy link
Member Author

I see there are still some unknowns, I have done a review in memsharded#51 (review)

@memsharded memsharded modified the milestones: 2.0.15, 2.1 Dec 17, 2023
@memsharded memsharded marked this pull request as ready for review February 14, 2024 11:20
@czoido czoido changed the base branch from release/2.0 to develop2 February 14, 2024 13:39
@czoido czoido merged commit 2111d5a into conan-io:develop2 Feb 15, 2024
2 checks passed
@memsharded memsharded deleted the fix/cmake_build_configs branch February 15, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants