Re: [Tarantool-patches] [PATCH] cmake: set CMP0037 policy to NEW by default

[Igor Munkin <imun@tarantool.org>]

Olya,

Thanks for the patch! Sorry for nitpicking, but I still have several
comments.

On 25.05.20, Olga Arkhangelskaia wrote:
> 
> Igor, I have fixed nits in commit message. Sorry for that. I have lost
> 
> somewhere line from vimrc with spellchecker =(
> 
> Moreover, I left warning and old behaviour for cmake 3.10 and less.
> 
> See diff below.
> 

<snipped>

> New commit message:
> 
>    + cmake: set CMP0037 policy to NEW
>    +
>    + To fix deprecation warning, CMP0037 policy was changed to NEW for 
> cmake
>    + 3.11 and above.
>    +
>    + CMP0037 old behavior (cmake 2.8.12) allowed target names such as test.
>    + In cmake 3.10 and below names test, help and etc. were reserved.
>    +
>    + Starting from cmake 3.11 these names are only reserved when the 
> corresponding
>    + feature is enabled (e.g. by including the CTest or CPack modules).
>    + Tarantool does not use CTest so the name test can be used.
>    +
>    + Closes: #3587

I guess you missed this one:
| > > Please, don't use ':' in gh tags. It just doesn't respect our commit
| > > message style.
| > >

> 
> Code diff:
> 
> -if(POLICY CMP0037)
> -    cmake_policy(SET CMP0037 NEW)
> -endif(POLICY CMP0037)

It looks better to reorder conditions: make policy test an outer one
and check CMake version in scope of it.

> +if(CMAKE_VERSION VERSION_LESS 3.11)

Minor: Strictly saying this policy should be used for versions starting
from 3.0, when the policy was introduced, to 3.11, when CMake team
realized that unconditional reserving for popular target names is
insane. At the same time I checked your last patch on Centos 7 and saw
no warning in the output. So I guess you can just leave the
corresponding comment or feel free to ignore this remark.

> +    if(POLICY CMP0037)
> +        cmake_policy(SET CMP0037 OLD)
> +    endif(POLICY CMP0037)
> +else()
> +    if(POLICY CMP0037)
> +        cmake_policy(SET CMP0037 NEW)
> +    endif(POLICY CMP0037)
> +endif()
> 

Otherwise, the changes LGTM, thanks! Sergey, please proceed with the
patch.

<snipped>

-- 
Best regards,
IM