Re: [Tarantool-patches] [PATCH luajit 01/25] test: prepare lauxilarily libs for LuaJIT-tests

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

Hi, Sergey!

thanks for the patch! see my comments below

Sergey

On 1/19/24 14:32, Sergey Kaplun wrote:
> This patch adds rules to build C and C++ libs from the
> <LuaJIT-tests/src/> directory. Also, it allows these libraries to be
> loaded via `require()` (since `luaL_openlib()` evolves global state).
> The name of "ctest" library is changed to "libctest" to allow it to be
> loaded via both `ffi.load()` and `require()`.
>
> Part of tarantool/tarantool#9398
> ---
>   test/LuaJIT-tests/CMakeLists.txt     | 29 +++++++++++++++++---
>   test/LuaJIT-tests/src/CMakeLists.txt | 40 ++++++++++++++++++++++++++++
>   test/LuaJIT-tests/src/ctest.c        |  4 +--
>   test/LuaJIT-tests/test.lua           |  7 +++--
>   4 files changed, 73 insertions(+), 7 deletions(-)
>   create mode 100644 test/LuaJIT-tests/src/CMakeLists.txt
>
> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
> index 9cd76ee9..c52bcc71 100644
> --- a/test/LuaJIT-tests/CMakeLists.txt
> +++ b/test/LuaJIT-tests/CMakeLists.txt
> @@ -1,12 +1,35 @@
>   # See the rationale in the root CMakeLists.txt
>   cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>   
> -add_custom_target(LuaJIT-tests DEPENDS ${LUAJIT_TEST_BINARY})
> +add_subdirectory(src)
> +
> +add_custom_target(LuaJIT-tests
> +  DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-prepare
> +)
> +
> +make_lua_path(LUA_CPATH
> +  PATHS
> +  ${CMAKE_CURRENT_BINARY_DIR}/src/?${CMAKE_SHARED_LIBRARY_SUFFIX}
> +)
> +
> +set(LUAJIT_TESTS_ENV
> +  "LUA_CPATH=\"${LUA_CPATH}\""
> +)
> +
> +set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:")
> +
> +if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +  list(APPEND LUAJIT_TESTS_ENV DYLD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +else()
> +  list(APPEND LUAJIT_TESTS_ENV LD_LIBRARY_PATH="${LD_LIBRARY_PATH}")
> +endif()
>   
>   add_custom_command(TARGET LuaJIT-tests
>     COMMENT "Running LuaJIT-tests"
>     COMMAND
> -    ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> -    +slow +ffi +bit +jit
> +    env
> +      ${LUAJIT_TESTS_ENV}
> +      ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua
> +      +slow +ffi +bit +jit
>     WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
>   )
> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
> new file mode 100644
> index 00000000..f07b3b6d
> --- /dev/null
> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
> @@ -0,0 +1,40 @@
> +# See the rationale in the root CMakeLists.txt.
> +cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
> +
> +# Build additional C/C++ libraries for tests.
> +macro(BuildTestLib lib sources)
> +  add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
> +  target_include_directories(${lib} PRIVATE
> +    ${LUAJIT_SOURCE_DIR}
> +  )
> +  set_target_properties(${lib} PROPERTIES
> +    LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> +    PREFIX ""
> +  )
> +  # XXX: The dynamic libraries are loaded with LuaJIT binary and
> +  # use symbols from it. So it is totally OK to have unresolved
> +  # symbols at build time.
> +  if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
> +    set_target_properties(${lib} PROPERTIES
> +      LINK_FLAGS "-undefined dynamic_lookup"
> +    )
> +  elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
> +    # XXX: This is necessary mostly for openSUSE builds, see also
> +    # https://bugzilla.suse.com/show_bug.cgi?id=1012388.
> +    # Just strip out the linker flag to suppress this linker
> +    # option.
> +    string(REPLACE "-Wl,--no-undefined" ""
> +      CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}"
> +    )
> +  endif()
> +  list(APPEND TESTLIBS ${lib})
> +endmacro()
> +

This macro duplicates the similar macro in BuildTestCLib in 
tarantool-tests suite.

Why you don't want to generalize it and reuse in both suites?

> +# Use `lib` prefix for loading via FFi and `require()`.
Typo: FFi -> FFI
> +BuildTestLib(libctest ctest.c)
> +enable_language(CXX)
> +BuildTestLib(cpptest cpptest.cpp)
> +
> +add_custom_target(LuaJIT-tests-prepare DEPENDS ${TESTLIBS})
> +
> +# vim: expandtab tabstop=2 shiftwidth=2
> diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/ctest.c
> index e99f2306..aa95b57b 100644
> --- a/test/LuaJIT-tests/src/ctest.c
> +++ b/test/LuaJIT-tests/src/ctest.c
I would rename file to libctest.c or ltest.c (like Roberto do) to 
highlight that it is a library.
> @@ -332,8 +332,8 @@ static luaL_Reg ct_funcs[] = {
>     {NULL, NULL}
>   };
>   
> -LUA_API int luaopen_ctest(lua_State *L)
> +LUA_API int luaopen_libctest(lua_State *L)
>   {
> -  luaL_register(L, "ctest", ct_funcs);
> +  luaL_register(L, "libctest", ct_funcs);
>     return 1;
>   }
> diff --git a/test/LuaJIT-tests/test.lua b/test/LuaJIT-tests/test.lua
> index b064eff7..749a27e7 100644
> --- a/test/LuaJIT-tests/test.lua
> +++ b/test/LuaJIT-tests/test.lua
> @@ -297,8 +297,11 @@ local function append_tree_to_plan(test_tree, opts, plan, prefix)
>   end
>   
>   local function seal_globals()
> -  local sealed_mt = {__newindex = function()
> -    error("Tests should not mutate global state", 3)
> +  local sealed_mt = {__newindex = function(_, k)
> +    -- Allow to load C/C++ libraries for the test.
> +    if k ~= "libctest" and k ~= "cpptest" then
> +      error("Tests should not mutate global state", 3)
> +    end
>     end}
>     local function seal(t)
>       if getmetatable(t) then return end