Tarantool development patches archive
 help / color / mirror / Atom feed
From: Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Igor Munkin <imun@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system
Date: Mon, 15 Feb 2021 19:13:54 +0300	[thread overview]
Message-ID: <20210215161354.GI9361@root> (raw)
In-Reply-To: <9426dbd66c51aa466ab89272894ad33a7b22edec.1612390822.git.imun@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

  reply	other threads:[~2021-02-15 16:14 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust " Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation Igor Munkin via Tarantool-patches
2021-02-15 13:12   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:17     ` Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system Igor Munkin via Tarantool-patches
2021-02-15 16:13   ` Sergey Kaplun via Tarantool-patches [this message]
2021-02-19 23:10     ` Igor Munkin via Tarantool-patches
2021-02-20  7:42       ` Timur Safin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI Igor Munkin via Tarantool-patches
2021-02-15 16:29   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:29     ` Igor Munkin via Tarantool-patches
     [not found]       ` <0b6601d7075c$64329060$2c97b120$@tarantool.org>
2021-02-20 10:18         ` Igor Munkin via Tarantool-patches
2021-02-24  7:16   ` Timur Safin via Tarantool-patches
2021-02-25 22:08     ` Igor Munkin via Tarantool-patches
2021-02-26 19:04       ` Timur Safin via Tarantool-patches
2021-02-26 19:09 ` [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Timur Safin via Tarantool-patches
2021-02-26 21:06   ` Igor Munkin via Tarantool-patches
2021-02-27 13:56 ` Sergey Kaplun via Tarantool-patches
2021-02-28 22:05 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210215161354.GI9361@root \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=skaplun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox