From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0219A469710 for ; Wed, 27 May 2020 13:15:36 +0300 (MSK) References: <20200316121548.48251-1-arkholga@tarantool.org> <20200427170936.GO11314@tarantool.org> <20200525122847.GH5455@tarantool.org> <85045cdc-ff4e-f1a1-7fb4-dbe67bb38160@tarantool.org> <20200526085907.GJ5455@tarantool.org> From: Olga Arkhangelskaia Message-ID: Date: Wed, 27 May 2020 13:15:35 +0300 MIME-Version: 1.0 In-Reply-To: <20200526085907.GJ5455@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH] cmake: set CMP0037 policy to NEW by default List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.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. >> > > >> 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. > > >