In CMake, support declining items depending on the configuration used #13071

mmomtchev · 2024-04-11T11:29:46Z

This adds support for conan CMake config files:

it centralizes the deduction of CMAKE_BUILD_TYPE which is not written to the trace file
it adds support for config generator expressions:
```
$<$<CONFIG:Debug>:DEBUG_MODE>
```
it centralizes and fixes the handling of IMPORTED_LOCATION_<CONFIG> directives

test cases/cmake/12 generator expressions/meson.build

mesonbuild/cmake/generator.py

mesonbuild/cmake/tracetargets.py

…n on Windows)

joukewitteveen · 2024-04-18T06:24:58Z

mesonbuild/cmake/interpreter.py

+            mlog.debug('CMake build type set from the meson build type')
+            cmake_args += [f'-DCMAKE_BUILD_TYPE={self.build_type}']


Wouldn't this set CMAKE_BUILD_TYPE to "plain" in case of an unsupported build type? self.build_type falls back to the Meson build type if it isn't in the map.

Some CMake projects have a default build type they use in case no build type is provided. I don't think we should break that by setting some weird CMAKE_BUILD_TYPE.

Normally, with CMake, the CMAKE_BUILD_TYPE is considered mandatory according to their latest recommendations.

The default in CMake, unless the project has defined something else, is an empty string, which can lead to all type of problems.

I can skip setting the build type if it is not in the buildtype_map - but this would have an effect only for projects where the meson project uses an unusual build type naming scheme. I do not know beforehand what is the CMake naming scheme.

I wonder if I will be making more or less cases work if the default one is Debug/Release according to the meson build type.

You can still set it to an empty string if you want to force the project's default.

@jpakkane determines the vision of Meson, but if it were me, I would greatly prefer to interfere as little as possible with external tools (i.e. CMake). I would trust the subproject developers in setting the appropriate defaults for their projects more than any blanket approach implemented in Meson. Put differently, I prefer Meson to implement the simplest logic possible to enable correct building (but not any simpler). The proposed build type logic feels both too invasive and too fragile to me.

dcbaker

There's a bunch of fixups for previous patches later in the series, those should be squashed back into the patch that introduced the issue. I also have a few inline comments for you

dcbaker · 2024-04-18T17:27:31Z

mesonbuild/cmake/interpreter.py

+        if any(arg.startswith('-DCMAKE_BUILD_TYPE=') for arg in cmake_args):
+            # Allow to override the CMAKE_BUILD_TYPE environment variable
+            mlog.debug('CMake build type explicitly set')
+            cmake_build_type = next(arg for arg in cmake_args if arg.startswith('-DCMAKE_BUILD_TYPE=')).split('=')[1]


There's a helper in utils.universal called first that does the same thing, but is a little easier to read (IMHO), even though it is rather filter like.

dcbaker · 2024-04-18T17:28:06Z

mesonbuild/cmake/generator.py

@@ -134,7 +134,7 @@ def target_file(arg: str) -> str:

        # Configurations (Debug, Release...)
        # CMake is case-insensitive
-        'CONFIG': lambda x: '1' if x.upper() == trace.env.coredata.get_option(OptionKey('buildtype')).upper() else '0'
+        'CONFIG': lambda x: '1' if x.upper() == T.cast('str', trace.env.coredata.get_option(OptionKey('buildtype'))).upper() else '0'


Could you squash the first three patches together? They're one logical change

You do not intend to squash on merge?

dcbaker · 2024-04-18T17:28:41Z

mesonbuild/cmake/tracetargets.py

@@ -54,7 +55,7 @@ def get_config_declined_property(target: CMakeTarget,
    cfg = trace.build_type.upper() if trace.build_type else None
    imported_cfgs: T.List[str] = []
    if f'MAP_IMPORTED_CONFIG_{cfg}' in target.properties:
-        cfg = target.properties[f'MAP_IMPORTED_CONFIG_{cfg}'].upper()
+        cfg = target.properties[f'MAP_IMPORTED_CONFIG_{cfg}'][0].upper()


This doesn't seem like a mypy issue?

No, it is not and I even added a unit test for this case in the next commit.

dcbaker · 2024-04-18T17:29:44Z

mesonbuild/cmake/traceparser.py

@@ -84,11 +84,12 @@ def __init__(self, name: str) -> None:
        self.working_dir: T.Optional[Path] = None

 class CMakeTraceParser:
-    def __init__(self, cmake_version: str, build_dir: Path, env: 'Environment', permissive: bool = True) -> None:
+    def __init__(self, cmake_version: str, build_dir: Path, env: 'Environment', build_type: str = None, permissive: bool = True) -> None:


This breaks the strict-optional rule that we want to turn on later, so build_type: T.Optional[str] = None

dcbaker · 2024-04-18T17:30:23Z

mesonbuild/cmake/tracetargets.py

+    cfg = trace.build_type.upper() if trace.build_type else None
+    imported_cfgs: T.List[str] = []
+    if f'MAP_IMPORTED_CONFIG_{cfg}' in target.properties:
+        cfg = target.properties[f'MAP_IMPORTED_CONFIG_{cfg}'].upper()


Didn't this get changed to target.properties[...][0].upper() in the last patch?

Yes, it should be [0], it is an array, there is a test for this case now

dcbaker · 2024-04-18T18:22:49Z

mesonbuild/cmake/tracetargets.py

+            if cmake_is_debug(trace.env):
+                if 'DEBUG' in imported_cfgs:
+                    cfg = 'DEBUG'
+                elif 'RELEASE' in imported_cfgs:
+                    cfg = 'RELEASE'
+            else:
+                if 'RELEASE' in imported_cfgs:
+                    cfg = 'RELEASE'


I think we can simplify this to:

if cmake_is_debug(trace.env) and 'DEBUG' in imported_cfgs: cfg = 'DEBUG' else: cfg = 'RELEASE'

This is the previous author's version, I simplified it

dcbaker · 2024-04-18T18:23:52Z

mesonbuild/cmake/tracetargets.py

@@ -12,7 +11,7 @@
 import typing as T

 if T.TYPE_CHECKING:
-    from .traceparser import CMakeTraceParser
+    from .traceparser import CMakeTraceParser, CMakeTarget


Should this be squashed into a previous patch?

mmomtchev requested review from mensinda and jpakkane as code owners April 11, 2024 11:29

dcbaker added the module:cmake Issues related to the cmake module, including cmake.subproject label Apr 11, 2024

dcbaker requested changes Apr 11, 2024

View reviewed changes

test cases/cmake/12 generator expressions/meson.build Outdated Show resolved Hide resolved

mesonbuild/cmake/generator.py Outdated Show resolved Hide resolved

mmomtchev force-pushed the generator-expression-config branch from 61e14be to c42d2f9 Compare April 12, 2024 09:49

github-advanced-security bot found potential problems Apr 15, 2024

View reviewed changes

mesonbuild/cmake/generator.py Fixed Show fixed Hide fixed

mesonbuild/cmake/tracetargets.py Fixed Show fixed Hide fixed

mmomtchev changed the title ~~support CMake $<CONFIG> generator expressions~~ In CMake, support declining items depending on the configuration used Apr 15, 2024

mmomtchev added 9 commits April 15, 2024 19:33

support CMake $<CONFIG> generator expressions

5eb39aa

fix the C-like syntax

de5c159

add an explicit cas to avoid the mypy error

3bc8baa

reimplement reading of config-declined properties

7b87249

fix mypy types

742d874

import only when checking to avoid a cycle

6666b01

test MAP_IMPORTED_<CONFIG> handling

eae58ba

refactor to address static code analysis tool remarks

349cf8f

simplify the makefile

7100b66

mmomtchev force-pushed the generator-expression-config branch from fe9a4ce to 7100b66 Compare April 15, 2024 17:33

mmomtchev and others added 5 commits April 15, 2024 20:46

construct the name of the static library

f5e1efd

use only listed configs when IMPORTED_CONFIGURATIONS is defined

a3eb158

set explicitly the package config directory

eecdfc0

construct the library name when it has been explicitly modified (meso…

a3e98e9

…n on Windows)

set explicitly the library name with MSVC

648a3dd

mmomtchev mentioned this pull request Apr 16, 2024

cmake: dependencies between CMake targets work only if the dependent target is imported #13101

Open

joukewitteveen reviewed Apr 18, 2024

View reviewed changes

dcbaker requested changes Apr 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In CMake, support declining items depending on the configuration used #13071

In CMake, support declining items depending on the configuration used #13071

mmomtchev commented Apr 11, 2024 •

edited

joukewitteveen Apr 18, 2024

mmomtchev Apr 18, 2024

joukewitteveen Apr 19, 2024

dcbaker left a comment

dcbaker Apr 18, 2024

dcbaker Apr 18, 2024

mmomtchev Apr 19, 2024

dcbaker Apr 18, 2024

mmomtchev Apr 19, 2024

dcbaker Apr 18, 2024

dcbaker Apr 18, 2024

mmomtchev Apr 19, 2024

dcbaker Apr 18, 2024

mmomtchev Apr 19, 2024

dcbaker Apr 18, 2024

		mlog.debug('CMake build type set from the meson build type')
		cmake_args += [f'-DCMAKE_BUILD_TYPE={self.build_type}']

In CMake, support declining items depending on the configuration used #13071

Are you sure you want to change the base?

In CMake, support declining items depending on the configuration used #13071

Conversation

mmomtchev commented Apr 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker 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

mmomtchev commented Apr 11, 2024 •

edited