Remove cmake_minimum_required from toolchain files #412
Conversation
This command is generally not included in toolchain files. It is used in project configuration files that will consume the toolchain file. Also, such a low value triggers warnings, support for <3.5.0 is being removed from CMake.
thewtex
force-pushed
the
toolchain-cmake-minimum-required
branch
from
96789d1
to
aee9c61
Compare
| @@ -1,8 +1,5 @@ | |||
| # Cmake toolchain description file for the Makefile | |||
|
|
|||
| # This is arbitrary, AFAIK, for now. | |||
| cmake_minimum_required(VERSION 3.4.0) | |||
There was a problem hiding this comment.
The android NDK toolchain file does seem to include a cmake_minimum_version: https://android.googlesource.com/platform/ndk/+/master/build/cmake/android.toolchain.cmake#35
Are you sure this is not supposed to be here?
Perhaps we could bump to to 3.6 like the android one to avoid the warning you mention?
There was a problem hiding this comment.
Yes, it is not supposed to be there. Was it originally included for a specific reason?
See the examples in the official CMake documentation. Or these examples.
The warning I pointed to is one example of unintended consequences, but there are more. CMake changes its behavior based on policies, which are impacted by a cmake_minimum_required call.
There was a problem hiding this comment.
That line as been there since the initial commit (c80964d).
I don't know it was included, it looks like we would have to go back to yury's wasmception to find out why.
It sounds like you know what you are talking about cmake-wise, and also emscripten's cmake does not specify cmake_minimum_required so I'm tempted to just approve this.
Lets wait for @pchickey to chime in though.
There was a problem hiding this comment.
Thank you for your attention and consideration.
This command is generally not included in toolchain files. It is used in
project configuration files that will consume the toolchain file. Also,
such a low value triggers warnings, support for <3.5.0 is being removed
from CMake.