From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 34599469710 for ; Mon, 25 May 2020 19:19:08 +0300 (MSK) References: <20200316121548.48251-1-arkholga@tarantool.org> <20200427170936.GO11314@tarantool.org> <20200525122847.GH5455@tarantool.org> From: Olga Arkhangelskaia Message-ID: <85045cdc-ff4e-f1a1-7fb4-dbe67bb38160@tarantool.org> Date: Mon, 25 May 2020 19:19:06 +0300 MIME-Version: 1.0 In-Reply-To: <20200525122847.GH5455@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 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 to fit 72 symbols. > > | complitely anavailable because cmake always reserved them. > > Typo: s/complitely/completely/. > Typo: s/anavailable/unavailable/. > Minor: I guess 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() > > > [1]: https://gitlab.com/tarantool/tarantool/-/jobs/562530585 >