Remove cmake_minimum_required from toolchain files #412
Conversation
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.
There was a problem hiding this comment.
I agree that this change is likely the right one to make.
There was a problem hiding this comment.
Ok, let's go with this. I'm not as familiar with CMake conventions as I would like but it seems like there is some consensus to remove this. I can see how a user of an already-built toolchain using these files doesn't really care what version of CMake was used to compile the toolchain. And CMake policy CMP0000 only really talks about requiring minimum versions in CMakeLists.txt.
@thewtex, can you remove this line from the wasi-sdk-p2.cmake file as well?
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
d0706b1 to
f7dbeaf
Compare
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.