-
Notifications
You must be signed in to change notification settings - Fork 993
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
[CMakeToolchain] Setting CMAKE_<LANG>_COMPILER
#12556
[CMakeToolchain] Setting CMAKE_<LANG>_COMPILER
#12556
Conversation
conans/test/integration/toolchains/cmake/test_cmaketoolchain.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! My two cents based on some recent discussions.
Regarding the name of the conf:
Users might intuitively expect that given a compiler name as a setting:
[settings]
compiler=xxx
That the following may "look" redundant or repetitive and users may wonder "am I not defining the same thing??"
[conf]
tools.build:compilers
When in fact they are two different things. I wonder if perhaps tools.build:compiler_executables
may be a more descriptive name? So that the setting refers to the name of the compiler in general, while the compiler_executables
is more about the actual program / command to run.
Regarding the documentation / description:
"Defines a Python dict-like with the compilers path to be used, e.g., {'CXX': '/usr/bin/clang++'}"
It may be worth mentioning that mentioning that this accepts a full path, a "name" (as done in one of the tests), in which case the underlying tools which works in the path.
Regarding more advanced usage:
Some tools support CC
and CXX
to have values that also have spaces - for example, I believe CXX=ccache clang++-13
would be a valid value when building with Autotools (leaving aside the fact that ccache may need further env vars to be set up correctly). Is this something we expect to work? What about build systems that don't support this because they have separate looking for compiler launchers? e.g. CMAKE_COMPILER_LAUNCHER=ccache
+ CMAKE_CXX_COMPILER=clang++-13
would work, but CMAKE_CXX_COMPILER="ccache clang++-13"
might not work?
Pointing to your comments and the original purpose of this PR, I've proposed another point of view exclusively for CMake. Let's discuss this new approach. |
734fde2
to
47255fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I struggle to see. The hardcoded definition of clang
have been removed from the toolchain, and now it will be necessary to specify it in the profile. If we want to build with clang some dependencies from ConanCenter we will need something like:
[conf]
tools.cmake.cmaketoolchain:set_compilers={"CXX": "C:/path/to/my/clang++", "C": ....}
tools.meson.mesontoolchain:set_compilers={"cpp": "C:/path/to/my/clang++", "c": ....}
tools.microsoft.msbuildtoolchain:set_compiler=...
tools.autotools.autotoolstoochain:set_compilers=={"CXX": "C:/path/to/my/clang++", "CC": ....}
# And maybe for some others Boost, OpenSSL, we also need the environment
[buildenv]
CC=(path)C:/path/to/my/clang
CXX=(path)C:/path/to/my/clang++
Of course there are some tricks that might not make necessary all of the above, like MesonToolchain
is now extracting things from the environment to define the meson_toolchain.ini
values of compiler=xxxxxx
, which is not great IMO, and also autotools might leverage the env-var directly, no strictly needed that this value is defined in the autotoolstoolchain.sh
(but it would be great for symmetry with other tools).
I am not convinced this is the best approach, and I still think that an abstract definition of the current compilers executables should be good enough for all build systems. Or just rely on the environment CC/CXX, and allow CMakeToolchain to capture it, so it is not strictly necessary for CMake users (the majority) to activate an environment script always, just to operate.
conans/model/conf.py
Outdated
@@ -52,6 +52,7 @@ | |||
"tools.build:defines": "List of extra definition flags used by different toolchains like CMakeToolchain and AutotoolsToolchain", | |||
"tools.build:sharedlinkflags": "List of extra flags used by CMakeToolchain for CMAKE_SHARED_LINKER_FLAGS_INIT variable", | |||
"tools.build:exelinkflags": "List of extra flags used by CMakeToolchain for CMAKE_EXE_LINKER_FLAGS_INIT variable", | |||
"tools.build:compiler_executables": "Defines a Python dict-like with the compilers path to be used. Allowed keys {'cc', 'cuda', 'cxx', 'objc', 'objcxx', 'rc'}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be great to have a simple mechanism to check possible values for any configuration if necessary.
Just a couple of comments. I think the last approach is much better, I really like it! On my side it's ready once they are solved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely much better, I like it.
Just recall to document it as experimental
, just in case.
I guess this PR also addresses #12474 (but maybe not all cases like archiver path, linker path or fortran path). |
@franramirez688 seems like it is breaking change, so it should be reverted (see conan-io/conan-center-index#14508) |
Maybe not revert, just set default |
Changelog: Feature: Add
tools.build:compiler_executables
configuration to set compilers variables in CMakeToolchain, MesonToolchain, and AutoToolsToolchain, e.g.,CMAKE_<LANG>_COMPILER
inCMakeToolchain
.Changelog: Fix: Remove hardcoded definitions of
CMAKE_CXX_COMPILER
inCMakeToolchain
.Docs: conan-io/docs#2833
Closes: #9962
See also #12635
-- Coming from #12577 (CLOSED)
The hardcoded definition of CMAKE_XXX_COMPILER in CMakeToolchain can be removed:
CMAKE_<LANG>_COMPILER
#12556 that puts it directly in conan_toolchain.cmake