only warn in CMake if build_type is empty and is_multi configuration #14349

thorsten-klein · 2023-07-21T12:41:39Z

Changelog: (Feature | Fix | Bugfix): Remove error output in case of non existent build_type setting

If there is no build_type setting existing, cmake will throw an exception during CMake build(). Therefore we must explicitly speciify the build_type argument.
Unfortunately we get some ugly error output when using this:

    def build(self):
        cmake = CMake(self)
        cmake.configure()
        cmake.build(build_type=build_type)

output:

ERROR: Don't specify 'build_type' at build time for single-config build systems

The error output should only happen if it is is_multi, since bt is only used then.

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.

CLAassistant · 2023-07-21T12:41:50Z

All committers have signed the CLA.

memsharded

Thanks very much for your contribution @thorsten-klein

CMake explicitly recommends defining a build-type, specially for single-config configurations:
https://cmake.org/cmake/help/latest/manual/cmake-buildsystem.7.html#default-and-custom-configurations

For single-config generators, the configuration is specified with the CMAKE_BUILD_TYPE variable at configure time and cannot be changed at build time. The default value will often be none of the above standard configurations and will instead be an empty string. A common misunderstanding is that this is the same as Debug, but that is not the case.
Users should always explicitly specify the build type instead to avoid this common problem.

The protection in Conan CMake helper is implementing this guard, requiring the build_type to be defined, for sure for multi-config, but this protection is also necessary for single-config scenarios.

I'd like to understand better this proposal, if the reason is to build packages without build_type against CMake recommendations, and what is the use case. I also assume that this means that you are not using CMakeDeps at all, because with this change you should have hit that issue too. Could you please clarify?

memsharded · 2023-09-18T15:16:11Z

conan/tools/cmake/cmake.py

@@ -108,7 +108,7 @@ def _build(self, build_type=None, target=None, cli_args=None, build_tool_args=No
                                         "single-config build systems")

        bt = build_type or self._conanfile.settings.get_safe("build_type")
-        if not bt:
+        if is_multi and not bt:


This protection for not defining the build_type was in place, necessary for CMake integration, because if build_type is not defined, it later fails in CMakeDeps with either a message like

Please, set the CMAKE_BUILD_TYPE variable when calling to CMake "
"adding the '-DCMAKE_BUILD_TYPE=<build_type>' argument

Or directly missing dependencies.

To reproduce this, using a standard conan new cmake_lib -d name=pkg -d version=0.1 and conan create . -pr=myprofile with a profile that doesn't define build_type is enough.

We get this error message without using CMakeDeps generator. I will create a unit test for this

It is only that we get some error output in log, but the build succeeds. I will create some test for it to show you

Sure, it will be good to have some tests. Please add consuming the package with test_package, for example.
Still the above feedback would be relevant: why going against CMake and not defining the build_type? What is the use case? Thanks!

The setting build_type we want to ignore, so we want to always enforce to build Release

Ok, this is very important. You don't want to build something without build_type. You want to build it in Release build-type. This is a fresh perspective that doesn't goes against CMake recommendations, I'll have a new look with this in mind.

There is a recommended flow, that we also implement in ConanCenter:

Leave the build_type defined

This will make sure that everything that needs build_type will work. From CMakeDeps, to the cmake_layout

Remove the build_type in the package_id() method, so usage of the package becomes independent from build_type.

Define build_type=Release in the profile to build this package, create it and upload it. From now all packages can tool_require it irrespective of the build_type

There is a further insight for this. Using the 2 profiles "build" and "host", it is no longer an issue or necessary to force or remove the build_type from tool_requires, as the "build" context will almost always use the build_type=Release naturally. This is the most recommended approach, it is easy, built-in, doesn't require changes in recipe and is well tested and used in production by many users.

Ok, found the gap.
You are right that it can make sense to not raise the exception for that case, and allow to force the Release in recipes.
However, it might make sense to have to define the recipe with explicit behavior like:

def layout(self): self.folders.build = "build" def generate(self): tc = CMakeToolchain(self) if self.settings.compiler != "msvc": tc.cache_variables["CMAKE_BUILD_TYPE"] = "Release" tc.generate() def build(self): cmake = CMake(self) cmake.configure() build_type = "Release" if self.settings.compiler == "msvc" else None cmake.build(build_type=build_type) def package(self): cmake = CMake(self) build_type = "Release" if self.settings.compiler == "msvc" else None cmake.install(build_type=build_type)

and then apply your change so it doesn't error. But not necessarily changing the error => warning message and still get the warning with would be still not great. It is better to not create the warning in the first place. That means that we might want also to make public some is_multi_configuration() interface, to simplify recipes and make them possible to do this.

I'll try to move this forward tomorrow, thanks for the feedback

In any case, please read the comment above, all this would be unnecessary with the "build" profile defining build_type=Release

Thank you very much :-)

So summary from my POV:

it is very unintuitive that cmake.build(build_type="Release") does not have any effect. So it totally makes sense to warn users about this behavior in a WARN/ERR message.

conan should raise exception if build type is not set and it is a multi configuration build

conan should WARN/ERR users if build_type is empty

In case of settings = "os", "compiler", "arch" the CMAKE_BUILD_TYPE is empty, even if build_type setting is specified in the profile. conan could set CMAKE_BUILD_TYPE=Release by default (at least for single configuration --> proposal see this PR)

memsharded · 2023-09-18T20:46:05Z

conans/test/integration/toolchains/cmake/test_cmake.py

+            generators = "CMakeToolchain"
+            def build(self):
+                cmake = CMake(self)
+                cmake.configure()


This build, in single-config build systems is not using build_type at all. What is the purpose of doing a CMake build without build_type, which is explicitly discouraged by CMake?

How can I specify a build type then in this case a profile does not specify any build_type?
Using cmake.build(build_type="Release") does not have any effect (see test above).
Shall I use tc.cache_variables?

Why not defining the build_type in the profile?

Refering to settings.yml the build_type MAY be empty. So we must cover this case as well.

memsharded · 2023-09-18T20:54:17Z

conans/test/integration/toolchains/cmake/test_cmake.py

@@ -31,3 +31,52 @@ def run(self, *args, **kwargs):
    assert "-something" in client.out
    assert "--testverbose" in client.out
    assert "-testok" in client.out
+
+def test_build_type_single_config_warn():


These tests passes in PRs because they run only in Linux in PRs. After merge, they will run in Windows too and fail (these tests fail in Windows due to multi-config VS). It would be better to move them to "functional" folder, so they run in all platforms also in PRs.

I think the best is to do actual builds, with fully functional examples in the "functional" folder, building libraries with full test_package functionality. This will make sure things are working.

I have created some tests for my all my use cases under functional. I always set build_type via -s build_type=Debug.

memsharded · 2023-09-25T14:12:50Z

Quick follow up, I'm working in #14780, trying to publish the necessary public interfaces for the use case in question, plus making sure the scenario works covering it with tests. Hopefully for next 2.0.12

thorsten-klein · 2023-09-25T14:19:01Z

Nice, thank you. Then I will close this PR.
Do you backport the 2.x changes to 1.x as well, so that the CMake helper behaves in the same way with conan 1.x?

memsharded · 2023-09-25T21:26:48Z

The backport is a bit more challenging, higher risks to break something, let's see how it goes with the PR to 2.0 and we can analyze it.

In any case, there is the pending question, why not the standard flow that most of Conan users are doing? The one recommended above, just leave the build_type setting declared, and define the build_type=Release in your "build" profile. That will already guarantee that the tool_requires are built and used in Release mode, no need of further features or changes, recipes are shorter and easier, and it already works out of the box with Conan 1.X and Conan 2.0?

thorsten-klein · 2023-09-26T05:56:33Z

Ok, I understand 👍🏻

Yes, I have started to migrate to the "standard flow".

I ensure that settings='build_type' is present in each conanfile.py
In my profiles I ensure that build_type is always set (build context: Release, host context: Debug|Release)
I do not specify cmake.build(build_type="Release") since it does not have any effect

thorsten-klein force-pushed the warn-build_type branch from 6fd0c6f to 7a0ae96 Compare July 21, 2023 12:42

memsharded reviewed Sep 18, 2023

View reviewed changes

accept empty build_type in case of CMake single configuration

722382d

thorsten-klein force-pushed the warn-build_type branch from 7a0ae96 to 722382d Compare September 18, 2023 19:32

thorsten-klein mentioned this pull request Sep 18, 2023

Conan 1.x: Do not output error in case of empty build_type #14771

Closed

5 tasks

memsharded reviewed Sep 18, 2023

View reviewed changes

set CMAKE_BUILD_TYPE in test

5c4cf44

thorsten-klein force-pushed the warn-build_type branch 2 times, most recently from c750d36 to 8aea393 Compare September 18, 2023 21:49

move test to functional

dd2edc1

thorsten-klein force-pushed the warn-build_type branch from 8aea393 to dd2edc1 Compare September 18, 2023 21:52

create parameterized test

6cfe578

thorsten-klein force-pushed the warn-build_type branch from b83149e to 6cfe578 Compare September 18, 2023 22:00

set CMAKE_BUILD_TYPE=Release by default

daa3bf5

memsharded mentioned this pull request Sep 25, 2023

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

Merged

thorsten-klein closed this Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only warn in CMake if build_type is empty and is_multi configuration #14349

only warn in CMake if build_type is empty and is_multi configuration #14349

thorsten-klein commented Jul 21, 2023 •

edited

Loading

CLAassistant commented Jul 21, 2023 •

edited

Loading

memsharded left a comment

memsharded Sep 18, 2023

memsharded Sep 18, 2023

thorsten-klein Sep 18, 2023

thorsten-klein Sep 18, 2023

memsharded Sep 18, 2023

memsharded Sep 18, 2023

memsharded Sep 18, 2023

memsharded Sep 18, 2023

memsharded Sep 18, 2023

thorsten-klein Sep 18, 2023

memsharded Sep 18, 2023

thorsten-klein Sep 18, 2023

memsharded Sep 18, 2023

thorsten-klein Sep 18, 2023

memsharded Sep 18, 2023

memsharded Sep 18, 2023

thorsten-klein Sep 18, 2023 •

edited

Loading

memsharded commented Sep 25, 2023

thorsten-klein commented Sep 25, 2023

memsharded commented Sep 25, 2023

thorsten-klein commented Sep 26, 2023

only warn in CMake if build_type is empty and is_multi configuration #14349

only warn in CMake if build_type is empty and is_multi configuration #14349

Conversation

thorsten-klein commented Jul 21, 2023 • edited Loading

CLAassistant commented Jul 21, 2023 • edited Loading

memsharded left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorsten-klein Sep 18, 2023 • edited Loading

Choose a reason for hiding this comment

memsharded commented Sep 25, 2023

thorsten-klein commented Sep 25, 2023

memsharded commented Sep 25, 2023

thorsten-klein commented Sep 26, 2023

thorsten-klein commented Jul 21, 2023 •

edited

Loading

CLAassistant commented Jul 21, 2023 •

edited

Loading

thorsten-klein Sep 18, 2023 •

edited

Loading