Fix #16078 - add type to CMakePresets cacheVariables #16079

jmarrec · 2024-04-15T10:45:09Z

Changelog: Feature: add type to CMakePresets cacheVariables

Close #16078

Fix [feature] CMakeToolchain cacheVariables should have a type #16078
I am assuming I'm missing tests and co, but this is more a proof of concept than anything else, to show that it wouldn't be too much effort to implement
Add types for BOOL, STRING, FILEPATH/PATH (based on assigning a pathlib.Path to the tc.cache_variables)

Refer to the issue that supports this Pull Request.
If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
I've read the Contributing guide.
I've followed the PEP8 style guides for Python code.
I've opened another PR in the Conan docs repo to the develop branch, documenting this one.

CLAassistant · 2024-04-15T10:45:15Z

All committers have signed the CLA.

jmarrec · 2024-04-15T10:47:23Z

conan/tools/cmake/toolchain/toolchain.py

+                cache_variables[name] = {"value": "ON" if value else "OFF", "type": "BOOL"}
+            elif isinstance(value, Path):
+                cache_variables[name] = {
+                    "value": str(value),
+                    "type": "FILEPATH" if value.is_file() else 'PATH'
+                }
+            elif isinstance(value, str):
+                cache_variables[name] = {"value": value, "type": "STRING"}
            elif isinstance(value, _PackageOption):
                if str(value).lower() in ["true", "false", "none"]:
-                    cache_variables[name] = "ON" if bool(value) else "OFF"
+                    cache_variables[name] = {"value": "ON" if bool(value) else "OFF", "type": "BOOL"}
                elif str(value).isdigit():
                    cache_variables[name] = int(value)
                else:
-                    cache_variables[name] = str(value)
+                    cache_variables[name] = {"value": str(value), "type": "STRING"}


Replace cache_variables with an object with both value and type where appropriate

Test example:

tc = CMakeToolchain(self) tc.cache_variables["MY_BOOL"] = False tc.cache_variables["MY_FILE_PATH"] = Path("/path/to/file") # assert Path("/path/to/file").is_file() tc.cache_variables["MY_FILE_PATH"] = Path("/path/to/dir") # assert Path("/path/to/file").is_dir() tc.generate()

jmarrec · 2024-04-15T10:48:24Z

conan/tools/cmake/presets.py

@@ -44,7 +44,7 @@ def generate(conanfile, toolchain_file, generator, cache_variables, preset_prefi

        if "BUILD_TESTING" not in cache_variables:
            if conanfile.conf.get("tools.build:skip_test", check_type=bool):
-                cache_variables["BUILD_TESTING"] = "OFF"
+                cache_variables["BUILD_TESTING"] = {"value": "OFF", "type": bool}


Also add it to a few vars in presets.py itself

jmarrec · 2024-04-15T10:50:12Z

conan/tools/cmake/presets.py

+            return f'"{val}"' if isinstance(val, str) and " " in val else f"{val}"
+
+        def _format_var(var, value_or_dict):
+            type_str = ""
+            if isinstance(value_or_dict, dict):
+                value = value_or_dict["value"]
+                type_str = f":{value_or_dict['type']}"
+            else:
+                value = value_or_dict
+
+            return f"-D{var}{type_str}={_format_val(value)}"

        # https://github.com/conan-io/conan/pull/12034#issuecomment-1253776285
        cache_variables_info = " ".join(
-            [f"-D{var}={_format_val(value)}" for var, value in cache_variables.items()])
-        add_toolchain_cache = f"-DCMAKE_TOOLCHAIN_FILE={toolchain_file} " \
+            [_format_var(var, value) for var, value in cache_variables.items()])
+        add_toolchain_cache = f"-DCMAKE_TOOLCHAIN_FILE:FILEPATH={toolchain_file} " \


Properly format the message that is printed as well:

conanfile.py: If your CMake version is not compatible with CMakePresets (<3.23) call cmake like: 'cmake <path> -G Ninja -DCMAKE_TOOLCHAIN_FILE:FILEPATH=/media/path/to/conan_toolchain.cmake -DMY_BOOL:BOOL=ON -DMY_FILE_PATH:FILEPATH=/path/to/file -DMY_DIR_PATH:PATH=/path/to/dir

memsharded · 2024-04-16T14:25:39Z

I have done a commit to fix one broken "integration" test, but then a lot of "functional" tests are failing:

logs.txt (Linux logs, but other OSs are failing as well)

And it is not failing in obvious ways. So this is a big concern, that this change might be breaking users in unexpected ways.
To move this forward it will be necessary a very compelling example of something that clearly fails (and can be marked as a bug) when the type of the cache variable is not specified, because so far it has been working quite smoothly.

memsharded · 2024-04-27T22:05:46Z

Hi @jmarrec

Have you been able to check my comment above?

To move this forward it will be necessary a very compelling example of something that clearly fails (and can be marked as a bug) when the type of the cache variable is not specified, because so far it has been working quite smoothly.

The changes you are proposing are not risk-free, even if the broken tests might be fixed, the risk of breaking users is still there. So it would be necessary to provide some broken/bug cases in which the proposed changes solve the issue.

Can you please try to provide such a "red" test to re-consider this PR? Otherwise I think we will have to discard this PR, even if the typing is a nice-to-have, if it is not solving some actual user issues, it might not be worth the risk of breaking users.

jmarrec · 2024-05-03T12:21:30Z

I can't think of a way to produce a bug, so if you don't want to pursue it that's fine.

memsharded · 2024-05-05T21:18:08Z

Yes, I think that unless it is solving some specific issue that we can reproduce, it would be better not to add this extra complexity, as it doesn't seem completely risk-free. Thanks for understanding.

jmarrec commented Apr 15, 2024

View reviewed changes

jmarrec added 2 commits April 15, 2024 15:08

Fix conan-io#16078 - add type to cmakepresets cacheVariables

b56582b

adjust test to include the type

dbbb1c3

jmarrec force-pushed the 16078_cacheVariables_types branch from 6c06148 to dbbb1c3 Compare April 15, 2024 13:09

memsharded mentioned this pull request Apr 16, 2024

[question] cacheVariables data type mismatch in generated CMakePresets.json #14568

Open

1 task

memsharded added 2 commits April 16, 2024 14:59

Merge branch 'develop2' into 16078_cacheVariables_types

3730b75

fix test

11c127b

memsharded added the staled The issue has been inactive for a while and will be closed soon label Apr 27, 2024

jmarrec closed this May 3, 2024

memsharded mentioned this pull request May 5, 2024

[feature] CMakeToolchain cacheVariables should have a type #16078

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #16078 - add type to CMakePresets cacheVariables #16079

Fix #16078 - add type to CMakePresets cacheVariables #16079

jmarrec commented Apr 15, 2024 •

edited by memsharded

CLAassistant commented Apr 15, 2024 •

edited

jmarrec Apr 15, 2024

jmarrec Apr 15, 2024

jmarrec Apr 15, 2024

memsharded commented Apr 16, 2024

memsharded commented Apr 27, 2024

jmarrec commented May 3, 2024 •

edited

memsharded commented May 5, 2024

Fix #16078 - add type to CMakePresets cacheVariables #16079

Fix #16078 - add type to CMakePresets cacheVariables #16079

Conversation

jmarrec commented Apr 15, 2024 • edited by memsharded

CLAassistant commented Apr 15, 2024 • edited

jmarrec Apr 15, 2024

Choose a reason for hiding this comment

jmarrec Apr 15, 2024

Choose a reason for hiding this comment

jmarrec Apr 15, 2024

Choose a reason for hiding this comment

memsharded commented Apr 16, 2024

memsharded commented Apr 27, 2024

jmarrec commented May 3, 2024 • edited

memsharded commented May 5, 2024

jmarrec commented Apr 15, 2024 •

edited by memsharded

CLAassistant commented Apr 15, 2024 •

edited

jmarrec commented May 3, 2024 •

edited