Feature: add support for setting CMAKE_GENERATOR_INSTANCE #16229
Conversation
|
|
|
As you can see, making some tests with @jcar87 we found a problem with quotation escaping. <str>.strip('\'').strip('"') if <str> is not None else NoneIt may be useful to add a this functionality to |
| # Test input from command line passing path between doble quotes | ||
| client.run('install . --profile:build=windows --profile:host=windows -c tools.cmake.cmaketoolchain:generator_instance="C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/"') | ||
| toolchain = client.load("conan_toolchain.cmake") | ||
| assert "set(CMAKE_GENERATOR_INSTANCE \"C:/Program Files (x86)/Microsoft Visual Studio/2017/BuildTools/\")" in toolchain |
There was a problem hiding this comment.
Use same assert in the 3 cases, not one with single quote without escaping, and the other asserts with double quotes and escaping. I'll copy the first one in the 3 cases.
| @@ -955,14 +958,18 @@ def context(self): | |||
| result = self._get_winsdk_version(system_version, generator_platform) | |||
| system_version, winsdk_version, gen_platform_sdk_version = result | |||
|
|
|||
| generator_instance = self._conanfile.conf.get("tools.cmake.cmaketoolchain:generator_instance") | |||
| generator_instance = generator_instance.strip('\'').strip('"') if generator_instance is not None else None | |||
There was a problem hiding this comment.
I think the quoting management would be better addressed more homogeneusly with other confs/options, and not in isolation. If we allow quotes in profiles for this conf, but not for others, it can be confusing and error prone (other confs adding quotes will fail).
I suggest:
- Remove this management of extra quotes in this PR
- Use strings without quotes in profile files in the tests (similar to all other previous confs)
Then we can consider about possible alternatives to make this integrated with conf.get(), so other confs can use it too, but separately to this PR.
There was a problem hiding this comment.
Okey, strips removed, we will consider later if we want to support quotations in conan profiles
There was a problem hiding this comment.
I have a a small general question: Couldn't this be set on an user toolchain so that vairables like this don't proliferate in the Conan conf??
Otherwise the code looks good :)
Edit: Nevermind, I just read #16222 (comment), it was already brought up!
Changelog: Feature: add support for setting CMAKE_GENERATOR_INSTANCE
Docs: conan-io/docs#3719
Closes: #16222