[Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system
Igor Munkin
imun at tarantool.org
Sat Feb 20 02:10:09 MSK 2021
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 at 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 at 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
More information about the Tarantool-patches
mailing list