Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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@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