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.

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

[feature] CMakeToolchain cacheVariables should have a type #16078

Closed
1 task done
jmarrec opened this issue Apr 15, 2024 · 1 comment
Closed
1 task done

[feature] CMakeToolchain cacheVariables should have a type #16078

jmarrec opened this issue Apr 15, 2024 · 1 comment

Comments

@jmarrec
Copy link
Contributor

jmarrec commented Apr 15, 2024

What is your suggestion?

Type mismatch in CMake configure arguments is a common issue that is really hard to track down, and as such I'm really pedantic about specifying the type of the variables when configuring CMake, especially for path/filepath ones.

--DMY_FLAG=ON
+-DMY_FLAG:BOOL=ON

-DMY_PATH=/path/to/file
+DMY_PATH:FILEPATH=/path/to/file

-DMY_DIR_PATH=/path/to/dir
+DMY_DIR_PATH:PATH=/path/to/dir

I noticed that the CMake presets do not hold a type for cacheVariables, while the CMake documentation explicitly says it can be a value, or a an object with both type and value, cf https://cmake.org/cmake/help/v3.29/manual/cmake-presets.7.html#configure-preset

Perhaps should consider explictly setting it, around

cache_variables = {}
for name, value in self.cache_variables.items():
if isinstance(value, bool):
cache_variables[name] = "ON" if value else "OFF"
elif isinstance(value, _PackageOption):
if str(value).lower() in ["true", "false", "none"]:
cache_variables[name] = "ON" if bool(value) else "OFF"
elif str(value).isdigit():
cache_variables[name] = int(value)
else:
cache_variables[name] = str(value)
else:
cache_variables[name] = value
buildenv, runenv, cmake_executable = None, None, None
if self._conanfile.conf.get("tools.cmake.cmaketoolchain:presets_environment", default="",
check_type=str, choices=("disabled", "")) != "disabled":
build_env = self.presets_build_environment.vars(self._conanfile) if self.presets_build_environment else VirtualBuildEnv(self._conanfile, auto_generate=True).vars()
run_env = self.presets_run_environment.vars(self._conanfile) if self.presets_run_environment else VirtualRunEnv(self._conanfile, auto_generate=True).vars()
buildenv = {name: value for name, value in
build_env.items(variable_reference="$penv{{{name}}}")}
runenv = {name: value for name, value in
run_env.items(variable_reference="$penv{{{name}}}")}
cmake_executable = self._conanfile.conf.get("tools.cmake:cmake_program", None) or self._find_cmake_exe()
write_cmake_presets(self._conanfile, toolchain, self.generator, cache_variables,
self.user_presets_path, self.presets_prefix, buildenv, runenv,
cmake_executable)

Related:


Quoting Professional CMake - Practical Guide, 15th edition by Craig Scott (I hope that's ok, since I can already find the same online at https://discourse.cmake.org/t/cmake-d-var-type-value/1135)

There is a special case for handling values initially declared without a type on the cmake command
line. If the project’s CMakeLists.txt file then tries to set the same cache variable and specifies a type
of FILEPATH or PATH , then if the value of that cache variable is a relative path, CMake will treat it as
being relative to the directory from which cmake was invoked and automatically convert it to an
absolute path. This is not particularly robust, since cmake could be invoked from any directory, not
just the build directory. Therefore, developers are advised to always include a type if specifying a
variable on the cmake command line for a variable that represents some kind of path.

It is a good habit to always specify the type of the variable on the command line in general anyway so that it is
likely to be shown in GUI applications in the most appropriate form. It will also prevent one of the
surprising behavior scenarios mentioned in the previous section

Have you read the CONTRIBUTING guide?

  • I've read the CONTRIBUTING guide
@memsharded
Copy link
Member

Closing after #16079 is closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants