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

allow integers as CMakeToolchain.preprocessor_definitions #8645

Merged
merged 5 commits into from Mar 21, 2021
Merged

allow integers as CMakeToolchain.preprocessor_definitions #8645

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6fbc4fe
allow integers as CMakeToolchain.preprocessor_definitions
memsharded Mar 14, 2021
c7203ac
fix test
memsharded Mar 14, 2021
e9163a7
fix test
memsharded Mar 14, 2021
86c5077
ordered dicts
memsharded Mar 15, 2021
416b07f
Merge branch 'develop' into feature/toolchain_cmake_preprocesor_int
memsharded 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
28 changes: 20 additions & 8 deletions conan/tools/cmake/base.py
View file
Open in desktop
@@ -1,6 +1,7 @@
import textwrap
from collections import OrderedDict, defaultdict
from collections import OrderedDict

import six
from jinja2 import DictLoader, Environment

from conans.util.files import save
Expand All @@ -17,17 +18,26 @@ def __getattribute__(self, config):
try:
return super(Variables, self).__getattribute__(config)
except AttributeError:
return self._configuration_types.setdefault(config, dict())
return self._configuration_types.setdefault(config, OrderedDict())

@property
def configuration_types(self):
# Reverse index for the configuration_types variables
ret = defaultdict(list)
ret = OrderedDict()
for conf, definitions in self._configuration_types.items():
for k, v in definitions.items():
ret[k].append((conf, v))
ret.setdefault(k, []).append((conf, v))
return ret

def quote_preprocessor_strings(self):
for key, var in self.items():
if isinstance(var, six.string_types):
self[key] = '"{}"'.format(var)
for config, data in self._configuration_types.items():
for key, var in data.items():
if isinstance(var, six.string_types):
data[key] = '"{}"'.format(var)


class CMakeToolchainBase(object):
filename = "conan_toolchain.cmake"
Expand All @@ -38,7 +48,7 @@ class CMakeToolchainBase(object):
{%- set genexpr = namespace(str='') %}
{%- for conf, value in values -%}
{%- set genexpr.str = genexpr.str +
'$<IF:$<CONFIG:' + conf + '>,"' + value|string + '",' %}
'$<IF:$<CONFIG:' + conf + '>,' + value|string + ',' %}
{%- if loop.last %}{% set genexpr.str = genexpr.str + '""' -%}{%- endif -%}
{%- endfor -%}
{% for i in range(values|count) %}{%- set genexpr.str = genexpr.str + '>' %}
Expand Down Expand Up @@ -95,8 +105,8 @@ class CMakeToolchainBase(object):
{% endblock %}

{% if shared_libs -%}
message(STATUS "Conan toolchain: Setting BUILD_SHARED_LIBS= {{ shared_libs }}")
set(BUILD_SHARED_LIBS {{ shared_libs }})
message(STATUS "Conan toolchain: Setting BUILD_SHARED_LIBS= {{ shared_libs }}")
set(BUILD_SHARED_LIBS {{ shared_libs }})
{%- endif %}

{% if parallel -%}
Expand Down Expand Up @@ -125,7 +135,7 @@ class CMakeToolchainBase(object):
# Preprocessor definitions
{% for it, value in preprocessor_definitions.items() -%}
# add_compile_definitions only works in cmake >= 3.12
add_definitions(-D{{ it }}="{{ value }}")
add_definitions(-D{{ it }}={{ value }})
{%- endfor %}
# Preprocessor definitions per configuration
{{ toolchain_macros.iterate_configs(preprocessor_definitions_config,
Expand All @@ -152,6 +162,8 @@ def _get_templates(self):
def _get_template_context_data(self):
""" Returns dict, the context for the '_template_toolchain'
"""
self.preprocessor_definitions.quote_preprocessor_strings()

ctxt_toolchain = {
"variables": self.variables,
"variables_config": self.variables.configuration_types,
Expand Down
23 changes: 17 additions & 6 deletions conans/test/functional/toolchains/cmake/test_cmake.py
View file
Open in desktop
Expand Up @@ -36,8 +36,11 @@ def generate(self):
tc.variables.debug["MYVAR2_CONFIG"] = "MYVAR2_DEBUG"
tc.variables.release["MYVAR2_CONFIG"] = "MYVAR2_RELEASE"
tc.preprocessor_definitions["MYDEFINE"] = "MYDEF_VALUE"
tc.preprocessor_definitions["MYDEFINEINT"] = 42
tc.preprocessor_definitions.debug["MYDEFINE_CONFIG"] = "MYDEF_DEBUG"
tc.preprocessor_definitions.release["MYDEFINE_CONFIG"] = "MYDEF_RELEASE"
tc.preprocessor_definitions.debug["MYDEFINEINT_CONFIG"] = 421
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this? I though you planned to remove that

Copy link
Member Author

Choose a reason for hiding this comment

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

I have not yet fully decided, on one hand it is clear there is demand, on the other, it could be a pure build-system thing, and Conan agnostic about it. In the meantime, it totally makes sense that users want to provide integer preprocessor definitions, as they are very common. Maybe @guiserle could elaborate a bit further about their use case, and why such preprocessor definitions are not integrated in the build system scripts directly.

Copy link

Choose a reason for hiding this comment

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

First of all, thanks a lot for considering my request.

My use case is to define configuration values for libraries such as FreeRTOS or MCU SDK packages. Currently we define conan options for such values and pass those values to cmake on conan build() method. However that means for every configuration value (and these libraries have lots of them) we need to add logic on both conanfile.py and cmake files.

On top of that we are using Clion as IDE and we need to add these cmake variables (and others such as version numbers) on the IDE project settings manually. I was thinking about setting these values directly on generate() method so they would all be included in toolchain file and it would only be necessary to add the toolchain as value for CMAKE_TOOLCHAIN_FILE.

If that is not the intended use case for the generate method and this should be implemented on another way, I will understand it. Also I have considered the implementation of a custom generator for my particular use case if necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to merge this, seems a valid use case, thanks for the feedback.

Just beware that self.options are also always strings, even if you defined them as option: [1, 2, 3], internally, they are always strings. So you might need to convert it to ints, if you use it from there like cmake.preprocessor_defintions["MYDEF"] = int(self.options.myoption)

Copy link

Choose a reason for hiding this comment

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

That's great. Thanks for taking my use case into account!

tc.preprocessor_definitions.release["MYDEFINEINT_CONFIG"] = 422
tc.generate()

def build(self):
Expand All @@ -48,7 +51,8 @@ def build(self):

lib_h = gen_function_h(name="app")
lib_cpp = gen_function_cpp(name="app", msg="App", includes=["hello"], calls=["hello"],
preprocessor=["MYVAR", "MYVAR_CONFIG", "MYDEFINE", "MYDEFINE_CONFIG"])
preprocessor=["MYVAR", "MYVAR_CONFIG", "MYDEFINE", "MYDEFINE_CONFIG",
"MYDEFINEINT", "MYDEFINEINT_CONFIG"])
main = gen_function_cpp(name="main", includes=["app"], calls=["app"])

cmakelist = textwrap.dedent("""
Expand Down Expand Up @@ -83,7 +87,7 @@ def build(self):
add_library(app_lib app_lib.cpp)
target_link_libraries(app_lib PRIVATE hello::hello)
target_compile_definitions(app_lib PRIVATE MYVAR="${MYVAR}")
target_compile_definitions(app_lib PRIVATE MYVAR_CONFIG=${MYVAR_CONFIG})
target_compile_definitions(app_lib PRIVATE MYVAR_CONFIG="${MYVAR_CONFIG}")
add_executable(app app.cpp)
target_link_libraries(app PRIVATE app_lib)
""")
Expand Down Expand Up @@ -130,7 +134,9 @@ def _run_build(self, settings=None, options=None):
def _modify_code(self):
lib_cpp = gen_function_cpp(name="app", msg="AppImproved", includes=["hello"],
calls=["hello"], preprocessor=["MYVAR", "MYVAR_CONFIG",
"MYDEFINE", "MYDEFINE_CONFIG"])
"MYDEFINE", "MYDEFINE_CONFIG",
"MYDEFINEINT",
"MYDEFINEINT_CONFIG"])
self.client.save({"app_lib.cpp": lib_cpp})

content = self.client.load("CMakeLists.txt")
Expand Down Expand Up @@ -158,6 +164,9 @@ def _run_app(self, build_type, bin_folder=False, msg="App", dyld_path=None):
self.assertIn("MYVAR_CONFIG: MYVAR_%s" % build_type.upper(), self.client.out)
self.assertIn("MYDEFINE: MYDEF_VALUE", self.client.out)
self.assertIn("MYDEFINE_CONFIG: MYDEF_%s" % build_type.upper(), self.client.out)
self.assertIn("MYDEFINEINT: 42", self.client.out)
self.assertIn("MYDEFINEINT_CONFIG: {}".format(421 if build_type == "Debug" else 422),
self.client.out)


@pytest.mark.skipif(platform.system() != "Windows", reason="Only for windows")
Expand Down Expand Up @@ -236,6 +245,7 @@ def _verify_out(marker=">>"):
"MYVAR_CONFIG": "MYVAR_RELEASE",
"MYDEFINE": "MYDEF_VALUE",
"MYDEFINE_CONFIG": "MYDEF_RELEASE"

})
self._run_app("Debug", bin_folder=True)
check_exe_run(self.client.out, "main", "msvc", visual_version, "Debug", arch, cppstd,
Expand Down Expand Up @@ -336,9 +346,10 @@ def test_toolchain_linux(self, build_type, cppstd, arch, libcxx, shared):
pic_str = "" if shared else "ON"
arch_str = "-m32" if arch == "x86" else "-m64"
cxx11_abi_str = "1" if libcxx == "libstdc++11" else "0"
defines = '_GLIBCXX_USE_CXX11_ABI=%s;MYDEFINE="MYDEF_VALUE";'\
'MYDEFINE_CONFIG=$<IF:$<CONFIG:debug>,"MYDEF_DEBUG",'\
'$<IF:$<CONFIG:release>,"MYDEF_RELEASE","">>' % cxx11_abi_str
defines = '_GLIBCXX_USE_CXX11_ABI=%s;MYDEFINE="MYDEF_VALUE";MYDEFINEINT=42;'\
'MYDEFINE_CONFIG=$<IF:$<CONFIG:debug>,"MYDEF_DEBUG",$<IF:$<CONFIG:release>,'\
'"MYDEF_RELEASE","">>;MYDEFINEINT_CONFIG=$<IF:$<CONFIG:debug>,421,'\
'$<IF:$<CONFIG:release>,422,"">>' % cxx11_abi_str
vals = {"CMAKE_CXX_STANDARD": "14",
"CMAKE_CXX_EXTENSIONS": extensions_str,
"CMAKE_BUILD_TYPE": build_type,
Expand Down