proposed changes #51
proposed changes #51
Conversation
|
|
||
| cmake = """ | ||
| cmake_minimum_required(VERSION 3.15) | ||
| project(pkg LANGUAGES NONE) | ||
| message(STATUS "CMAKE_BUILD_TYPE: '${CMAKE_BUILD_TYPE}'") |
There was a problem hiding this comment.
This doesn't seem a valid check. CMAKE_BUILD_TYPE must be not defined for all multi-config generators, irrespective of all the inputs, toolchain, command line, etc.
| @@ -100,7 +100,8 @@ def _configure_preset(conanfile, generator, cache_variables, toolchain_file, mul | |||
| if preset_prefix: | |||
| name = f"{preset_prefix}-{name}" | |||
| if not multiconfig and build_type: | |||
| cache_variables["CMAKE_BUILD_TYPE"] = build_type | |||
| if not "CMAKE_BUILD_TYPE" in cache_variables: | |||
There was a problem hiding this comment.
This wouldnt be a fix for the test, because the test doesn't use the presets at all, seems unrelated. It is true that command line usage of cmake --preset might require other changes that my PR didn't consider, but for the test below, it is unrelated.
| @pytest.mark.parametrize("conanfile_settings", | ||
| ['"os", "compiler", "arch"', | ||
| '"os", "compiler", "arch", "build_type"' | ||
| ]) | ||
| @pytest.mark.parametrize("tc_cache_variables", | ||
| [True, False]) |
There was a problem hiding this comment.
This is testing too different use cases. Even if the code of the test is similar enough to do a parameterized test, the use cases can be quite different.
The use case I defined in my PR was: "The conanfile doesn't define a build_type setting, and needs to define a hardcoded one so the build creates that configuration.
There would be other use cases, like why defining build_type in the settings, and then ignoring it, overriding it in the recipe? This seems more an ill-formed case, creating 2 different package_ids with the same binaries.
It would be much better to provide 4 different tests, each test representing a different use case, and what is the user story that such test is represeting.
Changelog: (Feature | Fix | Bugfix): Describe here your pull request
Docs: https://github.com/conan-io/docs/pull/XXXX
developbranch, documenting this one.Note: By default this PR will skip the slower tests and will use a limited set of python versions. Check here how to increase the testing level by writing some tags in the current PR body text.