From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 C1C21469710 for ; Tue, 26 May 2020 12:07:32 +0300 (MSK) Date: Tue, 26 May 2020 11:59:07 +0300 From: Igor Munkin Message-ID: <20200526085907.GJ5455@tarantool.org> References: <20200316121548.48251-1-arkholga@tarantool.org> <20200427170936.GO11314@tarantool.org> <20200525122847.GH5455@tarantool.org> <85045cdc-ff4e-f1a1-7fb4-dbe67bb38160@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <85045cdc-ff4e-f1a1-7fb4-dbe67bb38160@tarantool.org> 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! 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. | > > > > 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. -- Best regards, IM