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

Jump to bottom

Feature: set CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON #8599

Merged
merged 6 commits into from Mar 23, 2021
Merged

Feature: set CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON #8599

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d38cb57
- set CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
SSE4 Mar 4, 2021
47965cf
- add conf & test
SSE4 Mar 17, 2021
e39d2a2
- remove some debug code
SSE4 Mar 17, 2021
fd15d4f
- fix file name for config
SSE4 Mar 17, 2021
53cc88b
Update base.py
SSE4 Mar 18, 2021
6984540
Update base.py
SSE4 Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions conan/tools/cmake/base.py
View file
Open in desktop
Expand Up @@ -85,6 +85,10 @@ class CMakeToolchainBase(object):
# We are going to adjust automagically many things as requested by Conan
# these are the things done by 'conan_basic_setup()'
set(CMAKE_EXPORT_NO_PACKAGE_REGISTRY ON)
{%- if find_package_prefer_config %}
set(CMAKE_FIND_PACKAGE_PREFER_CONFIG {{ find_package_prefer_config }})
{%- endif %}
#set(CMAKE_FIND_PACKAGE_PREFER_CONFIG ON)
# To support the cmake_find_package generators
{% if cmake_module_path -%}
set(CMAKE_MODULE_PATH {{ cmake_module_path }} ${CMAKE_MODULE_PATH})
Expand Down Expand Up @@ -133,6 +137,8 @@ class CMakeToolchainBase(object):
""")

def __init__(self, conanfile, **kwargs):
import six

self._conanfile = conanfile
self.variables = Variables()
self.preprocessor_definitions = Variables()
Expand All @@ -143,6 +149,15 @@ def __init__(self, conanfile, **kwargs):

self.build_type = None

self.find_package_prefer_config = \
conanfile.conf["tools.cmake.cmaketoolchain"].find_package_prefer_config
if self.find_package_prefer_config is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This is not opting-out, it is opting-in please recall my comment:

Sounds good, lets add a [conf] to opt-out this behavior, I'd suggest something like

Then simplify code, as you said they are always strings, so go for a oneliner:

if self.find_package_prefer_config is not None:
    self.find_package_prefer_config = "OFF" if self.find_package_prefer_config.lower() in ("false", "0", "off") else "ON"

Copy link
Contributor Author

@SSE4 SSE4 Mar 18, 2021 •
edited

Choose a reason for hiding this comment

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

I am not sure I understand. this is opt-out, the behavior matches CMake exactly:

  • conf not defined -> CMAKE_FIND_PACKAGE_PREFER_CONFIG not defined
  • conf=True -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=False -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=OFF

Copy link

Choose a reason for hiding this comment

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

The whole idea was to have it ON by default so inexperienced users using conanfile.txt without any conf sections have a friendly dev env. Advanced users may want to disable it if they so desire.

As a side note, still there are many inexperienced users out there both for Conan and CMake. Doing things that we consider trivial is not easy for them at all. For the last two days, I am remotely helping one of them to enable mp-units support via conanfile.txt even though I thought that my docs are clear: https://mpusz.github.io/units/usage.html#conan-cmake-release. Today I finally saw that person's CMake file and it turned out to be:

image

Copy link
Contributor Author

@SSE4 SSE4 Mar 18, 2021 •
edited

Choose a reason for hiding this comment

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

okay, I added the ON as default again. there was a confusion because initially I implemented ON as default, then others said it should be controlled by conf instead. hopefully the current behavior makes sense:

  • conf not defined -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=True -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=ON
  • conf=False -> CMAKE_FIND_PACKAGE_PREFER_CONFIG=OFF

(although it's not exactly an opt out for the default CMake behavior, it should help for your case)

Copy link

Choose a reason for hiding this comment

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

Actually, in this case, a user correctly applied CONFIG REQUIRED as suggested in my docs. The problem was with the last two lines 😉

Copy link
Member

Choose a reason for hiding this comment

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

@SSE4 my request was:

Sounds good, lets add a [conf] to opt-out this behavior, I'd suggest something like:

A config required to opt-out. That means that by default it is ON, and you need to do something proactively to opt-out of that feature. If you need to define a conf for activating the feature, it is not an opt-out feature, it is an opt-in feature.

if isinstance(self.find_package_prefer_config, six.string_types):
self.find_package_prefer_config = self.find_package_prefer_config == "True"
elif isinstance(self.find_package_prefer_config, six.integer_types):
self.find_package_prefer_config = bool(self.find_package_prefer_config)
self.find_package_prefer_config = "ON" if self.find_package_prefer_config else "OFF"

Copy link
Contributor Author

@SSE4 SSE4 Mar 17, 2021 •
edited

Choose a reason for hiding this comment

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

two notes:

def _get_templates(self):
return {
'toolchain_macros': self._toolchain_macros_tpl,
Expand All @@ -160,6 +175,7 @@ def _get_template_context_data(self):
"cmake_prefix_path": self.cmake_prefix_path,
"cmake_module_path": self.cmake_module_path,
"build_type": self.build_type,
"find_package_prefer_config": self.find_package_prefer_config,
}
return ctxt_toolchain

Expand Down
57 changes: 57 additions & 0 deletions conans/test/functional/toolchains/cmake/test_cmake.py
View file
Open in desktop
Expand Up @@ -516,3 +516,60 @@ def build(self):
client.run("install .")
client.run("build .")
self.assertIn("VALUE OF CONFIG STRING: my new value", client.out)


@pytest.mark.toolchain
@pytest.mark.tool_cmake
class CMakeFindPackagePreferConfigTest(unittest.TestCase):

@parameterized.expand([(True,), (False,)])
def test_prefer_config(self, prefer_config):
conanfile = textwrap.dedent("""
from conans import ConanFile
from conan.tools.cmake import CMake, CMakeToolchain
class App(ConanFile):
settings = "os", "arch", "compiler", "build_type"
exports_sources = "CMakeLists.txt"
def generate(self):
toolchain = CMakeToolchain(self)
toolchain.generate()
def build(self):
cmake = CMake(self)
cmake.configure()
""")

cmakelist = textwrap.dedent("""
cmake_minimum_required(VERSION 3.15)
project(my_project)
message(STATUS "CMAKE_FIND_PACKAGE_PREFER_CONFIG ${CMAKE_FIND_PACKAGE_PREFER_CONFIG}")
find_package(Comandante REQUIRED)
""")

find = textwrap.dedent("""
message(STATUS "using FindComandante.cmake")
""")

config = textwrap.dedent("""
message(STATUS "using Comandante-config.cmake")
""")

profile = textwrap.dedent("""
include(default)
[conf]
tools.cmake.cmaketoolchain:find_package_prefer_config={}
""").format(prefer_config)

client = TestClient()
client.save({"conanfile.py": conanfile,
"CMakeLists.txt": cmakelist,
"FindComandante.cmake": find,
"Comandante-config.cmake": config,
"profile": profile})

client.run("install . --profile profile")
client.run("build .")

if prefer_config:
self.assertIn("using Comandante-config.cmake", client.out)
else:
self.assertIn("using FindComandante.cmake", client.out)