Make cmake minimum version same accross repo #136
Conversation
| @@ -10,7 +10,7 @@ | |||
| # Unit tests | |||
| # ========== | |||
|
|
|||
| cmake_minimum_required(VERSION 3.1) | |||
| cmake_minimum_required(VERSION 3.24) | |||
There was a problem hiding this comment.
We do not bump the version of required tools unless there is explicit need for it. Is there such a need here?
There was a problem hiding this comment.
This was just for consistency between the main cmakelists and the one in the tests. No other reason.
There was a problem hiding this comment.
It being 3.1 was irrelevant since the main one requested a minimum of 3.24 .
There was a problem hiding this comment.
Ok, then my question holds for the main one but let's see if somebody complains. Generally older tools are available in more places than the newer ones and it is a trade-off between having new tools with better functionality and having them everywhere so that the users can use the software we build.
There was a problem hiding this comment.
This is where is why the main version was requested to have a new version due the use of MAKE_COMPILE_WARNING_AS_ERROR in the ci #88 (comment) . We could probably reduce the minimum by using a different variable.
There was a problem hiding this comment.
Regarding consistency - My thinking (also Johan & Sylvain would agree) is that whenever we update the version for some dependency or anything it should enforce consistency among all xeus stack . So technically we look at xeus-python as the first thing where a change is introduced and all other xeus kernels follow.
As far as I know we're using 3.4.3 these days
There was a problem hiding this comment.
@vgvassilev @anutosh491 So what we need is an older cmake variable which is essentially equivalent to MAKE_COMPILE_WARNING_AS_ERROR . Then we can have a lower minimum which is consistent with the rest of the xeus stack.
There was a problem hiding this comment.
I agree that xeus-cpp should do what the rest of the stack does unless it has a good reason not to. For example, treating warnings as errors is a good reason to not conform in my point of view. Of course we can implement it with more code in a conforming way but that’d require more efforts which may not be justified.
I think the change is fine for me, just pointing out something that we should be careful about.
Currently cmake minimum version in the test folder is older than the one for the main repo. This updates it to have the same minimum.