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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

Added your tag:
| Reviewed-by: Sergey Kaplun <skaplun@tarantool.org>

> 
> 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, <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.

No, it's not. See here[3].

Ignoring.

> 
> > 
> > Signed-off-by: Igor Munkin <imun@tarantool.org>
> > ---

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

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

<snipped>

> > +macro(TestAndAppendFLag flags flag)
> 
> Nit: Why macro not function? It is visible to all project after
> including.
> Feel free to ignore.

<flags> 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()
> >  

<snipped>

> > +set(LUAJIT_USE_TEST OFF CACHE BOOL
> 
> Sorry, don't get it: why OFF by default?

To respect CMP0002.

> 
> > +    "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?

I don't know. I guess there is something cached in CMakeCache.txt that
leads to such warnings. Maybe Timur will clarify this behaviour.

> 

<snipped>

> > +add_dependencies(build_bundled_libs libluajit)
> 
> Nit: Looks like it should be in the root <CMakeLists.txt>, not here.
> Feel free to ignore.

Originally, it was here, so I would like to leave it here too.

Ignoring.

> 

<snipped>

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

<snipped>

> 
> 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})

================================================================================

> 

<snipped>

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