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

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

[Olga Arkhangelskaia]

Remove setting old CMP0037 policy explicitly, it may be
removed in later versions of cmake.

Closes #3587
---
Branches: https://github.com/tarantool/tarantool/tree/OKriw/gh-3587-CMP0037-OLD-cmake-deprecation-warning-full-ci

 test/CMakeLists.txt | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 9b5df7dc5..61bf7d5e1 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -17,10 +17,6 @@ endfunction()
 add_compile_flags("C;CXX"
     "-Wno-unused-parameter")
 
-if(POLICY CMP0037)
-    cmake_policy(SET CMP0037 OLD)
-endif(POLICY CMP0037)
-
 add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap
     COMMAND ${CMAKE_COMMAND} -E create_symlink
         ${PROJECT_SOURCE_DIR}/third_party/luajit/test
-- 
2.20.1 (Apple Git-117)

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

[Igor Munkin]

Olya,

Thanks for the patch! Unfortunately, the changes don't fix the
problem[1]. Did you find the approach provided by Sasha[2] wrong?

On 16.03.20, Olga Arkhangelskaia wrote:
> Remove setting old CMP0037 policy explicitly, it may be
> removed in later versions of cmake.
> 
> Closes #3587
> ---
> Branches: https://github.com/tarantool/tarantool/tree/OKriw/gh-3587-CMP0037-OLD-cmake-deprecation-warning-full-ci
> 
>  test/CMakeLists.txt | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 9b5df7dc5..61bf7d5e1 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -17,10 +17,6 @@ endfunction()
>  add_compile_flags("C;CXX"
>      "-Wno-unused-parameter")
>  
> -if(POLICY CMP0037)
> -    cmake_policy(SET CMP0037 OLD)
> -endif(POLICY CMP0037)
> -
>  add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap
>      COMMAND ${CMAKE_COMMAND} -E create_symlink
>          ${PROJECT_SOURCE_DIR}/third_party/luajit/test
> -- 
> 2.20.1 (Apple Git-117)
> 

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/472976392#L464
[2]: https://github.com/tarantool/tarantool/issues/3587#issue-346204529

-- 
Best regards,
IM

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

[Olga Arkhangelskaia]

Hi Igor! Thanks for the review.

I did not get Sasha's approach. However, according to cmake 
documentations name test in

add_custom_target can be used if the modules CTest or CPAck is enabled.

https://cmake.org/cmake/help/latest/policy/CMP0037.html?highlight=cmp0037

So we just need to use NEW behaviour and cmake 3.11 and above.

See:

-if(POLICY CMP0037)
-    cmake_policy(SET CMP0037 OLD)
-endif(POLICY CMP0037)
-

+ if(POLICY CMP0037)
+    cmake_policy(SET CMP0037 NEW)
+ endif(POLICY CMP0037)
+


27.04.2020 20:09, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! Unfortunately, the changes don't fix the
> problem[1]. Did you find the approach provided by Sasha[2] wrong?
>
> On 16.03.20, Olga Arkhangelskaia wrote:
>> Remove setting old CMP0037 policy explicitly, it may be
>> removed in later versions of cmake.
>>
>> Closes #3587
>> ---
>> Branches: https://github.com/tarantool/tarantool/tree/OKriw/gh-3587-CMP0037-OLD-cmake-deprecation-warning-full-ci
>>
>>   test/CMakeLists.txt | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
>> index 9b5df7dc5..61bf7d5e1 100644
>> --- a/test/CMakeLists.txt
>> +++ b/test/CMakeLists.txt
>> @@ -17,10 +17,6 @@ endfunction()
>>   add_compile_flags("C;CXX"
>>       "-Wno-unused-parameter")
>>   
>> -if(POLICY CMP0037)
>> -    cmake_policy(SET CMP0037 OLD)
>> -endif(POLICY CMP0037)
>> -
>>   add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap
>>       COMMAND ${CMAKE_COMMAND} -E create_symlink
>>           ${PROJECT_SOURCE_DIR}/third_party/luajit/test
>> -- 
>> 2.20.1 (Apple Git-117)
>>
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/472976392#L464
> [2]: https://github.com/tarantool/tarantool/issues/3587#issue-346204529
>

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

[Igor Munkin]

Olya,

Thanks for the patch! The warning is gone (I checked the patch manually
on Gentoo with Cmake 3.14.6), but I see configuration failures on some
platforms (e.g. Ubuntu 18.04[1]). It looks like the problem occurs when
NEW policy is used for CMake older than 3.11 (Ubuntu 18.04 default repos
provides 3.10.2). I reproduced the failure manually with Ubuntu docker.
After reverting your patch, configuration succeeds with no warnings. It
looks like we need a custom behaviour for different CMake versions,
doesn't it?

Furthermore, I left several nits regarding commit message below, please
consider them.

On 21.05.20, Olga Arkhangelskaia wrote:
> Hi Igor! Thanks for the review.
> 
> I did not get Sasha's approach. However, according to cmake 
> documentations name test in
> 
> add_custom_target can be used if the modules CTest or CPAck is enabled.
> 
> https://cmake.org/cmake/help/latest/policy/CMP0037.html?highlight=cmp0037
> 
> So we just need to use NEW behaviour and cmake 3.11 and above.
> 
> See:

Please also include the commit message whether it was changed:
| cmake: set CMP0037 policy to NEW
|
| To stop deprecation warning CMP0037 policy was changed to NEW.

Typo: s/stop/fix/.

| Deprecation warnings tells us that OLD behaviour will be dropped in the

Minor: s/tells/inform/.

| next versions.
|
| CMP0037 old behavior (cmake 2.8.12) allowed taget names such as test,

Typo: s/taget/target/.

| however in versions 3.10 and below names test, help and etc. were

Typo: consider line break right after <were> to fit 72 symbols.

| complitely anavailable because cmake always reserved them.

Typo: s/complitely/completely/.
Typo: s/anavailable/unavailable/.
Minor: I guess <always> can be omitted considering the past tense.

| Starting from cmake 3.11 this names only reserved when the corresponding

Typo: s/this names only reserved/these names are only reserved/.

| feature is enabled (e.g. by including the CTest or CPack modules).
| Tarantool does not use CTest so the name test can be used.
| Users have to use cmake 3.11 and above.

Side note: Do we need to update Tarantool build requirements anywhere?

|
| Closes: #3587

Please, don't use ':' in gh tags. It just doesn't respect our commit
message style.

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

Side note: there is no whitespace change on the branch.

> 

<snipped>

[1]: https://gitlab.com/tarantool/tarantool/-/jobs/562530585

-- 
Best regards,
IM

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

[Olga Arkhangelskaia]

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.

25.05.2020 15:28, Igor Munkin пишет:
> Olya,
>
> Thanks for the patch! The warning is gone (I checked the patch manually
> on Gentoo with Cmake 3.14.6), but I see configuration failures on some
> platforms (e.g. Ubuntu 18.04[1]). It looks like the problem occurs when
> NEW policy is used for CMake older than 3.11 (Ubuntu 18.04 default repos
> provides 3.10.2). I reproduced the failure manually with Ubuntu docker.
> After reverting your patch, configuration succeeds with no warnings. It
> looks like we need a custom behaviour for different CMake versions,
> doesn't it?
>
> Furthermore, I left several nits regarding commit message below, please
> consider them.
>
> On 21.05.20, Olga Arkhangelskaia wrote:
>> Hi Igor! Thanks for the review.
>>
>> I did not get Sasha's approach. However, according to cmake
>> documentations name test in
>>
>> add_custom_target can be used if the modules CTest or CPAck is enabled.
>>
>> https://cmake.org/cmake/help/latest/policy/CMP0037.html?highlight=cmp0037
>>
>> So we just need to use NEW behaviour and cmake 3.11 and above.
>>
>> See:
> Please also include the commit message whether it was changed:
> | cmake: set CMP0037 policy to NEW
> |
> | To stop deprecation warning CMP0037 policy was changed to NEW.
>
> Typo: s/stop/fix/.
>
> | Deprecation warnings tells us that OLD behaviour will be dropped in the
>
> Minor: s/tells/inform/.
>
> | next versions.
> |
> | CMP0037 old behavior (cmake 2.8.12) allowed taget names such as test,
>
> Typo: s/taget/target/.
>
> | however in versions 3.10 and below names test, help and etc. were
>
> Typo: consider line break right after <were> to fit 72 symbols.
>
> | complitely anavailable because cmake always reserved them.
>
> Typo: s/complitely/completely/.
> Typo: s/anavailable/unavailable/.
> Minor: I guess <always> can be omitted considering the past tense.
>
> | Starting from cmake 3.11 this names only reserved when the corresponding
>
> Typo: s/this names only reserved/these names are only reserved/.
>
> | feature is enabled (e.g. by including the CTest or CPack modules).
> | Tarantool does not use CTest so the name test can be used.
> | Users have to use cmake 3.11 and above.
>
> Side note: Do we need to update Tarantool build requirements anywhere?
>
> |
> | Closes: #3587
>
> Please, don't use ':' in gh tags. It just doesn't respect our commit
> message style.
>
>> -if(POLICY CMP0037)
>> -    cmake_policy(SET CMP0037 OLD)
>> -endif(POLICY CMP0037)
>> -
>>
>> + if(POLICY CMP0037)
>> +    cmake_policy(SET CMP0037 NEW)
>> + endif(POLICY CMP0037)
>> +
>>
> Side note: there is no whitespace change on the branch.
>
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

Code diff:

-if(POLICY CMP0037)
-    cmake_policy(SET CMP0037 NEW)
-endif(POLICY CMP0037)
+if(CMAKE_VERSION VERSION_LESS 3.11)
+    if(POLICY CMP0037)
+        cmake_policy(SET CMP0037 OLD)
+    endif(POLICY CMP0037)
+else()
+    if(POLICY CMP0037)
+        cmake_policy(SET CMP0037 NEW)
+    endif(POLICY CMP0037)
+endif()


> <snipped>
>
> [1]: https://gitlab.com/tarantool/tarantool/-/jobs/562530585
>

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

[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.
| > >

> 
> 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

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

[Igor Munkin]

Almost forgot: please provide a ChangeLog entry.

-- 
Best regards,
IM

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

[Olga Arkhangelskaia]

@ChaneLog

- use cmake CMP0037 new politics starting from 3.11 and above

26.05.2020 12:12, Igor Munkin пишет:
> Almost forgot: please provide a ChangeLog entry.
>

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

[Olga Arkhangelskaia]

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>
>

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

[Alexander Turenko]

LGTM.

Pushed to master and 2.4. Several minor comments below.

WBR, Alexander Turenko.

On Wed, May 27, 2020 at 01:15:35PM +0300, Olga Arkhangelskaia wrote:
> 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.

Nit: AFAIU, 'politics' and 'policy' are not synonyms: the former is more
about goverment actions (see [1]).

> +    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.

Nit: We usually fit our comments within 66 symbols (except URIs).

>          cmake_policy(SET CMP0037 NEW)
> -    endif(POLICY CMP0037)
> +    endif()
>  endif()

I don't see those changes on the branch [2]. Anyway, I applied those
changes manually and changes according to the comments above, pushed it
to master and 2.4 and removed the branch.

[1]: https://dictionary.cambridge.org/us/grammar/british-grammar/politics-political-politician-or-policy
[2]: OKriw/gh-3587-CMP0037-OLD-cmake-deprecation-warning-full-ci