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

[Olga Arkhangelskaia <arkholga@tarantool.org>]

Hi Igor!

Thanks for the review:

I have changed the order and left a small comment, so the diff looks like:

-if(CMAKE_VERSION VERSION_LESS 3.11)
-    if(POLICY CMP0037)
+if (POLICY CMP0037)
+    # https://cmake.org/cmake/help/v3.11/release/3.11.html#other-changes
+    # cmake below 3.11 reserves name test. Use old politics.
+    if (CMAKE_VERSION VERSION_LESS 3.11)
          cmake_policy(SET CMP0037 OLD)
-    endif(POLICY CMP0037)
-else()
-    if(POLICY CMP0037)
+    else()
+        # Starting from cmake 3.11 name test reserved in special cases 
and can
+        # be used as target name.
          cmake_policy(SET CMP0037 NEW)
-    endif(POLICY CMP0037)
+    endif()
  endif()


26.05.2020 11:59, Igor Munkin пишет:
> 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.
> | > >
Removed :
>> 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>
>