[Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system
Sergey Kaplun
skaplun at tarantool.org
Mon Feb 15 19:13:54 MSK 2021
Hi, Igor!
Thanks for the patch!
I'm glad to see the new <cmake/luajit.cmake>! 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, <LuaJIT-test> and <LuaJIT-luacheck> 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 <imun at tarantool.org>
> ---
> .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 = {
<snipped>
> 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)
<snipped>
> 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.
> #
<snipped>
> +# 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.
<snipped>
> +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()
>
<snipped>
> +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 $<TARGET_FILE:tarantool> 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 <test> 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()
<snipped>
> +add_dependencies(build_bundled_libs libluajit)
Nit: Looks like it should be in the root <CMakeLists.txt>, not here.
Feel free to ignore.
>
> - set (luajit_buildoptions
> - BUILDMODE=static
<snipped>
> 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 @@
<snipped>
> diff --git a/debian/control b/debian/control
> index ce810ee67..d2390e95c 100644
> --- a/debian/control
> +++ b/debian/control
<snipped>
> 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
<snipped>
> 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)
<snipped>
> 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)
<snipped>
> 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
More information about the Tarantool-patches
mailing list