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 BA4F66EC5A; Mon, 15 Feb 2021 19:14:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BA4F66EC5A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1613405683; bh=ak9DKZq8ag9j6G34hIQHRTUfheP6zLNDajMONmnmOds=; 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=sqD6oJOIMzzCXzkjwXRlPZP35VrBRMuZFuo2lNYkt/ALFiAukj9EnmSdSYzsTm41B HOeVGWVc0CgD+GVPyJC2G4wRo8C95POwyuBdI6MJh67N+YrgUj0PDEHW2gUW0y3myT Q1BVj9fBcdJ4MLiHbzWTx80wGTW66mT0yx0hwRvs= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 B14B76EC5A for ; Mon, 15 Feb 2021 19:14:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B14B76EC5A Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1lBgWD-0004vy-HJ; Mon, 15 Feb 2021 19:14:38 +0300 Date: Mon, 15 Feb 2021 19:13:54 +0300 To: Igor Munkin Message-ID: <20210215161354.GI9361@root> References: <9426dbd66c51aa466ab89272894ad33a7b22edec.1612390822.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9426dbd66c51aa466ab89272894ad33a7b22edec.1612390822.git.imun@tarantool.org> X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91883A1EE8D2E99327A08A4263FA5D41CBCD47BF76BEDDBF7182A05F5380850408A1E0E90DF0E7755A58C60489C3EDF6DBCD2C9853F65808953C35257FABC5685 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71BB7708D34E2BFDAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637835928C62272F24E8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC999110C33E5F4A0F2AE182D0984F5AC914FCD5FF8B6ED2F7389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C07E7E81EEA8A9722B8941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B68424CA1AAF98A6958941B15DA834481F9449624AB7ADAF3735872C767BF85DA29E625A9149C048EE0A3850AC1BE2E735D2D576BCF940C7364AD6D5ED66289B524E70A05D1297E1BB35872C767BF85DA227C277FBC8AE2E8B81DF583BEE9BDE68EFF80C71ABB335746BA297DBC24807EA27F269C8F02392CDC58410348177836EABEDDA51113D120200306258E7E6ABB4E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2BBE337FB72E923155C0AF1600DCBC20BF7270DC4A9C1D11620FB76076734EE2C X-C1DE0DAB: 0D63561A33F958A5A7A88F89D0B76E517512B8A07120D9BFCEEC36D7D9E56729D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75448CF9D3A7B2C848410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D341E08D7EE1804B763732150C050CE0152C20E3256E65DE6EA733D456488B17924AD9A92DEBC8F92571D7E09C32AA3244CCA11FDC9D9BEEE2A26ADA32B946F4546B4DF56057A86259FFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojiBTwj6noE5f7LPcULCLoBw== X-Mailru-Sender: 3B9A0136629DC91206CBC582EFEF4CB4197A1360F1652197AC536894B359309E9EA0A951FF19AFE5F2400F607609286E924004A7DEC283833C7120B22964430C52B393F8C72A41A89437F6177E88F7363CDA0F3B3F5B9367 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: Sergey Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. 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. > > 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. > 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. > > Signed-off-by: Igor Munkin > --- > .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/.luacheckrc b/.luacheckrc > index b75d89a0c..4d5593a83 100644 > --- a/.luacheckrc > +++ b/.luacheckrc > @@ -35,7 +35,6 @@ exclude_files = { > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 4fbd19558..b9f4ec465 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -159,7 +159,7 @@ add_custom_target(ctags DEPENDS tags) > 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 @@ > - > # > # LuaJIT configuration file. > # > +# A copy of LuaJIT is maintained within Tarantool source tree. > +# It's located in third_party/luajit. > # > # LUAJIT_FOUND Typo: s/LUAJIT_FOUND/LuaJIT_FOUND/ based on [1]. May be redundant, see the comment below. > # LUAJIT_LIBRARIES > # LUAJIT_INCLUDE_DIRS > # > +# This stuff is extremely fragile, proceed with caution. > +macro(TestAndAppendFLag flags flag) Nit: Why macro not function? It is visible to all project after including. Feel free to ignore. > + 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}") > endif() > endmacro() > > +set(BUILDMODE static CACHE STRING > + "Build mode: build only static lib" FORCE) > +set(LUAJIT_ENABLE_GC64 ${LUAJIT_ENABLE_GC64} CACHE BOOL > + "GC64 mode for x64" FORCE) > +set(LUAJIT_SMART_STRINGS ON CACHE BOOL > + "Harder string hashing function" FORCE) > +set(LUAJIT_TEST_BINARY $ CACHE STRING > + "Lua implementation to be used for tests (tarantool)" FORCE) > +set(LUAJIT_USE_TEST OFF CACHE BOOL Sorry, don't get it: why OFF by default? > + "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? > + set(LUAJIT_USE_APICHECK ON CACHE BOOL > + "Assertions for the Lua/C API" FORCE) > + set(LUAJIT_USE_ASSERT ON CACHE BOOL > + "Assertions for the whole LuaJIT VM" FORCE) > +endif() > > -if (LUAJIT_PREFIX AND ENABLE_BUNDLED_LUAJIT) > - message (FATAL_ERROR "Options LUAJIT_PREFIX and ENABLE_BUNDLED_LUAJIT " > - "are not compatible with each other.") > +# Valgrind can be used only with the system allocator. For more > +# info see LuaJIT root CMakeLists.txt. Typo: s/info see/info, see/ > +if(ENABLE_VALGRIND) > + set(LUAJIT_USE_SYSMALLOC ON CACHE BOOL > + "System provided memory allocator (realloc)" FORCE) > + set(LUAJIT_USE_VALGRIND ON CACHE BOOL > + "Valgrind support" FORCE) > endif() > +add_dependencies(build_bundled_libs libluajit) Nit: Looks like it should be in the root , not here. Feel free to ignore. > > - set (luajit_buildoptions > - BUILDMODE=static > set(LuaJIT_FIND_REQUIRED TRUE) Nit: I see no reason to keep this variable alive. > 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. > 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? Feel free to ignore. > + > +# XXX: Since Tarantool use LuaJIT internals to implement all > +# required interfaces, several defines and flags need to be set > +# for Tarantool too. > +# FIXME: Hope everything below will have gone in a future. Side note: Me too! > + > +# Include LuaJIT source directory to use the internal headers. > +include_directories(${LUAJIT_SOURCE_ROOT}/src) > + > +# Since LUAJIT_SMART_STRINGS is enabled for LuaJIT bundle, it > +# should be unconditionally enabled for Tarantool too. Otherwise, > +# all modules using LuaJIT internal headers are misaligned. > +add_definitions(-DLUAJIT_SMART_STRINGS=1) > +# The same is done for LUAJIT_ENABLE_GC64 but under the condition. > +if(LUAJIT_ENABLE_GC64) > + add_definitions(-DLUAJIT_ENABLE_GC64) > +endif() 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. > + > +# Restore the preserved CFLAGS. > +set(CMAKE_C_FLAGS ${CMAKE_C_FLAGS_BCKP}) > diff --git a/cmake/luatest.cpp b/cmake/luatest.cpp > deleted file mode 100644 > index e9c951933..000000000 > --- a/cmake/luatest.cpp > +++ /dev/null > @@ -1,80 +0,0 @@ > diff --git a/debian/control b/debian/control > index ce810ee67..d2390e95c 100644 > --- a/debian/control > +++ b/debian/control > diff --git a/rpm/tarantool.spec b/rpm/tarantool.spec > index ffd161862..939442f10 100644 > --- a/rpm/tarantool.spec > +++ b/rpm/tarantool.spec > @@ -108,6 +108,8 @@ BuildRequires: python-gevent >= 1.0 > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 9a712bc29..b12bce58b 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > @@ -52,21 +52,21 @@ lua_source(lua_sources lua/httpc.lua) > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index baa704037..d40b92237 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -12,14 +12,6 @@ function(build_module module files) > diff --git a/third_party/luajit b/third_party/luajit > index c0c2e62a5..2350dd7ed 160000 > --- a/third_party/luajit > +++ b/third_party/luajit > @@ -1 +1 @@ > -Subproject commit c0c2e62a5404b51ba740ed28762e65b2ef56bcf9 > +Subproject commit 2350dd7ed112c2c7b2f96c6ef8701eeac62bf9d7 > -- > 2.25.0 > [1]: https://cmake.org/cmake/help/v3.3/module/FindPackageHandleStandardArgs.html -- Best regards, Sergey Kaplun