From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 52B52469710 for ; Mon, 25 May 2020 15:37:12 +0300 (MSK) Date: Mon, 25 May 2020 15:28:47 +0300 From: Igor Munkin Message-ID: <20200525122847.GH5455@tarantool.org> References: <20200316121548.48251-1-arkholga@tarantool.org> <20200427170936.GO11314@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org 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. > [1]: https://gitlab.com/tarantool/tarantool/-/jobs/562530585 -- Best regards, IM