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 2FAAA6EC58; Sat, 20 Feb 2021 02:10:14 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2FAAA6EC58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613776214; bh=9od/OkVKRbHpAP5NYOJ+AfGrIyHmVVYabWSCtMdqmUE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=zcp+IUxX8XvBEOAZWG/gVPfJsa/hVDD5PqfyLQQIVEE/kmbwdnOOErE3tSRENF5n4 yWDnvPqzPg8gtCArFVIwZRm1CGa53SeLSjPQCmblQyNyJpjbuT0hz/Sboy++CGycmu trSaeSvcWDiieiZ0S/Ba6u9zcVDebVrBUk+l929U= 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 A6AE16EC58 for ; Sat, 20 Feb 2021 02:10:13 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A6AE16EC58 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lDEua-0004JC-Fm; Sat, 20 Feb 2021 02:10:12 +0300 Date: Sat, 20 Feb 2021 02:10:09 +0300 To: Sergey Kaplun Message-ID: <20210219231009.GS5448@tarantool.org> References: <9426dbd66c51aa466ab89272894ad33a7b22edec.1612390822.git.imun@tarantool.org> <20210215161354.GI9361@root> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210215161354.GI9361@root> X-Clacks-Overhead: GNU Terry Pratchett User-Agent: Mutt/1.10.1 (2018-07-13) X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD975C3EC174F5669229511437AA01F46811CFCF616A939B362182A05F53808504045B30753305A0F194861C502DC383608F20AAF70ED1C40ED729CAD7BD4762644 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A179494B5629353BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370BACBAB4C30C4AEB8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC1209201920434A12D9CABE3D0348878C20AF8618449A488E389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C0D9442B0B5983000E8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6D082881546D93491CC7F00164DA146DA6F5DAA56C3B73B23C77107234E2CFBA567F23339F89546C55F5C1EE8F4F765FCB9CEE4F2B4A90F8475ECD9A6C639B01BBD4B6F7A4D31EC0BC0CAF46E325F83A522CA9DD8327EE4930A3850AC1BE2E73528A6D463EDFD0DBBC4224003CC836476C0CAF46E325F83A50BF2EBBBDD9D6B0F05F538519369F3743B503F486389A921A5CC5B56E945C8DA X-C1DE0DAB: 0D63561A33F958A526A2602F152CA2F557FFC193313EDAA0BE4E1F89DC54611ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D345C064E16D8ABF33577472BD3DD386D77F0C262A534C1FAF9EB1BE521A346EC8BAAA40AAC9287BE1C1D7E09C32AA3244CE123AA33EC56068114A125E17E5003783E8609A02908F271927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojPfquRGj7313miDfZB+/E0w== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638227E1E339316A01DF41409FFEF855F938FA7C8D0F45F857DBFE9F1EFEE2F478337FB559BB5D741EB964C8C2C849690F8E70A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system 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: Igor Munkin via Tarantool-patches Reply-To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, Thanks for your review! On 15.02.21, Sergey Kaplun wrote: > Hi, Igor! > > Thanks for the patch! > > I'm glad to see the new ! It becomes much prettier! > It's not mentioned here, **but** now we can do experiments with > Tarantool in bed with LuaJIT much easier! Like build Tarantool with > `-DLUAJIT_DISABLE_JIT=ON` and check the results of benchmarks. Cool! > > LGTM, except a few questions and nitpicks below. Added your tag: | Reviewed-by: Sergey Kaplun > > On 04.02.21, Igor Munkin wrote: > > LuaJIT submodule is bumped to introduce the following changes: > > * test: run luacheck static analysis via CMake > > * test: fix warnings found with luacheck in misclib* > > * test: run LuaJIT tests via CMake > > * build: replace GNU Make with CMake > > * build: preserve the original build system > > Nit: I prefer Kirill's order of notation from oldest to newest, > because I'm not used to reading upside down. > Feel free to ignore. AFAICS, there is no strict order of the patches listed in bump (examples are here[1] and here[2]), so I'll leave it unchanged. Ignoring. > > > > > Since LuaJIT build system is ported to CMake in scope of the changeset > > mentioned above, the module building the LuaJIT bundled in Tarantool is > > completely reworked. There is no option to build Tarantool against > > another prebuilt LuaJIT due to a91962c0df8f649f7ebee2fb2c90c0e3810acf5f > > ('Until Bug#962848 is fixed, don't try to compile with external > > Nit: Citation is good, but with a link it would be perfect. > Feel free to ignore. The link to the commit itself? This doesn't respect our practice. > > > LuaJIT'), so all redundant options defining the libluajit to be used in > > Tarantool are dropped with the related auxiliary files. > > > > To run LuaJIT related tests or static analysis for Lua files within > > LuaJIT repository, and targets are used > > respectively as a dependency of the corresponding Tarantool targets. > > > > As an additional dependency to run LuaJIT tests, prove[1] utility is > > required, so the necessary binary packages are added to the lists with > > build requirements. > > > > [1]: https://metacpan.org/pod/TAP::Harness#prove > > > > Closes #4862 > > Closes #5470 > > Nit: Sorry me to be pedantic, but actually the next commit closes this > issue, because tests are not run at that commit in CI. > Feel free to ignore. No, it's not. See here[3]. Ignoring. > > > > > Signed-off-by: Igor Munkin > > --- Added ChangeLog for the new style: ================================================================================ diff --git a/changelogs/unreleased/luajit-cmake.md b/changelogs/unreleased/luajit-cmake.md new file mode 100644 index 000000000..fd4e8fa59 --- /dev/null +++ b/changelogs/unreleased/luajit-cmake.md @@ -0,0 +1,5 @@ +## feature/testing + +* Implemented self-sufficient LuaJIT testing environment. As a result LuaJIT + build system is partially ported to CMake and all testing machinery is + enclosed within tarantool/luajit repository (gh-4862, gh-5470). ================================================================================ > > .luacheckrc | 1 - > > CMakeLists.txt | 2 +- > > cmake/luajit.cmake | 375 ++++++++++++-------------------------------- > > cmake/luatest.cpp | 80 ---------- > > debian/control | 2 + > > rpm/tarantool.spec | 2 + > > src/CMakeLists.txt | 28 ++-- > > test/CMakeLists.txt | 18 +-- > > third_party/luajit | 2 +- > > 9 files changed, 119 insertions(+), 391 deletions(-) > > delete mode 100644 cmake/luatest.cpp > > diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake > > index 1c7784e11..455b4967b 100644 > > --- a/cmake/luajit.cmake > > +++ b/cmake/luajit.cmake > > @@ -1,306 +1,125 @@ > > +macro(TestAndAppendFLag flags flag) > > Nit: Why macro not function? It is visible to all project after > including. > Feel free to ignore. variable need to be expanded twice: the first time to interpolate the variable name and the second time to interpolate that variable value. Unfortunately, there is no way to implement it in a different way via CMake. Ignoring. > > > + string(REGEX REPLACE "-" "_" TESTFLAG ${flag}) > > + string(TOUPPER ${TESTFLAG} TESTFLAG) > > + # XXX: can't use string(PREPEND ...) on ancient versions. > > + set(TESTFLAG "CC_HAS${TESTFLAG}") > > + if(${${TESTFLAG}}) > > + set(${flags} "${${flags}} ${flag}") Here is the problem spot. > > endif() > > endmacro() > > > > +set(LUAJIT_USE_TEST OFF CACHE BOOL > > Sorry, don't get it: why OFF by default? To respect CMP0002. > > > + "Generate target" FORCE) > > + > > +# Enable internal LuaJIT assertions for Tarantool Debug build. > > +if(CMAKE_BUILD_TYPE STREQUAL "Debug") > > Side note: I get tons of warnings like: > > | [ 47%] Building C object third_party/luajit/src/CMakeFiles/core_static.dir/lib_string.c.o > | [ 47%] Building C object third_party/luajit/src/CMakeFiles/core_static.dir/lib_table.c.o > | /home/burii/reviews/tarantool/luajit-cmake/third_party/luajit/src/lj_strfmt.c: In function 'lj_strfmt_putfxint': > | /home/burii/reviews/tarantool/luajit-cmake/third_party/luajit/src/lj_strfmt.c:260:9: warning: variable 'ps' set but not used [-Wunused-but-set-variable] > | 260 | char *ps; > | | ^~ > > If build with `-DCMAKE_BUILD_TYPE=Debug` first, **do not** run > `rm CMakeCache.txt` and then build with > `-DCMAKE_BUILD_TYPE=RelWithDebInfo`. I am not familiar with CMake, is it > an expected behaviour? I don't know. I guess there is something cached in CMakeCache.txt that leads to such warnings. Maybe Timur will clarify this behaviour. > > > +add_dependencies(build_bundled_libs libluajit) > > Nit: Looks like it should be in the root , not here. > Feel free to ignore. Originally, it was here, so I would like to leave it here too. Ignoring. > > > set(LuaJIT_FIND_REQUIRED TRUE) > > Nit: I see no reason to keep this variable alive. Yes, it looks excess. Dropped it below. > > > find_package_handle_standard_args(LuaJIT > > REQUIRED_VARS LUAJIT_INCLUDE LUAJIT_LIB) > > Also, I think these lines are redundant at all, like the previous one. Ditto. > > > set(LUAJIT_INCLUDE_DIRS ${LUAJIT_INCLUDE}) > > set(LUAJIT_LIBRARIES ${LUAJIT_LIB}) > > Nit: Why do not just provide ${LUAJIT_INCLUDE_DIRS} and ${LUAJIT_LIBRARIES} > at once? Fixed, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake index a31fa1acd..33ab88f2a 100644 --- a/cmake/luajit.cmake +++ b/cmake/luajit.cmake @@ -4,9 +4,8 @@ # A copy of LuaJIT is maintained within Tarantool source tree. # It's located in third_party/luajit. # -# LUAJIT_FOUND -# LUAJIT_LIBRARIES # LUAJIT_INCLUDE_DIRS +# LUAJIT_LIBRARIES # # This stuff is extremely fragile, proceed with caution. @@ -81,8 +80,8 @@ set(LUAJIT_BINARY_ROOT ${PROJECT_BINARY_DIR}/third_party/luajit) add_subdirectory(${LUAJIT_SOURCE_ROOT} ${LUAJIT_BINARY_ROOT} EXCLUDE_FROM_ALL) set(LUAJIT_PREFIX ${LUAJIT_BINARY_ROOT}/src) -set(LUAJIT_INCLUDE ${LUAJIT_PREFIX}) -set(LUAJIT_LIB ${LUAJIT_PREFIX}/libluajit.a) +set(LUAJIT_INCLUDE_DIRS ${LUAJIT_PREFIX}) +set(LUAJIT_LIBRARIES ${LUAJIT_PREFIX}/libluajit.a) add_dependencies(build_bundled_libs libluajit) @@ -98,12 +97,6 @@ install( DESTINATION ${MODULE_INCLUDEDIR} ) -set(LuaJIT_FIND_REQUIRED TRUE) -find_package_handle_standard_args(LuaJIT - REQUIRED_VARS LUAJIT_INCLUDE LUAJIT_LIB) -set(LUAJIT_INCLUDE_DIRS ${LUAJIT_INCLUDE}) -set(LUAJIT_LIBRARIES ${LUAJIT_LIB}) - # XXX: Since Tarantool use LuaJIT internals to implement all # required interfaces, several defines and flags need to be set # for Tarantool too. ================================================================================ > Feel free to ignore. > > > Nit: Assume I become mad and try to configure Tarantool like this: > > | $ cmake -DLUAJIT_DISABLE_FFI=ON . > > Obviously it leads to the following compilation errors: > > | $ make -j > | ... > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libserver.a(utils.c.o): in function `luaL_pushcdata': > | /home/burii/reviews/tarantool/luajit-cmake/src/lua/utils.c:72: undefined reference to `lj_ctype_info' > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: /home/burii/reviews/tarantool/luajit-cmake/src/lua/utils.c:91: undefined reference to `lj_cconv_ct_init' > | /usr/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: ../../src/libserver.a(utils.c.o): in function `luaL_setcdatagc': > | /home/burii/reviews/tarantool/luajit-cmake/src/lua/utils.c:235: undefined reference to `lj_cdata_setfin' > > May be we should raise an error in configure phase when user tries to > build Tarantool with this flag? > > Feel free to ignore. Nice catch. Fixed, squashed, force-pushed to the branch. Diff is below: ================================================================================ diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake index 455b4967b..a31fa1acd 100644 --- a/cmake/luajit.cmake +++ b/cmake/luajit.cmake @@ -120,6 +120,12 @@ add_definitions(-DLUAJIT_SMART_STRINGS=1) if(LUAJIT_ENABLE_GC64) add_definitions(-DLUAJIT_ENABLE_GC64) endif() +# XXX: Tarantool can't be built with FFI machinery disabled, since +# there are lots of internals implemented with it. Hence, forbid +# user to disable FFI at configuration phase. +if(LUAJIT_DISABLE_FFI) + message(FATAL_ERROR "Tarantool requires LuaJIT FFI machinery to be enabled") +endif() # Restore the preserved CFLAGS. set(CMAKE_C_FLAGS ${CMAKE_C_FLAGS_BCKP}) ================================================================================ > > > -- > > 2.25.0 > > > > [1]: https://cmake.org/cmake/help/v3.3/module/FindPackageHandleStandardArgs.html > > -- > Best regards, > Sergey Kaplun [1]: https://github.com/tarantool/tarantool/commit/e503974 [2]: https://github.com/tarantool/tarantool/commit/a159409 [3]: https://lists.tarantool.org/tarantool-patches/cover.1612390822.git.imun@tarantool.org/T/#mb9f16885c859fa058ac944b9f391a10c5f804576 -- Best regards, IM