From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id DDA0D7030C; Tue, 9 Feb 2021 16:55:45 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DDA0D7030C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1612878946; bh=mmY7Nuz0Ki0nUXkum7aNLMTk/atc2vCsD+7ly7pwtSI=; h=To:References:In-Reply-To:Date:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=EiTHdrbLJqC60ww+RVmqune4EHCO6gZY9bCPFuOywYXTJ58SZlnwDFBTHEaNOUIso Ck0dxdkuki2xLfq2LkmqphHsfq9tMLSfpqRblYb7U0fJRU+PGupTfhrCwR38HwzzVq JgjHbWXVE60r8IRz53U7xmaWgmeGocapKqm9uuG8= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 0A7297030C for ; Tue, 9 Feb 2021 16:55:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0A7297030C Received: by smtp54.i.mail.ru with esmtpa (envelope-from ) id 1l9TUQ-0004Vi-PV; Tue, 09 Feb 2021 16:55:39 +0300 To: "'Igor Munkin'" References: <7cd42be38d86c832ecd4ba0f3edd7ae83aead7ad.1612291495.git.imun@tarantool.org> <11be01d6fb48$9d0e1050$d72a30f0$@tarantool.org> <20210208155638.GB5448@tarantool.org> In-Reply-To: <20210208155638.GB5448@tarantool.org> Date: Tue, 9 Feb 2021 16:55:35 +0300 Message-ID: <05a601d6feeb$3ddc0f50$b9942df0$@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQJPfenCWrjRxS0krGRZARdqgOlJ3gKwr8KjAj52qboB0o5oQKkoynZg Content-Language: ru X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD953AC099BC0052A9CD238BCF93DF23716D1711D0DDC4F5AC2182A05F5380850409C7B6ACA2AC715406F36ABFE4B5D934993E71DF61ACDFC1CE500390AB859712F X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71BB7708D34E2BFDAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A431CDDF496E6E598638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC23A03A9B45077CD5E61F88CB64AC3EAF097267803B713AFA389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0D9442B0B5983000E8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B64854413538E1713FCC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C22496816F24B7DE16C0876E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8B120C23C05AEC2B18EC76A7562686271EEC990983EF5C032935872C767BF85DA29E625A9149C048EE0A3850AC1BE2E7358CCB3ED2A1DE23044AD6D5ED66289B524E70A05D1297E1BB35872C767BF85DA227C277FBC8AE2E8B9EB32FE0C26A3A4C75ECD9A6C639B01B4E70A05D1297E1BBC6867C52282FAC85D9B7C4F32B44FF57C2F2A386D11C4599BD9CCCA9EDD067B1EDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A578A1ADF2D92026B9EEBE9E2F008FA9B8F55B1281C70B2CB3D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D1AE09A115117C96DE1012152E2A00B4134E14C4A0510E014968913A816189C99BE80C8D4FAA269B1D7E09C32AA3244C4DDD20F6D463280B462AE2288F6C2412F2F5F14F68F1805BFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojg67HQLCbniLzxv6EVmLPIg== X-Mailru-Sender: 6CA451E36783D721CBEA96CEA26D325D431F77A50506AFDF29F9ADD5BDF39654B7CBEF92542CD7C82F97C478340294DCC77752E0C033A69E0F0C7111264B8915FF1320A92A5534336C18EFA0BB12DBB0 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/5] build: replace GNU Make with CMake X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Timur Safin via Tarantool-patches Reply-To: Timur Safin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" LGTM, if you need a shorter answer, but there are some further comments = below... : From: Igor Munkin : Subject: Re: [PATCH luajit 2/5] build: replace GNU Make with CMake :=20 : > : + : > : +# CMake generated artefacts : > : +CMakeCache.txt : > : +CMakeFiles : > : +Makefile : > : +cmake_install.cmake : > : +compile_commands.json : > : +install_manifest.txt : > : +luajit-parse-memprof : > : +luajit.pc : > : > Uh-oh, this ugly hack would be handled by single exclusion : > build*/ : > if we would all use the same (idiomatic) approach for out-of-source : > build. But we do not yet accustomed to that, so... never mind! :=20 : I personally prefer mechanisms to policies: this approach allows one = to : freely name so called directory on his own e.g. , : , or even , that is my favourite one. : Furthermore this approach allows to ignore CMake output for both : out-of-source and in-source build. Such configuration is much more : flexible, IMHO, and leads to a neglible and well localized changes in : .gitignore. But, nevermind ;) Ok, most of those names are generic enough as generated by cmake, so it's a good idea to exclude them.=20 ... :=20 : > : + : > : +# Switch to harder (and slower) hash function when a collision : > : +# chain in the string hash table exceeds certain length. : > : +option(LUAJIT_SMART_STRINGS "Harder string hashing function" ON) : > : +if(LUAJIT_SMART_STRINGS) : > : + AppendFlags(TARGET_C_FLAGS -DLUAJIT_SMART_STRINGS=3D1) : > : +endif() : > : > The same note about lack of ENABLE prefix in the option name. :=20 : Well, again: we do care about consistency. Unfortunately, this option = is : provided only in our fork, so the inconsistency you're talking about = was : messed up years ago. I propose to port the build system as is now with : no changes in the option names, but this is a nice point to refactor : them and make them consistent in scope of a separate issue. Thoughts? Ok, then leave them as is. This patchset supposed to be refactoring of=20 what is already there.=20 ... : > : > FWIW -msse2 implicitly assumes -msse but if that way it used to be : > in makefiles than so be it. Don't need to "improve" it. :=20 : Yes, I tried to move everything making as few changes as possible. I = believe : there are lots of places to be "modernized" later. Yup, we should leave it as is today. ... :=20 : > : > : + : > : +include(SetTargetFlags) : > : +list(APPEND TARGET_LIBS m) : > : > ${TARGET_LIBS} below used as space separated list of words, it's = probably : > bad idea to operate with it as list, because we end up in trace with : something : > like : > : > ./third_party/luajit/src/CMakeLists.txt(302): : target_include_directories(luajit_static PRIVATE : ./build/third_party/luajit/src ) : > ./third_party/luajit/src/CMakeLists.txt(305): : target_link_libraries(luajit_static libluajit_static dl;m ) : > :=20 : Hm, I'm not quite sure what is broken by such usage, but everything is : OK when I run with VERBOSE=3D1. Here is the related part: : | $ make VERBOSE=3D1 : | <...> : | [100%] Linking C executable luajit : | cd /tarantool-luajit/src && /usr/bin/cmake -E cmake_link_script : CMakeFiles/luajit_static.dir/link.txt --verbose=3D1 : | /usr/bin/cc -fomit-frame-pointer -fno-stack-protector -Wall = -rdynamic - : Wl,-E CMakeFiles/luajit_static.dir/luajit.c.o -o luajit libluajit.a = -ldl - : lm : | make[2]: Leaving directory '/tarantool-luajit' : | [100%] Built target luajit_static : | <...> Ok then, that's probably means that target_link_libraries() is expecting = list=20 at that place, and this works as expected. So, never mind.=20 ... : > : > Here we have reasonable cmake complain: : > : > -- Configuring done : > WARNING: Target "luajit_static" has EXCLUDE_FROM_ALL set and will = not be : built by default but : > an install rule has been provided for it. CMake does not define : behavior for this case. : > WARNING: Target "libluajit_static" has EXCLUDE_FROM_ALL set and = will not : be built by default : > but an install rule has been provided for it. CMake does not = define : behavior for this case. : > :=20 : Well, I have no warning at all on my machine... It's cmake 3.13.4 here, may be warning is version dependent.=20 : | $ cmake . : | -- The C compiler identification is GNU 8.3.0 : | -- Check for working C compiler: /usr/bin/cc : | -- Check for working C compiler: /usr/bin/cc -- works : | -- Detecting C compiler ABI info : | -- Detecting C compiler ABI info - done : | -- Detecting C compile features : | -- Detecting C compile features - done : | -- [SetVersion] Reading version from VCS: v2.1.0-beta3-79-ge616d62 : | -- The ASM compiler identification is GNU : | -- Found assembler: /usr/bin/cc : | -- Configuring done : | -- Generating done : | -- Build files have been written to: /tarantool-luajit :=20 : > There is no much point to install ${LIBLUAJIT_STATIC_DEPS} or : ${LIBLUAJIT_SHARED_DEPS} if we have : > excluded luajit_static and/or libluajit_shared from target all : dependencies via EXCLUDE_FROM_ALL. :=20 : ... however, I looked to the docs[2] and there is mentioned the fact : such behaviour is not defined and user is responsible to ensure = whether : the corresponding artefacts are built prior to installing them. I : believed this machinery can be done on CMake side, but I forgot that : CMake is a crap. Wrapped all install rules with if/endif, squashed, : force-pushed to the branch. Diff is below: :=20 : = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D : =3D=3D=3D=3D :=20 : diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt : index 5724c8b..51e97a6 100644 : --- a/src/CMakeLists.txt : +++ b/src/CMakeLists.txt : @@ -338,21 +338,35 @@ set(LUAJIT_BINARY $ : PARENT_SCOPE) : add_custom_target(libluajit DEPENDS ${LIBLUAJIT_DEPS}) : add_custom_target(luajit ALL DEPENDS libluajit ${LUAJIT_DEPS}) :=20 : -install(TARGETS ${LUAJIT_DEPS} : - RUNTIME : - DESTINATION bin : - COMPONENT luajit : -) : -install(TARGETS ${LIBLUAJIT_STATIC_DEPS} : - ARCHIVE : - DESTINATION lib : - COMPONENT luajit : -) : -install(TARGETS ${LIBLUAJIT_SHARED_DEPS} : - LIBRARY : - DESTINATION lib : - COMPONENT luajit : -) : +# Unfortunately, CMake provides no guarantees for install commands : +# used for the targets excluded from and obligues user to : +# handle this manually on his side. Hence check whether the : +# targets used below are presented for the chosen build mode. : +# See more info in CMake docs below: : +# https://cmake.org/cmake/help/v3.1/prop_tgt/EXCLUDE_FROM_ALL.html : +if(TARGET ${LUAJIT_DEPS}) : + install(TARGETS ${LUAJIT_DEPS} : + RUNTIME : + DESTINATION bin : + COMPONENT luajit : + ) : +endif() : + : +if(TARGET ${LIBLUAJIT_STATIC_DEPS}) : + install(TARGETS ${LIBLUAJIT_STATIC_DEPS} : + ARCHIVE : + DESTINATION lib : + COMPONENT luajit : + ) : +endif() : + : +if(TARGET ${LIBLUAJIT_SHARED_DEPS}) : + install(TARGETS ${LIBLUAJIT_SHARED_DEPS} : + LIBRARY : + DESTINATION lib : + COMPONENT luajit : + ) : +endif() Noice! Best Regards, Timur