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.

Sign up for GitHub

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

proposed changes #51

Closed
wants to merge 1 commit into from
Closed

proposed changes #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion conan/tools/cmake/presets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cache_variables["CMAKE_BUILD_TYPE"] = build_type
ret = {
"name": name,
"displayName": "'{}' config".format(name),
Expand Down
40 changes: 34 additions & 6 deletions conans/test/functional/toolchains/cmake/test_cmake_toolchain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1354,7 +1354,13 @@ def build(self):
assert "-- MYVAR1 MYVALUE1!!" in client.out


def test_no_build_type():
@pytest.mark.parametrize("conanfile_settings",
['"os", "compiler", "arch"',
'"os", "compiler", "arch", "build_type"'
])
@pytest.mark.parametrize("tc_cache_variables",
[True, False])
Comment on lines +1357 to +1362
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def test_build_type(conanfile_settings, tc_cache_variables):
client = TestClient()

conanfile = textwrap.dedent("""
Expand All @@ -1364,16 +1370,16 @@ def test_no_build_type():
class AppConan(ConanFile):
name = "pkg"
version = "1.0"
settings = "os", "compiler", "arch"
settings = {}
exports_sources = "CMakeLists.txt"

def layout(self):
self.folders.build = "build"

def generate(self):
tc = CMakeToolchain(self)
if not tc.is_multi_configuration:
tc.cache_variables["CMAKE_BUILD_TYPE"] = "Release"
if {} and not tc.is_multi_configuration:
tc.cache_variables["CMAKE_BUILD_TYPE"] = "MinSizeRel"
tc.generate()

def build(self):
Expand All @@ -1386,14 +1392,36 @@ def package(self):
cmake = CMake(self)
build_type = "Release" if cmake.is_multi_configuration else None
cmake.install(build_type=build_type)
""")
""".format(conanfile_settings, tc_cache_variables))

cmake = """
cmake_minimum_required(VERSION 3.15)
project(pkg LANGUAGES NONE)
message(STATUS "CMAKE_BUILD_TYPE: '${CMAKE_BUILD_TYPE}'")
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""

client.save({"conanfile.py": conanfile,
"CMakeLists.txt": cmake})
client.run("create .")

# priorities for value of CMAKE_BUILD_TYPE:
# 1. tc.cache_variables["CMAKE_BUILD_TYPE"] if specified
# 2. 'build_type' if build_type is considered under settings
# 3. None (empty)

client.run("create .") # same as: -s build_type=Release
assert "Don't specify 'build_type' at build time" not in client.out
if tc_cache_variables:
assert "CMAKE_BUILD_TYPE: 'MinSizeRel'" in client.out
elif "build_type" in conanfile_settings:
assert "CMAKE_BUILD_TYPE: 'Release'" in client.out
else:
assert "CMAKE_BUILD_TYPE: ''" in client.out

client.run("create . -s build_type=Debug")
assert "Don't specify 'build_type' at build time" not in client.out
if tc_cache_variables:
assert "CMAKE_BUILD_TYPE: 'MinSizeRel'" in client.out
elif "build_type" in conanfile_settings:
assert "CMAKE_BUILD_TYPE: 'Debug'" in client.out
else:
assert "CMAKE_BUILD_TYPE: ''" in client.out