* [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system
@ 2021-02-03 23:22 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
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 UTC (permalink / raw)
To: Sergey Kaplun, Timur Safin; +Cc: tarantool-patches
The first patch of this series fixes the inaccuracy for out of source
build type. If Lua source path given to <lua_source> function is
relative, the output file is generated in the binary directory. At the
same time if the given path to be compiled to *.lua.c is absolute, the
output file is generated in source directory instead of the binary one.
In scope of the second patch the module for building the bundled LuaJIT
is completely reworked considering the changes made in LuaJIT repo[1].
The last patch adds LuaJIT tests to every CI job type except the one for
static build testing routine on OSX: there is no way to run LuaJIT tests
for out of source build on OSX due to SIP[2].
[1]: https://lists.tarantool.org/tarantool-patches/cover.1612291495.git.imun@tarantool.org/T/#t
[2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html
Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4862-cmake-full-ci
Issues:
* https://github.com/tarantool/tarantool/issues/4862
* https://github.com/tarantool/tarantool/issues/5470
CI is not green since OSX 11 is pretty stormy today. Hope it will be
fine the day when the changeset is merged into the trunk.
@ChangeLog:
* Port LuaJIT build system to CMake and make its testing environment
self-sufficient (gh-4862, gh-5470).
Igor Munkin (3):
build: fix lua.c file generation
build: adjust LuaJIT build system
ci: enable LuaJIT tests in CI
.luacheckrc | 1 -
.travis.mk | 20 +++
CMakeLists.txt | 2 +-
cmake/luajit.cmake | 375 ++++++++++++--------------------------------
cmake/luatest.cpp | 80 ----------
cmake/utils.cmake | 6 +-
debian/control | 2 +
rpm/tarantool.spec | 2 +
src/CMakeLists.txt | 28 ++--
test/CMakeLists.txt | 18 +--
third_party/luajit | 2 +-
11 files changed, 143 insertions(+), 393 deletions(-)
delete mode 100644 cmake/luatest.cpp
--
2.25.0
^ permalink raw reply [flat|nested] 19+ messages in thread* [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 ` Igor Munkin via Tarantool-patches 2021-02-15 13:12 ` Sergey Kaplun via Tarantool-patches 2021-02-03 23:22 ` [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system Igor Munkin via Tarantool-patches ` (4 subsequent siblings) 5 siblings, 1 reply; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 UTC (permalink / raw) To: Sergey Kaplun, Timur Safin; +Cc: tarantool-patches If Lua source path given to <lua_source> function is relative, the output file is generated in the binary directory. At the same time if the given path to be compiled to *.lua.c is absolute, the output file is generated in source directory instead of the binary one. This patch fixes the latter case providing the valid behaviour for out of source build type. Needed for #4862 Signed-off-by: Igor Munkin <imun@tarantool.org> --- cmake/utils.cmake | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmake/utils.cmake b/cmake/utils.cmake index eaec821b3..6d6960468 100644 --- a/cmake/utils.cmake +++ b/cmake/utils.cmake @@ -40,9 +40,11 @@ endmacro(set_source_files_compile_flags) # A helper function to compile *.lua source into *.lua.c sources function(lua_source varname filename) if (IS_ABSOLUTE "${filename}") + string (REPLACE "${CMAKE_SOURCE_DIR}" "${CMAKE_BINARY_DIR}" + genname "${filename}") set (srcfile "${filename}") - set (tmpfile "${filename}.new.c") - set (dstfile "${filename}.c") + set (tmpfile "${genname}.new.c") + set (dstfile "${genname}.c") else(IS_ABSOLUTE "${filename}") set (srcfile "${CMAKE_CURRENT_SOURCE_DIR}/${filename}") set (tmpfile "${CMAKE_CURRENT_BINARY_DIR}/${filename}.new.c") -- 2.25.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation 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 0 siblings, 1 reply; 19+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2021-02-15 13:12 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches Hi, Igor! Thanks for the patch! LGTM, except one nitpick below. On 04.02.21, Igor Munkin wrote: > If Lua source path given to <lua_source> function is relative, the Typo? s/Lua source path/the Lua source path/ Feel free to ignore. > output file is generated in the binary directory. At the same time if > the given path to be compiled to *.lua.c is absolute, the output > file is generated in source directory instead of the binary one. This > patch fixes the latter case providing the valid behaviour for out of Typo: s/case providing/case, providing/ > source build type. > > Needed for #4862 > > Signed-off-by: Igor Munkin <imun@tarantool.org> > --- > cmake/utils.cmake | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/cmake/utils.cmake b/cmake/utils.cmake > index eaec821b3..6d6960468 100644 > --- a/cmake/utils.cmake > +++ b/cmake/utils.cmake > @@ -40,9 +40,11 @@ endmacro(set_source_files_compile_flags) <snipped> > -- > 2.25.0 > -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation 2021-02-15 13:12 ` Sergey Kaplun via Tarantool-patches @ 2021-02-19 21:17 ` Igor Munkin via Tarantool-patches 0 siblings, 0 replies; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-19 21:17 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches Sergey, Thanks for your review! On 15.02.21, Sergey Kaplun wrote: > Hi, Igor! > > Thanks for the patch! > > LGTM, except one nitpick below. Fixed the nit below and added your tag: | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org> > <snipped> > -- > Best regards, > Sergey Kaplun -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 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-03 23:22 ` Igor Munkin via Tarantool-patches 2021-02-15 16:13 ` Sergey Kaplun via Tarantool-patches 2021-02-03 23:22 ` [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI Igor Munkin via Tarantool-patches ` (3 subsequent siblings) 5 siblings, 1 reply; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 UTC (permalink / raw) To: Sergey Kaplun, Timur Safin; +Cc: tarantool-patches 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 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 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 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 = { "test/box/*.test.lua", "test/engine/*.test.lua", "test/engine_long/*.test.lua", - "test/luajit-tap/**/*.lua", "test/replication/*.test.lua", "test/sql/**/*.lua", "test/swim/*.test.lua", 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) # Enable 'make luacheck' target. # -add_custom_target(luacheck) +add_custom_target(luacheck DEPENDS LuaJIT-luacheck) add_custom_command(TARGET luacheck COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}" COMMENT "Perform static analysis of Lua code" 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. # -# A copy of LuaJIT is maintained within Tarantool -# source. It's located in third_party/luajit. -# -# Instead of this copy, Tarantool can be compiled -# with a system-wide LuaJIT, or LuaJIT at a given -# prefix. This is used when compiling Tarantool -# as part of a distribution, e.g. Debian. -# -# To explicitly request use of the bundled LuaJIT, -# add -DENABLE_BUNDLED_LUAJIT=True to CMake -# configuration flags. -# To explicitly request use of LuaJIT at a given -# prefix, use -DLUAJIT_PREFIX=/path/to/LuaJIT. -# -# These two options are incompatible with each other. -# -# If neither of the two options is given, this script -# first attempts to use the system-installed LuaJIT -# and, in case it is not present or can not be used, -# falls back to the bundled one. +# A copy of LuaJIT is maintained within Tarantool source tree. +# It's located in third_party/luajit. # # LUAJIT_FOUND # LUAJIT_LIBRARIES # LUAJIT_INCLUDE_DIRS # +# This stuff is extremely fragile, proceed with caution. -# -# Bundled LuaJIT paths. -# -set (LUAJIT_BUNDLED_PREFIX "${PROJECT_BINARY_DIR}/third_party/luajit/src") -set (LUAJIT_BUNDLED_LIB "${LUAJIT_BUNDLED_PREFIX}/libluajit.a") - -macro (luajit_use_bundled) - set (LUAJIT_PREFIX "${LUAJIT_BUNDLED_PREFIX}") - set (LUAJIT_INCLUDE "${PROJECT_SOURCE_DIR}/third_party/luajit/src") - set (LUAJIT_LIB "${LUAJIT_BUNDLED_LIB}") - set (ENABLE_BUNDLED_LUAJIT True) -endmacro() - -# -# LuaJIT testing routine -# (see cmake/luatest.cpp for a description). -# -macro (luajit_test) - file (READ "${CMAKE_SOURCE_DIR}/cmake/luatest.cpp" LUAJIT_TEST) - set (CMAKE_REQUIRED_LIBRARIES "${LUAJIT_LIB}") - if (${CMAKE_SYSTEM_NAME} STREQUAL "Linux") - set (CMAKE_REQUIRED_LIBRARIES "-ldl ${CMAKE_REQUIRED_LIBRARIES}") - endif() - set (CMAKE_REQUIRED_INCLUDES "${LUAJIT_INCLUDE}") - CHECK_CXX_SOURCE_RUNS ("${LUAJIT_TEST}" LUAJIT_RUNS) - unset (LUAJIT_TEST) - unset (CMAKE_REQUIRED_LIBRARIES) - unset (CMAKE_REQUIRED_INCLUDES) -endmacro() - -# -# Check if there is a system LuaJIT availaible and -# usable with the server (determined by a compiled test). -# -macro (luajit_try_system) - find_path (LUAJIT_INCLUDE lj_obj.h PATH_SUFFIXES luajit-2.0 luajit) - find_library (LUAJIT_LIB NAMES luajit luajit-5.1 PATH_SUFFIXES x86_64-linux-gnu) - if (LUAJIT_INCLUDE AND LUAJIT_LIB) - message (STATUS "include: ${LUAJIT_INCLUDE}, lib: ${LUAJIT_LIB}") - message (STATUS "Found a system-wide LuaJIT.") - luajit_test() - if ("${LUAJIT_RUNS}" STREQUAL "1") - message (STATUS "System-wide LuaJIT at ${LUAJIT_LIB} is suitable for use.") - else() - message (WARNING "System-wide LuaJIT at ${LUAJIT_LIB} is NOT suitable for use, using the bundled one.") - luajit_use_bundled() - endif() - else() - message (FATAL_ERROR "Not found a system LuaJIT") - #luajit_use_bundled() - endif() -endmacro() - -# -# Check if there is a usable LuaJIT at the given prefix path. -# -macro (luajit_try_prefix) - find_path (LUAJIT_INCLUDE "lua.h" ${LUAJIT_PREFIX} NO_DEFAULT_PATH) - find_library (LUAJIT_LIB "luajit" ${LUAJIT_PREFIX} NO_DEFAULT_PATH) - if (LUAJIT_INCLUDE AND LUAJIT_LIB) - include_directories("${LUAJIT_INCLUDE}") - luajit_test() - if (LUAJIT_RUNS) - message (STATUS "LuaJIT at ${LUAJIT_PREFIX} is suitable for use.") - else() - message (FATAL_ERROR "LuaJIT at ${LUAJIT_PREFIX} is NOT suitable for use.") - endif() - else() - message (FATAL_ERROR "Couldn't find LuaJIT in '${LUAJIT_PREFIX}'") +macro(TestAndAppendFLag flags flag) + 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() -# -# LuaJIT options. -# -option(ENABLE_BUNDLED_LUAJIT "Enable building of the bundled LuaJIT" ON) -option(LUAJIT_PREFIX "Build with LuaJIT at the given path" "") +# Preserve the current CFLAGS and to not spoil the original ones with LuaJIT +# specific flags and defines. +set(CMAKE_C_FLAGS_BCKP ${CMAKE_C_FLAGS}) + +TestAndAppendFLag(CMAKE_C_FLAGS -Wno-parentheses-equality) +TestAndAppendFLag(CMAKE_C_FLAGS -Wno-tautological-compare) +TestAndAppendFLag(CMAKE_C_FLAGS -Wno-misleading-indentation) +TestAndAppendFLag(CMAKE_C_FLAGS -Wno-varargs) +TestAndAppendFLag(CMAKE_C_FLAGS -Wno-implicit-fallthrough) + +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 + "Generate <test> target" FORCE) + +# Enable internal LuaJIT assertions for Tarantool Debug build. +if(CMAKE_BUILD_TYPE STREQUAL "Debug") + 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. +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() -if (LUAJIT_PREFIX) - # trying to build with specified LuaJIT. - luajit_try_prefix() -elseif (NOT ENABLE_BUNDLED_LUAJIT) - # trying to build with system LuaJIT, macro can turn on - # building of LuaJIT bundled with the server source. - luajit_try_system() -else() - luajit_use_bundled() +# FIXME: ASAN support is badly implemented in LuaJIT and there is +# not a specific build options for this. At the same time there +# are several places wrapped with LUAJIT_USE_ASAN define. +# Just enable it here if needed and patiently wait until ASAN +# support is implemented properly in LuaJIT. +if(ENABLE_ASAN) + add_definitions(-DLUAJIT_USE_ASAN=1) endif() -unset (LUAJIT_RUNS) -#include_directories("${LUAJIT_INCLUDE}") +if(TARGET_OS_DARWIN) + # Necessary to make LuaJIT (and Tarantool) work on Darwin, see + # http://luajit.org/install.html. + set(CMAKE_EXE_LINKER_FLAGS + "${CMAKE_EXE_LINKER_FLAGS} -pagezero_size 10000 -image_base 100000000") +endif() -macro(luajit_build) - set (luajit_cc ${CMAKE_C_COMPILER} ${CMAKE_C_COMPILER_ARG1}) - set (luajit_hostcc ${CMAKE_HOST_C_COMPILER}) - # Cmake rules concerning strings and lists of strings are weird. - # set (foo "1 2 3") defines a string, while - # set (foo 1 2 3) defines a list. - # Use separate_arguments() to turn a string into a list (splits at ws). - # It appears that variable expansion rules are context-dependent. - # With the current arrangement add_custom_command() - # does the right thing. We can even handle pathnames with - # spaces though a path with an embeded semicolon or a quotation mark - # will most certainly wreak havok. - # - # This stuff is extremely fragile, proceed with caution. - set (luajit_cflags ${CMAKE_C_FLAGS}) - set (luajit_ldflags ${CMAKE_EXE_LINKER_FLAGS}) - separate_arguments(luajit_cflags) - separate_arguments(luajit_ldflags) - if(CC_HAS_WNO_PARENTHESES_EQUALITY) - set(luajit_cflags ${luajit_cflags} -Wno-parentheses-equality) - endif() - if(CC_HAS_WNO_TAUTOLOGICAL_COMPARE) - set(luajit_cflags ${luajit_cflags} -Wno-tautological-compare) - endif() - if(CC_HAS_WNO_MISLEADING_INDENTATION) - set(luajit_cflags ${luajit_cflags} -Wno-misleading-indentation) - endif() - if(CC_HAS_WNO_VARARGS) - set(luajit_cflags ${luajit_cflags} -Wno-varargs) - endif() - if (CC_HAS_WNO_IMPLICIT_FALLTHROUGH) - set(luajit_cflags ${luajit_cflags} -Wno-implicit-fallthrough) - endif() +# Define the locations for LuaJIT sources and artefacts. +set(LUAJIT_SOURCE_ROOT ${PROJECT_SOURCE_DIR}/third_party/luajit) +set(LUAJIT_BINARY_ROOT ${PROJECT_BINARY_DIR}/third_party/luajit) - if (LUAJIT_ENABLE_GC64) - add_definitions(-DLUAJIT_ENABLE_GC64=1) - elseif(TARGET_OS_DARWIN) - # Necessary to make LuaJIT work on Darwin, see - # http://luajit.org/install.html - set(CMAKE_EXE_LINKER_FLAGS ${CMAKE_EXE_LINKER_FLAGS} - "-pagezero_size 10000 -image_base 100000000") - endif() +add_subdirectory(${LUAJIT_SOURCE_ROOT} ${LUAJIT_BINARY_ROOT} EXCLUDE_FROM_ALL) - # We are consciously ommiting debug info in RelWithDebInfo mode - if (${CMAKE_BUILD_TYPE} STREQUAL "Debug") - set (luajit_ccopt -O0) - if (CC_HAS_GGDB) - set (luajit_ccdebug -g -ggdb) - else () - set (luajit_ccdebug -g) - endif () - add_definitions(-DLUA_USE_APICHECK=1) - add_definitions(-DLUA_USE_ASSERT=1) - else () - set (luajit_ccopt -O2) - set (luajit_ccdbebug "") - endif() - if (${CMAKE_SYSTEM_NAME} STREQUAL Darwin) - # Pass sysroot - prepended in front of system header/lib dirs, - # i.e. <sysroot>/usr/include, <sysroot>/usr/lib. - # Needed for XCode users without command line tools installed, - # they have headers/libs deep inside /Applications/Xcode.app/... - if (NOT "${CMAKE_OSX_SYSROOT}" STREQUAL "") - set (luajit_cflags ${luajit_cflags} ${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}) - set (luajit_ldflags ${luajit_ldlags} ${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}) - set (luajit_hostcc ${luajit_hostcc} ${CMAKE_C_SYSROOT_FLAG} ${CMAKE_OSX_SYSROOT}) - endif() - # Pass deployment target - if ("${CMAKE_OSX_DEPLOYMENT_TARGET}" STREQUAL "") - # Default to 10.6 since @rpath support is NOT available in - # earlier versions, needed by AddressSanitizer. - set (luajit_osx_deployment_target 10.6) - else() - set (luajit_osx_deployment_target ${CMAKE_OSX_DEPLOYMENT_TARGET}) - endif() - set(luajit_ldflags - ${luajit_ldflags} -Wl,-macosx_version_min,${luajit_osx_deployment_target}) - endif() - if (ENABLE_GCOV) - set (luajit_ccdebug ${luajit_ccdebug} -fprofile-arcs -ftest-coverage) - endif() - if (ENABLE_VALGRIND) - add_definitions(-DLUAJIT_USE_SYSMALLOC=1) - add_definitions(-DLUAJIT_USE_VALGRIND=1) - set (luajit_xcflags ${luajit_xcflags} - -I${PROJECT_SOURCE_DIR}/src/lib/small/third_party) - endif() - # AddressSanitizer - CFLAGS were set globaly - if (ENABLE_ASAN) - add_definitions(-DLUAJIT_USE_ASAN=1) - set (luajit_ldflags ${luajit_ldflags} -fsanitize=address) - endif() - add_definitions(-DLUAJIT_SMART_STRINGS=1) - # Add all COMPILE_DEFINITIONS to xcflags - get_property(defs DIRECTORY PROPERTY COMPILE_DEFINITIONS) - foreach(def ${defs}) - set(luajit_xcflags ${luajit_xcflags} -D${def}) - endforeach() +set(LUAJIT_PREFIX ${LUAJIT_BINARY_ROOT}/src) +set(LUAJIT_INCLUDE ${LUAJIT_PREFIX}) +set(LUAJIT_LIB ${LUAJIT_PREFIX}/libluajit.a) - # Pass the same toolchain that is used for building of - # tarantool itself, because tools from different toolchains - # can be incompatible. A compiler and a linker are already set - # above. - set (luajit_ld ${CMAKE_LINKER}) - set (luajit_ar ${CMAKE_AR} rcus) - # Enablibg LTO for luajit if DENABLE_LTO set. - if (ENABLE_LTO) - message(STATUS "Enable LTO for luajit") - set (luajit_ldflags ${luajit_ldflags} ${LDFLAGS_LTO}) - message(STATUS "ld: " ${luajit_ldflags}) - set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO}) - message(STATUS "cflags: " ${luajit_cflags}) - set (luajit_ar ${AR_LTO} rcus) - message(STATUS "ar: " ${luajit_ar}) - endif() - set (luajit_strip ${CMAKE_STRIP}) +add_dependencies(build_bundled_libs libluajit) - set (luajit_buildoptions - BUILDMODE=static - HOST_CC="${luajit_hostcc}" - TARGET_CC="${luajit_cc}" - TARGET_CFLAGS="${luajit_cflags}" - TARGET_LD="${luajit_ld}" - TARGET_LDFLAGS="${luajit_ldflags}" - TARGET_AR="${luajit_ar}" - TARGET_STRIP="${luajit_strip}" - TARGET_SYS="${CMAKE_SYSTEM_NAME}" - CCOPT="${luajit_ccopt}" - CCDEBUG="${luajit_ccdebug}" - XCFLAGS="${luajit_xcflags}" - Q='' - # We need to set MACOSX_DEPLOYMENT_TARGET to at least 10.6, - # because 10.4 SDK (which is set by default in LuaJIT's - # Makefile) is not longer included in Mac OS X Mojave 10.14. - # See also https://github.com/LuaJIT/LuaJIT/issues/484 - MACOSX_DEPLOYMENT_TARGET="${luajit_osx_deployment_target}") - if (${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR}) - add_custom_command(OUTPUT ${PROJECT_BINARY_DIR}/third_party/luajit/src/libluajit.a - WORKING_DIRECTORY ${PROJECT_BINARY_DIR}/third_party/luajit - COMMAND $(MAKE) ${luajit_buildoptions} clean - COMMAND $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a - DEPENDS ${CMAKE_SOURCE_DIR}/CMakeCache.txt - ) - else() - add_custom_command(OUTPUT ${PROJECT_BINARY_DIR}/third_party/luajit - COMMAND ${CMAKE_COMMAND} -E make_directory "${PROJECT_BINARY_DIR}/third_party/luajit" - ) - add_custom_command(OUTPUT ${PROJECT_BINARY_DIR}/third_party/luajit/src/libluajit.a - WORKING_DIRECTORY ${PROJECT_BINARY_DIR}/third_party/luajit - COMMAND ${CMAKE_COMMAND} -E copy_directory ${PROJECT_SOURCE_DIR}/third_party/luajit ${PROJECT_BINARY_DIR}/third_party/luajit - COMMAND $(MAKE) ${luajit_buildoptions} clean - COMMAND $(MAKE) -C src ${luajit_buildoptions} jit/vmdef.lua libluajit.a - DEPENDS ${PROJECT_BINARY_DIR}/CMakeCache.txt ${PROJECT_BINARY_DIR}/third_party/luajit - ) - endif() - add_custom_target(libluajit - DEPENDS ${PROJECT_BINARY_DIR}/third_party/luajit/src/libluajit.a - ) - add_dependencies(build_bundled_libs libluajit) - unset (luajit_buildoptions) - set (inc ${PROJECT_SOURCE_DIR}/third_party/luajit/src) - install (FILES ${inc}/lua.h ${inc}/lualib.h ${inc}/lauxlib.h - ${inc}/luaconf.h ${inc}/lua.hpp ${inc}/luajit.h ${inc}/lmisclib.h - DESTINATION ${MODULE_INCLUDEDIR}) -endmacro() - -# -# Building shipped luajit only if there is no -# usable system one (see cmake/luajit.cmake) or by demand. -# -if (ENABLE_BUNDLED_LUAJIT) - luajit_build() -endif() +install( + FILES + ${LUAJIT_SOURCE_ROOT}/src/lua.h + ${LUAJIT_SOURCE_ROOT}/src/lualib.h + ${LUAJIT_SOURCE_ROOT}/src/lauxlib.h + ${LUAJIT_SOURCE_ROOT}/src/luaconf.h + ${LUAJIT_SOURCE_ROOT}/src/lua.hpp + ${LUAJIT_SOURCE_ROOT}/src/luajit.h + ${LUAJIT_SOURCE_ROOT}/src/lmisclib.h + 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. +# FIXME: Hope everything below will have gone in a future. + +# 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() + +# 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 @@ -/* - * Copyright 2010-2015, Tarantool AUTHORS, please see AUTHORS file. - * - * Redistribution and use in source and binary forms, with or - * without modification, are permitted provided that the following - * conditions are met: - * - * 1. Redistributions of source code must retain the above - * copyright notice, this list of conditions and the - * following disclaimer. - * - * 2. Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following - * disclaimer in the documentation and/or other materials - * provided with the distribution. - * - * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND - * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED - * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL - * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, - * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL - * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF - * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR - * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF - * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF - * SUCH DAMAGE. - */ - -/* - * Find out LuaJIT behavior on the current platform. - * - * LuaJIT uses different stack unwinding mechanisms on 32-bit x86 - * and 64-bit x86-64 hardware: on a 32-bit system it can use - * its own * longjmp-style "internal stack unwinding". - * Among other things, this mechanism doesn't support exception - * propagation from Lua panic function (lua_atpanic()), and - * this is exactly what Tarantool does: throws an exception - * in lua_atpanic(). - * - * Which mechanism to use is determined at library - * compile time, by a set of flags - * (-fexceptions -funwind-tables -DLUAJIT_UNWIND_EXTERNAL), - * hence, when configuring, we can't just check the library file - * to find out whether or not it will work. - * Instead, we compile and run this test. - * - * http://lua-users.org/lists/lua-l/2010-04/msg00470.html - */ - -#include <cstdlib> -#include <lua.hpp> - -static int panic = 0; - -static int lua_panic_cb(lua_State *L) { - if (!panic++) - throw 0; - abort(); - return 0; -} - -int -main(int argc, char * argv[]) -{ - lua_State *L = luaL_newstate(); - if (L == NULL) - return 1; - lua_atpanic(L, lua_panic_cb); - try { - lua_pushstring(L, "uncallable"); - lua_call(L, 0, LUA_MULTRET); - } catch (...) { - /* If we're lucky, we should get here. */ - } - lua_close(L); - return 0; -} diff --git a/debian/control b/debian/control index ce810ee67..d2390e95c 100644 --- a/debian/control +++ b/debian/control @@ -20,6 +20,8 @@ Build-Depends: cdbs (>= 0.4.100), debhelper (>= 9), dpkg-dev (>= 1.16.1~), libicu-dev, # libcurl build dependencies zlib1g-dev, +# Install prove to run LuaJIT tests. + libtest-harness-perl, Section: database Standards-Version: 3.9.8 Homepage: http://tarantool.org/ 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 BuildRequires: python-yaml >= 3.0.9 %endif %endif +# Install prove to run LuaJIT tests. +BuildRequires: perl-Test-Harness Name: tarantool # ${major}.${major}.${minor}.${patch}, e.g. 1.6.8.175 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) lua_source(lua_sources lua/iconv.lua) lua_source(lua_sources lua/swim.lua) # LuaJIT jit.* library -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/bc.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/bcsave.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/dis_x86.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/dis_x64.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/dump.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/vmdef.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/v.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/p.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/src/jit/zone.lua") +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bc.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/bcsave.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dis_x86.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dis_x64.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/dump.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/v.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/p.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/src/jit/zone.lua) +lua_source(lua_sources ${LUAJIT_BINARY_ROOT}/src/jit/vmdef.lua) # LuaJIT tools.* library -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/tools/memprof.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/tools/memprof/humanize.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/tools/memprof/parse.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/tools/utils/bufread.lua") -lua_source(lua_sources "${CMAKE_BINARY_DIR}/third_party/luajit/tools/utils/symtab.lua") +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/tools/memprof.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/tools/memprof/humanize.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/tools/memprof/parse.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/tools/utils/bufread.lua) +lua_source(lua_sources ${LUAJIT_SOURCE_ROOT}/tools/utils/symtab.lua) add_custom_target(generate_lua_sources WORKING_DIRECTORY ${CMAKE_BINARY_DIR}/src/box 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) endif(TARGET_OS_DARWIN) endfunction() -function(build_lualib lib sources) - add_library(${lib} SHARED ${sources}) - set_target_properties(${lib} PROPERTIES PREFIX "") - if(TARGET_OS_DARWIN) - set_target_properties(${lib} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup") - endif(TARGET_OS_DARWIN) -endfunction() - add_compile_flags("C;CXX" "-Wno-unused-parameter") @@ -43,10 +35,6 @@ if(POLICY CMP0037) endif() endif(POLICY CMP0037) -execute_process(COMMAND ${CMAKE_COMMAND} -E create_symlink - ${PROJECT_SOURCE_DIR}/third_party/luajit/test - ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap) - add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/small COMMAND ${CMAKE_COMMAND} -E create_symlink ${PROJECT_SOURCE_DIR}/src/lib/small/test/ @@ -57,14 +45,14 @@ add_custom_target(symlink_small_tests ALL add_custom_target(test DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/small - ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap + LuaJIT-test COMMAND ${PROJECT_SOURCE_DIR}/test/test-run.py --builddir=${PROJECT_BINARY_DIR} --vardir=${PROJECT_BINARY_DIR}/test/var) add_custom_target(test-force DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/small - ${CMAKE_CURRENT_BINARY_DIR}/luajit-tap + LuaJIT-test COMMAND ${PROJECT_SOURCE_DIR}/test/test-run.py --builddir=${PROJECT_BINARY_DIR} --vardir=${PROJECT_BINARY_DIR}/test/var @@ -78,8 +66,6 @@ if(ENABLE_FUZZER) add_subdirectory(fuzz) endif() add_subdirectory(unit) -add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test - ${PROJECT_BINARY_DIR}/third_party/luajit/test) # Disable connector_c for 1.6 #add_subdirectory(connector_c) 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system 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 2021-02-19 23:10 ` Igor Munkin via Tarantool-patches 0 siblings, 1 reply; 19+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2021-02-15 16:13 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system 2021-02-15 16:13 ` Sergey Kaplun via Tarantool-patches @ 2021-02-19 23:10 ` Igor Munkin via Tarantool-patches 2021-02-20 7:42 ` Timur Safin via Tarantool-patches 0 siblings, 1 reply; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-19 23:10 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system 2021-02-19 23:10 ` Igor Munkin via Tarantool-patches @ 2021-02-20 7:42 ` Timur Safin via Tarantool-patches 0 siblings, 0 replies; 19+ messages in thread From: Timur Safin via Tarantool-patches @ 2021-02-20 7:42 UTC (permalink / raw) To: 'Igor Munkin', 'Sergey Kaplun'; +Cc: tarantool-patches : From: Igor Munkin <imun@tarantool.org> : Subject: Re: [PATCH 2/3] build: adjust LuaJIT build system : : Sergey, : : Thanks for your review! : ... : : > > +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. Yup, with function instead of macro it would be very verbose and Unreadable. Macro is the simplest and cleanest way here for their purpose. : : 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> : : : > : > > + "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. Yes, toolchain settings (including compiler settings and build mode) Are cached in CMakeCache.txt and not reassigned from command-line if there is cached value. You have to delete toolchain cached values to make all variables be redefined. If you not delete CMAkeCache.txt you get shorter cmake cycle, which might be not exactly what you hoped to get. : : > : : <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. The more local - the better. Agreed. : : Ignoring. : : > : Timur ^ permalink raw reply [flat|nested] 19+ messages in thread
* [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 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-03 23:22 ` [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 ` Igor Munkin via Tarantool-patches 2021-02-15 16:29 ` Sergey Kaplun via Tarantool-patches 2021-02-24 7:16 ` Timur Safin via Tarantool-patches 2021-02-26 19:09 ` [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Timur Safin via Tarantool-patches ` (2 subsequent siblings) 5 siblings, 2 replies; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-03 23:22 UTC (permalink / raw) To: Sergey Kaplun, Alexander V. Tikhonov; +Cc: tarantool-patches This patch adds LuaJIT tests to every CI job type except the one for static build testing routine on OSX: there is no way to run LuaJIT tests for out of source build on OSX due to SIP[1]. [1]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html Follows up #4862 Signed-off-by: Igor Munkin <imun@tarantool.org> --- .travis.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.travis.mk b/.travis.mk index 50524007c..c2b79a4e5 100644 --- a/.travis.mk +++ b/.travis.mk @@ -134,6 +134,7 @@ build_debian: configure_debian test_debian_no_deps: build_debian cd test && /usr/bin/python test-run.py --force $(TEST_RUN_EXTRA_PARAMS) + make LuaJIT-test test_debian: deps_debian test_debian_no_deps @@ -148,6 +149,7 @@ build_coverage_debian: test_coverage_debian_no_deps: build_coverage_debian # Enable --long tests for coverage cd test && /usr/bin/python test-run.py --force $(TEST_RUN_EXTRA_PARAMS) --long + make LuaJIT-test lcov --compat-libtool --directory src/ --capture --output-file coverage.info.tmp \ --rc lcov_branch_coverage=1 --rc lcov_function_coverage=1 lcov --compat-libtool --remove coverage.info.tmp 'tests/*' 'third_party/*' '/usr/*' \ @@ -213,6 +215,10 @@ test_asan_debian_no_deps: build_asan_debian LSAN_OPTIONS=suppressions=${PWD}/asan/lsan.supp \ ASAN_OPTIONS=heap_profile=0:unmap_shadow_on_exit=1:detect_invalid_pointer_pairs=1:symbolize=1:detect_leaks=1:dump_instruction_bytes=1:print_suppressions=0 \ ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) + ASAN=ON \ + LSAN_OPTIONS=suppressions=${PWD}/asan/lsan.supp \ + ASAN_OPTIONS=heap_profile=0:unmap_shadow_on_exit=1:detect_invalid_pointer_pairs=1:symbolize=1:detect_leaks=1:dump_instruction_bytes=1:print_suppressions=0 \ + make LuaJIT-test test_asan_debian: deps_debian deps_buster_clang_11 test_asan_debian_no_deps @@ -237,6 +243,7 @@ test_static_build_cmake_linux: make -j && ctest -V cd test && /usr/bin/python test-run.py --force \ --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-build $(TEST_RUN_EXTRA_PARAMS) + make -C ${PWD}/static-build/tarantool-prefix/src/tarantool-build LuaJIT-test # ################### # Static Analysis @@ -274,6 +281,7 @@ test_oos_no_deps: build_oos ${OOS_SRC_PATH}/test/test-run.py \ --builddir ${OOS_BUILD_PATH} \ --vardir ${OOS_BUILD_PATH}/test/var --force + make -C ${OOS_BUILD_PATH} LuaJIT-test test_oos: deps_debian test_oos_no_deps @@ -339,6 +347,7 @@ INIT_TEST_ENV_OSX=\ test_osx_no_deps: build_osx ${INIT_TEST_ENV_OSX}; \ cd test && ./test-run.py --vardir ${OSX_VARDIR} --force $(TEST_RUN_EXTRA_PARAMS) + make LuaJIT-test test_osx: deps_osx test_osx_no_deps @@ -362,6 +371,16 @@ test_static_build_cmake_osx: base_deps_osx cd test && ./test-run.py --vardir ${OSX_VARDIR} \ --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool-build \ --force $(TEST_RUN_EXTRA_PARAMS) + # FIXME: Hell with SIP on OSX: Tarantool (and also LuaJIT) + # is built out of sources, so the test located in the + # source directory fails to load the shared library built + # in the binary directory via dlopen(3). + # For more info proceed the link below: + # https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html + # Do not run LuaJIT related tests for this built until the + # issue is not resolved. + # + # make -C ${PWD}/static-build/tarantool-prefix/src/tarantool-build LuaJIT-test ########### @@ -378,6 +397,7 @@ build_freebsd: test_freebsd_no_deps: build_freebsd cd test && python2.7 test-run.py --force $(TEST_RUN_EXTRA_PARAMS) + make LuaJIT-test test_freebsd: deps_freebsd test_freebsd_no_deps -- 2.25.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 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 2021-02-24 7:16 ` Timur Safin via Tarantool-patches 1 sibling, 1 reply; 19+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2021-02-15 16:29 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches Hi, Igor! Thanks for the patch! LGTM, except a few nits below. On 04.02.21, Igor Munkin wrote: > This patch adds LuaJIT tests to every CI job type except the one for > static build testing routine on OSX: there is no way to run LuaJIT tests > for out of source build on OSX due to SIP[1]. > > [1]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html > > Follows up #4862 Nit: looks like 'Closes' not 'Follows up'. Feel free to ignore. > > Signed-off-by: Igor Munkin <imun@tarantool.org> > --- > .travis.mk | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) Nit: There is one general comment for the whole patch: In my opinion, it is better to run `make LuaJIT-tests` **before** Tarantool test suite. If LuaJIT is broken failed Tarantool tests information is useless (Is it LuaJIT? Always has been.). Feel free to ignore. > > diff --git a/.travis.mk b/.travis.mk > index 50524007c..c2b79a4e5 100644 > --- a/.travis.mk > +++ b/.travis.mk > @@ -134,6 +134,7 @@ build_debian: configure_debian <snipped> > + # FIXME: Hell with SIP on OSX: Tarantool (and also LuaJIT) > + # is built out of sources, so the test located in the > + # source directory fails to load the shared library built > + # in the binary directory via dlopen(3). > + # For more info proceed the link below: Typo: s/info, proceed/info proceed/ > + # https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html > + # Do not run LuaJIT related tests for this built until the > + # issue is not resolved. > + # > + # make -C ${PWD}/static-build/tarantool-prefix/src/tarantool-build LuaJIT-test <snipped> > -- > 2.25.0 > -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 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> 0 siblings, 1 reply; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-19 21:29 UTC (permalink / raw) To: Sergey Kaplun; +Cc: tarantool-patches Sergey, Thanks for your review! On 15.02.21, Sergey Kaplun wrote: > Hi, Igor! > > Thanks for the patch! > > LGTM, except a few nits below. Added your tag: | Reviewed-by: Sergey Kaplun <skaplun@tarantool.org> > > On 04.02.21, Igor Munkin wrote: > > This patch adds LuaJIT tests to every CI job type except the one for > > static build testing routine on OSX: there is no way to run LuaJIT tests > > for out of source build on OSX due to SIP[1]. > > > > [1]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html > > > > Follows up #4862 > > Nit: looks like 'Closes' not 'Follows up'. > Feel free to ignore. No, the issue is closed via the previous commit. There is not a word about CI in the issue, so this commit simply follows up the closing one. Ignoring. > > > > > Signed-off-by: Igor Munkin <imun@tarantool.org> > > --- > > .travis.mk | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > Nit: There is one general comment for the whole patch: > In my opinion, it is better to run `make LuaJIT-tests` **before** > Tarantool test suite. If LuaJIT is broken failed Tarantool tests > information is useless (Is it LuaJIT? Always has been.). > Feel free to ignore. I assume the following: there is no flaky tests in LuaJIT (at least now) but there are many in Tarantool. If we place LuaJIT-related tests before Tarantool tests, then we need to rerun the former if the latter fail. But when Tarantool tests fully succeed (even with no test-run internal issues), LuaJIT tests are unlikely to fail. This makes sense when LuaJIT tests will grow in a nearest future. Ignoring for now but let's return to the question if it is necessary. > <snipped> > > -- > Best regards, > Sergey Kaplun -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
[parent not found: <0b6601d7075c$64329060$2c97b120$@tarantool.org>]
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI [not found] ` <0b6601d7075c$64329060$2c97b120$@tarantool.org> @ 2021-02-20 10:18 ` Igor Munkin via Tarantool-patches 0 siblings, 0 replies; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-20 10:18 UTC (permalink / raw) To: Timur Safin; +Cc: tarantool-patches Timur, On 20.02.21, Timur Safin wrote: > : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On > : Subject: Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI > : > : Sergey, > : > : Thanks for your review! > : > Nit: There is one general comment for the whole patch: > : > In my opinion, it is better to run `make LuaJIT-tests` **before** > : > Tarantool test suite. If LuaJIT is broken failed Tarantool tests > : > information is useless (Is it LuaJIT? Always has been.). > : > Feel free to ignore. > : > : I assume the following: there is no flaky tests in LuaJIT (at least now) > : but there are many in Tarantool. If we place LuaJIT-related tests before > : Tarantool tests, then we need to rerun the former if the latter fail. > : But when Tarantool tests fully succeed (even with no test-run internal > : issues), LuaJIT tests are unlikely to fail. This makes sense when LuaJIT > : tests will grow in a nearest future. > : > : Ignoring for now but let's return to the question if it is necessary. > : > > Here I'd rather agree with Sergey, that LuaJIT tests are prerequisite for > all lua testing in Tarantool. Then OK. I reordered the commands, squashed, force-pushed to the branch. > > Timur > -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 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-24 7:16 ` Timur Safin via Tarantool-patches 2021-02-25 22:08 ` Igor Munkin via Tarantool-patches 1 sibling, 1 reply; 19+ messages in thread From: Timur Safin via Tarantool-patches @ 2021-02-24 7:16 UTC (permalink / raw) To: 'Igor Munkin', 'Sergey Kaplun', 'Alexander V. Tikhonov' Cc: TML This 1 thing is the only one that is bother me: it looks dependency of Tarantool LuaJIT tests are not yet properly assigned between build Targets we have. Let me put it how it looks like from my side: - LuaJIT-test is phony, defined in third_party/luajit target and depends on tarantool-tests - tarantool-tests are not phony (i.e. you could not run Tarantool-tests Twice, if you wish to) - third_party/luajit/test is also depending on LuaJIT-luacheck Which is not phone either, and is not runnable if you Run it outside of Tarantool build (because .luacheckrc is At the root of Tarantool repo, not LuaJIT repo). - Also LuaJIT-test is explicitly referred in the travis makefiles And not implicitly called by dependency of Tarantool testing. This bothers me a lot, looks like dependency tree is not working correctly at the moment. Could you please make luacheck and Tarantool-test targets phony, and add LuaJIT-test as prerequisite of Tarantool testing (in cmake)? Thanks, Timur : -----Original Message----- : From: Tarantool-patches <tarantool-patches-bounces@dev.tarantool.org> On : Behalf Of Igor Munkin via Tarantool-patches : Sent: Thursday, February 4, 2021 2:22 AM : To: Sergey Kaplun <skaplun@tarantool.org>; Alexander V. Tikhonov : <avtikhon@tarantool.org> : Cc: tarantool-patches@dev.tarantool.org : Subject: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI : : This patch adds LuaJIT tests to every CI job type except the one for : static build testing routine on OSX: there is no way to run LuaJIT tests : for out of source build on OSX due to SIP[1]. : : [1]: : https://developer.apple.com/library/archive/documentation/Security/Conceptua : l/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtecti : ons.html : : Follows up #4862 : : Signed-off-by: Igor Munkin <imun@tarantool.org> : --- : .travis.mk | 20 ++++++++++++++++++++ : 1 file changed, 20 insertions(+) : : diff --git a/.travis.mk b/.travis.mk : index 50524007c..c2b79a4e5 100644 : --- a/.travis.mk : +++ b/.travis.mk : @@ -134,6 +134,7 @@ build_debian: configure_debian : : test_debian_no_deps: build_debian : cd test && /usr/bin/python test-run.py --force : $(TEST_RUN_EXTRA_PARAMS) : + make LuaJIT-test : : test_debian: deps_debian test_debian_no_deps : : @@ -148,6 +149,7 @@ build_coverage_debian: : test_coverage_debian_no_deps: build_coverage_debian : # Enable --long tests for coverage : cd test && /usr/bin/python test-run.py --force : $(TEST_RUN_EXTRA_PARAMS) --long : + make LuaJIT-test : lcov --compat-libtool --directory src/ --capture --output-file : coverage.info.tmp \ : --rc lcov_branch_coverage=1 --rc lcov_function_coverage=1 : lcov --compat-libtool --remove coverage.info.tmp 'tests/*' : 'third_party/*' '/usr/*' \ : @@ -213,6 +215,10 @@ test_asan_debian_no_deps: build_asan_debian : LSAN_OPTIONS=suppressions=${PWD}/asan/lsan.supp \ : : ASAN_OPTIONS=heap_profile=0:unmap_shadow_on_exit=1:detect_invalid_poin : ter_pairs=1:symbolize=1:detect_leaks=1:dump_instruction_bytes=1:print_suppre : ssions=0 \ : ./test-run.py --force $(TEST_RUN_EXTRA_PARAMS) : + ASAN=ON \ : + LSAN_OPTIONS=suppressions=${PWD}/asan/lsan.supp \ : + : ASAN_OPTIONS=heap_profile=0:unmap_shadow_on_exit=1:detect_invalid_poin : ter_pairs=1:symbolize=1:detect_leaks=1:dump_instruction_bytes=1:print_suppre : ssions=0 \ : + make LuaJIT-test : : test_asan_debian: deps_debian deps_buster_clang_11 test_asan_debian_no_deps : : @@ -237,6 +243,7 @@ test_static_build_cmake_linux: : make -j && ctest -V : cd test && /usr/bin/python test-run.py --force \ : --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool- : build $(TEST_RUN_EXTRA_PARAMS) : + make -C ${PWD}/static-build/tarantool-prefix/src/tarantool-build : LuaJIT-test : : # ################### : # Static Analysis : @@ -274,6 +281,7 @@ test_oos_no_deps: build_oos : ${OOS_SRC_PATH}/test/test-run.py \ : --builddir ${OOS_BUILD_PATH} \ : --vardir ${OOS_BUILD_PATH}/test/var --force : + make -C ${OOS_BUILD_PATH} LuaJIT-test : : test_oos: deps_debian test_oos_no_deps : : @@ -339,6 +347,7 @@ INIT_TEST_ENV_OSX=\ : test_osx_no_deps: build_osx : ${INIT_TEST_ENV_OSX}; \ : cd test && ./test-run.py --vardir ${OSX_VARDIR} --force : $(TEST_RUN_EXTRA_PARAMS) : + make LuaJIT-test : : test_osx: deps_osx test_osx_no_deps : : @@ -362,6 +371,16 @@ test_static_build_cmake_osx: base_deps_osx : cd test && ./test-run.py --vardir ${OSX_VARDIR} \ : --builddir ${PWD}/static-build/tarantool-prefix/src/tarantool- : build \ : --force $(TEST_RUN_EXTRA_PARAMS) : + # FIXME: Hell with SIP on OSX: Tarantool (and also LuaJIT) : + # is built out of sources, so the test located in the : + # source directory fails to load the shared library built : + # in the binary directory via dlopen(3). : + # For more info proceed the link below: : + # : https://developer.apple.com/library/archive/documentation/Security/Conceptua : l/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtecti : ons.html : + # Do not run LuaJIT related tests for this built until the : + # issue is not resolved. : + # : + # make -C ${PWD}/static-build/tarantool-prefix/src/tarantool-build : LuaJIT-test : : : ########### : @@ -378,6 +397,7 @@ build_freebsd: : : test_freebsd_no_deps: build_freebsd : cd test && python2.7 test-run.py --force $(TEST_RUN_EXTRA_PARAMS) : + make LuaJIT-test : : test_freebsd: deps_freebsd test_freebsd_no_deps : : -- : 2.25.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 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 0 siblings, 1 reply; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-25 22:08 UTC (permalink / raw) To: Timur Safin; +Cc: TML Timur, On 24.02.21, Timur Safin wrote: > This 1 thing is the only one that is bother me: it looks dependency of > Tarantool LuaJIT tests are not yet properly assigned between build > Targets we have. > > Let me put it how it looks like from my side: > - LuaJIT-test is phony, defined in third_party/luajit target and > depends on tarantool-tests > - tarantool-tests are not phony (i.e. you could not run Tarantool-tests > Twice, if you wish to) Please, do not take it personally (at least since I've heard such "bothering" from several teammates), but I consider this as a disease. Why do you need to run the *passed* tests twice? To make *more* sure? Kinda Windows-way... Unfortunately, I can't create a dialog window with "Are you sure these tests passed?" message, so I removed the artefacts produced by .PHONY rules you mentioned above. Now you can run <test> and <luacheck> targets as much as you want. > - third_party/luajit/test is also depending on LuaJIT-luacheck > Which is not phone either, and is not runnable if you > Run it outside of Tarantool build (because .luacheckrc is > At the root of Tarantool repo, not LuaJIT repo). This is not true, there is a LuaJIT specific .luacheckrc in LuaJIT repo. The issue you've shared with me relates to the case when <bindir> is located within <srcdir> -- so called "idiomatic build way". Since luacheck is run against all Lua files within <srcdir>, autogenerated jit/vmdef.lua (the single file producing luacheck warnings) is also checked. I explicitly excluded it from the file list to be checked. Now everything is fine. > - Also LuaJIT-test is explicitly referred in the travis makefiles > And not implicitly called by dependency of Tarantool testing. LuaJIT-test is a dependency for Tarantool testing (i.e. <test> target), but Make is not used for running Tarantool tests in CI, so there is no other convenient way than using <make LuaJIT-test> in .travis.mk. I hope this will have been fixed in the future and it is definitely out of the scope of this issue. > > This bothers me a lot, looks like dependency tree is not working > correctly at the moment. Could you please make luacheck and Tarantool-test > targets phony, and add LuaJIT-test as prerequisite of Tarantool > testing (in cmake)? I hope the changes below are enough. Fixed in LuaJIT submodule, squashed, force-pushed to the branch. ================================================================================ diff --git a/.gitignore b/.gitignore index 22921bf..2103a30 100644 --- a/.gitignore +++ b/.gitignore @@ -18,7 +18,5 @@ cmake_install.cmake cmake_uninstall.cmake compile_commands.json install_manifest.txt -luacheck.ok luajit-parse-memprof luajit.pc -tests.ok diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bb0ff91..d166c9d 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -6,33 +6,32 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) find_program(LUACHECK luacheck) if(LUACHECK) set(LUACHECK_RC ${PROJECT_SOURCE_DIR}/.luacheckrc) - set(LUACHECK_OK ${CMAKE_CURRENT_BINARY_DIR}/luacheck.ok) file(GLOB_RECURSE LUACHECK_DEPS ${PROJECT_SOURCE_DIR}/*.lua) - add_custom_command( + add_custom_target(${PROJECT_NAME}-luacheck + DEPENDS ${LUACHECK_RC} ${LUACHECK_DEPS} + ) + add_custom_command(TARGET ${PROJECT_NAME}-luacheck COMMENT "Running luacheck static analysis" - OUTPUT ${LUACHECK_OK} - DEPENDS ${LUACHECK} ${LUACHECK_RC} ${LUACHECK_DEPS} COMMAND ${LUACHECK} ${PROJECT_SOURCE_DIR} --codes --config ${LUACHECK_RC} - && touch ${LUACHECK_OK} - # XXX: Filenames in .luacheckrc are considered relative to - # the working directory, hence luacheck should be run in the - # project root directory. - WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} + # XXX: jit/vmdef.lua is an autogenerated Lua source, so + # there is no need to run luacheck against it. Hence + # explicitly exclude this file from the list for check. + --exclude-files ${LUAJIT_BINARY_DIR}/jit/vmdef.lua + # XXX: Filenames in .luacheckrc are considered relative to + # the working directory, hence luacheck should be run in the + # project root directory. + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} ) else() - add_custom_command( + add_custom_target(${PROJECT_NAME}-luacheck) + add_custom_command(TARGET ${PROJECT_NAME}-luacheck COMMENT "`luacheck' is not found, so ${PROJECT_NAME}-luacheck target is dummy" - OUTPUT luacheck.ok - COMMAND touch luacheck.ok - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) endif() -add_custom_target(${PROJECT_NAME}-luacheck DEPENDS luacheck.ok) - add_subdirectory(tarantool-tests) add_custom_target(${PROJECT_NAME}-test DEPENDS diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt index 52c3007..e9ac62a 100644 --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -72,10 +72,11 @@ file(GLOB TEST_DEPS ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX}) # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list # with dependecies are set in scope of BuildTestLib macro. -add_custom_command( - COMMENT "Running Tarantool tests" - OUTPUT tests.ok +add_custom_target(tarantool-tests DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS} +) +add_custom_command(TARGET tarantool-tests + COMMENT "Running Tarantool tests" COMMAND env LUA_PATH="${LUA_PATH}\;\;" @@ -85,8 +86,6 @@ add_custom_command( --exec ${LUAJIT_TEST_BINARY} --ext ${LUA_TEST_SUFFIX} --failures --shuffle - && touch tests.ok WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) -add_custom_target(tarantool-tests DEPENDS tests.ok) ================================================================================ > > Thanks, > Timur > <snipped> > -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI 2021-02-25 22:08 ` Igor Munkin via Tarantool-patches @ 2021-02-26 19:04 ` Timur Safin via Tarantool-patches 0 siblings, 0 replies; 19+ messages in thread From: Timur Safin via Tarantool-patches @ 2021-02-26 19:04 UTC (permalink / raw) To: 'Igor Munkin'; +Cc: 'TML' Hell, yeah! LGTM! : -----Original Message----- : From: Igor Munkin <imun@tarantool.org> : Sent: Friday, February 26, 2021 1:09 AM : To: Timur Safin <tsafin@tarantool.org> : Cc: 'Sergey Kaplun' <skaplun@tarantool.org>; 'Alexander V. Tikhonov' : <avtikhon@tarantool.org>; TML <tarantool-patches@dev.tarantool.org> : Subject: Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI : : Timur, : : On 24.02.21, Timur Safin wrote: : > This 1 thing is the only one that is bother me: it looks dependency of : > Tarantool LuaJIT tests are not yet properly assigned between build : > Targets we have. : > : > Let me put it how it looks like from my side: : > - LuaJIT-test is phony, defined in third_party/luajit target and : > depends on tarantool-tests : > - tarantool-tests are not phony (i.e. you could not run Tarantool-tests : > Twice, if you wish to) : : Please, do not take it personally (at least since I've heard such : "bothering" from several teammates), but I consider this as a disease. : Why do you need to run the *passed* tests twice? To make *more* sure? : Kinda Windows-way... Unfortunately, I can't create a dialog window with : "Are you sure these tests passed?" message, so I removed the artefacts : produced by .PHONY rules you mentioned above. Now you can run <test> and : <luacheck> targets as much as you want. : : > - third_party/luajit/test is also depending on LuaJIT-luacheck : > Which is not phone either, and is not runnable if you : > Run it outside of Tarantool build (because .luacheckrc is : > At the root of Tarantool repo, not LuaJIT repo). : : This is not true, there is a LuaJIT specific .luacheckrc in LuaJIT repo. : The issue you've shared with me relates to the case when <bindir> is : located within <srcdir> -- so called "idiomatic build way". Since : luacheck is run against all Lua files within <srcdir>, autogenerated : jit/vmdef.lua (the single file producing luacheck warnings) is also : checked. I explicitly excluded it from the file list to be checked. Now : everything is fine. : : > - Also LuaJIT-test is explicitly referred in the travis makefiles : > And not implicitly called by dependency of Tarantool testing. : : LuaJIT-test is a dependency for Tarantool testing (i.e. <test> target), : but Make is not used for running Tarantool tests in CI, so there is no : other convenient way than using <make LuaJIT-test> in .travis.mk. I hope : this will have been fixed in the future and it is definitely out of the : scope of this issue. : : > : > This bothers me a lot, looks like dependency tree is not working : > correctly at the moment. Could you please make luacheck and Tarantool-test : > targets phony, and add LuaJIT-test as prerequisite of Tarantool : > testing (in cmake)? : : I hope the changes below are enough. Fixed in LuaJIT submodule, : squashed, force-pushed to the branch. : : ============================================================================ : ==== : : diff --git a/.gitignore b/.gitignore : index 22921bf..2103a30 100644 : --- a/.gitignore : +++ b/.gitignore : @@ -18,7 +18,5 @@ cmake_install.cmake : cmake_uninstall.cmake : compile_commands.json : install_manifest.txt : -luacheck.ok : luajit-parse-memprof : luajit.pc : -tests.ok : diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt : index bb0ff91..d166c9d 100644 : --- a/test/CMakeLists.txt : +++ b/test/CMakeLists.txt : @@ -6,33 +6,32 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) : find_program(LUACHECK luacheck) : if(LUACHECK) : set(LUACHECK_RC ${PROJECT_SOURCE_DIR}/.luacheckrc) : - set(LUACHECK_OK ${CMAKE_CURRENT_BINARY_DIR}/luacheck.ok) : file(GLOB_RECURSE LUACHECK_DEPS ${PROJECT_SOURCE_DIR}/*.lua) : - add_custom_command( : + add_custom_target(${PROJECT_NAME}-luacheck : + DEPENDS ${LUACHECK_RC} ${LUACHECK_DEPS} : + ) : + add_custom_command(TARGET ${PROJECT_NAME}-luacheck : COMMENT "Running luacheck static analysis" : - OUTPUT ${LUACHECK_OK} : - DEPENDS ${LUACHECK} ${LUACHECK_RC} ${LUACHECK_DEPS} : COMMAND : ${LUACHECK} ${PROJECT_SOURCE_DIR} : --codes : --config ${LUACHECK_RC} : - && touch ${LUACHECK_OK} : - # XXX: Filenames in .luacheckrc are considered relative to : - # the working directory, hence luacheck should be run in the : - # project root directory. : - WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} : + # XXX: jit/vmdef.lua is an autogenerated Lua source, so : + # there is no need to run luacheck against it. Hence : + # explicitly exclude this file from the list for check. : + --exclude-files ${LUAJIT_BINARY_DIR}/jit/vmdef.lua : + # XXX: Filenames in .luacheckrc are considered relative to : + # the working directory, hence luacheck should be run in the : + # project root directory. : + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} : ) : else() : - add_custom_command( : + add_custom_target(${PROJECT_NAME}-luacheck) : + add_custom_command(TARGET ${PROJECT_NAME}-luacheck : COMMENT "`luacheck' is not found, so ${PROJECT_NAME}-luacheck target is : dummy" : - OUTPUT luacheck.ok : - COMMAND touch luacheck.ok : - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} : ) : endif() : : -add_custom_target(${PROJECT_NAME}-luacheck DEPENDS luacheck.ok) : - : add_subdirectory(tarantool-tests) : : add_custom_target(${PROJECT_NAME}-test DEPENDS : diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool- : tests/CMakeLists.txt : index 52c3007..e9ac62a 100644 : --- a/test/tarantool-tests/CMakeLists.txt : +++ b/test/tarantool-tests/CMakeLists.txt : @@ -72,10 +72,11 @@ file(GLOB TEST_DEPS : ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX}) : : # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list : # with dependecies are set in scope of BuildTestLib macro. : -add_custom_command( : - COMMENT "Running Tarantool tests" : - OUTPUT tests.ok : +add_custom_target(tarantool-tests : DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS} : +) : +add_custom_command(TARGET tarantool-tests : + COMMENT "Running Tarantool tests" : COMMAND : env : LUA_PATH="${LUA_PATH}\;\;" : @@ -85,8 +86,6 @@ add_custom_command( : --exec ${LUAJIT_TEST_BINARY} : --ext ${LUA_TEST_SUFFIX} : --failures --shuffle : - && touch tests.ok : WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} : ) : : -add_custom_target(tarantool-tests DEPENDS tests.ok) : : ============================================================================ : ==== : : > : > Thanks, : > Timur : > : : <snipped> : : > : : -- : Best regards, : IM ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Igor Munkin via Tarantool-patches ` (2 preceding siblings ...) 2021-02-03 23:22 ` [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI Igor Munkin via Tarantool-patches @ 2021-02-26 19:09 ` 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 5 siblings, 1 reply; 19+ messages in thread From: Timur Safin via Tarantool-patches @ 2021-02-26 19:09 UTC (permalink / raw) To: 'Igor Munkin', 'Sergey Kaplun'; +Cc: tarantool-patches Without newer versions of patchsets it's hard to mark properly the exact patch you actually approve, so putting this the simpler way - I ack it from the patchset cover letter LGTM whole patchset! Regards, Timur : From: Igor Munkin <imun@tarantool.org> : Subject: [PATCH 0/3] Adjust LuaJIT build system : : The first patch of this series fixes the inaccuracy for out of source : build type. If Lua source path given to <lua_source> function is : relative, the output file is generated in the binary directory. At the : same time if the given path to be compiled to *.lua.c is absolute, the : output file is generated in source directory instead of the binary one. : : In scope of the second patch the module for building the bundled LuaJIT : is completely reworked considering the changes made in LuaJIT repo[1]. : : The last patch adds LuaJIT tests to every CI job type except the one for : static build testing routine on OSX: there is no way to run LuaJIT tests : for out of source build on OSX due to SIP[2]. : : [1]: https://lists.tarantool.org/tarantool- : patches/cover.1612291495.git.imun@tarantool.org/T/#t : [2]: : https://developer.apple.com/library/archive/documentation/Security/Conceptua : l/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtecti : ons.html : : Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4862-cmake-full- : ci : Issues: : * https://github.com/tarantool/tarantool/issues/4862 : * https://github.com/tarantool/tarantool/issues/5470 : : CI is not green since OSX 11 is pretty stormy today. Hope it will be : fine the day when the changeset is merged into the trunk. : : @ChangeLog: : * Port LuaJIT build system to CMake and make its testing environment : self-sufficient (gh-4862, gh-5470). : : Igor Munkin (3): : build: fix lua.c file generation : build: adjust LuaJIT build system : ci: enable LuaJIT tests in CI : : .luacheckrc | 1 - : .travis.mk | 20 +++ : CMakeLists.txt | 2 +- : cmake/luajit.cmake | 375 ++++++++++++-------------------------------- : cmake/luatest.cpp | 80 ---------- : cmake/utils.cmake | 6 +- : debian/control | 2 + : rpm/tarantool.spec | 2 + : src/CMakeLists.txt | 28 ++-- : test/CMakeLists.txt | 18 +-- : third_party/luajit | 2 +- : 11 files changed, 143 insertions(+), 393 deletions(-) : delete mode 100644 cmake/luatest.cpp : : -- : 2.25.0 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 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 0 siblings, 0 replies; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-26 21:06 UTC (permalink / raw) To: Timur Safin; +Cc: tarantool-patches Timur, Thanks a lot for your review! On 26.02.21, Timur Safin wrote: > Without newer versions of patchsets it's hard to mark > properly the exact patch you actually approve, so putting > this the simpler way - I ack it from the patchset cover letter > > LGTM whole patchset! Added your tag to all patches: | Reviewed-by: Timur Safin <tsafin@tarantool.org> > > Regards, > Timur > <snipped> > -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Igor Munkin via Tarantool-patches ` (3 preceding siblings ...) 2021-02-26 19:09 ` [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Timur Safin via Tarantool-patches @ 2021-02-27 13:56 ` Sergey Kaplun via Tarantool-patches 2021-02-28 22:05 ` Igor Munkin via Tarantool-patches 5 siblings, 0 replies; 19+ messages in thread From: Sergey Kaplun via Tarantool-patches @ 2021-02-27 13:56 UTC (permalink / raw) To: Igor Munkin; +Cc: tarantool-patches Igor, I'll borrow Timur's hack: LGTM whole patchset! -- Best regards, Sergey Kaplun ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Igor Munkin via Tarantool-patches ` (4 preceding siblings ...) 2021-02-27 13:56 ` Sergey Kaplun via Tarantool-patches @ 2021-02-28 22:05 ` Igor Munkin via Tarantool-patches 5 siblings, 0 replies; 19+ messages in thread From: Igor Munkin via Tarantool-patches @ 2021-02-28 22:05 UTC (permalink / raw) To: Sergey Kaplun, Timur Safin; +Cc: tarantool-patches I've checked the patchset into 1.10, 2.6, 2.7 and master. On 04.02.21, Igor Munkin wrote: > The first patch of this series fixes the inaccuracy for out of source > build type. If Lua source path given to <lua_source> function is > relative, the output file is generated in the binary directory. At the > same time if the given path to be compiled to *.lua.c is absolute, the > output file is generated in source directory instead of the binary one. > > In scope of the second patch the module for building the bundled LuaJIT > is completely reworked considering the changes made in LuaJIT repo[1]. > > The last patch adds LuaJIT tests to every CI job type except the one for > static build testing routine on OSX: there is no way to run LuaJIT tests > for out of source build on OSX due to SIP[2]. > > [1]: https://lists.tarantool.org/tarantool-patches/cover.1612291495.git.imun@tarantool.org/T/#t > [2]: https://developer.apple.com/library/archive/documentation/Security/Conceptual/System_Integrity_Protection_Guide/FileSystemProtections/FileSystemProtections.html > > Branch: https://github.com/tarantool/tarantool/tree/imun/gh-4862-cmake-full-ci > Issues: > * https://github.com/tarantool/tarantool/issues/4862 > * https://github.com/tarantool/tarantool/issues/5470 > > CI is not green since OSX 11 is pretty stormy today. Hope it will be > fine the day when the changeset is merged into the trunk. > > @ChangeLog: > * Port LuaJIT build system to CMake and make its testing environment > self-sufficient (gh-4862, gh-5470). > > Igor Munkin (3): > build: fix lua.c file generation > build: adjust LuaJIT build system > ci: enable LuaJIT tests in CI > <snipped> > > -- > 2.25.0 > -- Best regards, IM ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-02-28 22:05 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system 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
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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox