Re: [Tarantool-patches] [PATCH v2 luajit 02/26] test: prepare lauxilarily libs for LuaJIT-tests

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
Please consider my comments below.
On Mon, Jan 29, 2024 at 01:45:02PM +0300, 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()`. "cpptest" library is
> renamed to "libcpptest" for consistency.
>
> Part of tarantool/tarantool#9398
> ---
>  test/LuaJIT-tests/CMakeLists.txt              | 29 +++++++++++++++++--
>  test/LuaJIT-tests/src/CMakeLists.txt          | 17 +++++++++++
>  .../src/{cpptest.cpp => libcpptest.cpp}       |  4 +--
>  test/LuaJIT-tests/src/{ctest.c => libctest.c} |  4 +--
>  test/LuaJIT-tests/test.lua                    |  7 +++--
>  5 files changed, 52 insertions(+), 9 deletions(-)
>  create mode 100644 test/LuaJIT-tests/src/CMakeLists.txt
>  rename test/LuaJIT-tests/src/{cpptest.cpp => libcpptest.cpp} (96%)
>  rename test/LuaJIT-tests/src/{ctest.c => libctest.c} (99%)
>
> 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
I think something like `LuaJIT-tests-libs` is better name for the
target.
> +)
> +
> +make_lua_path(LUA_CPATH
> +  PATHS
> +  ${CMAKE_CURRENT_BINARY_DIR}/src/?${CMAKE_SHARED_LIBRARY_SUFFIX}
> +)
> +
> +set(LUAJIT_TESTS_ENV
> +  "LUA_CPATH=\"${LUA_CPATH}\""
IINM, outer quotes are redundant here.
> +)
> +
> +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..fb14d6d9
> --- /dev/null
> +++ b/test/LuaJIT-tests/src/CMakeLists.txt
> @@ -0,0 +1,17 @@
> +# 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)
> +  AddTestLib(${lib} ${sources})
> +  list(APPEND TESTLIBS ${lib})
> +endmacro()
See the comment about this macro in the previous patch.
> +
> +# Use `lib` prefix for loading via FFI and `require()`.
> +BuildTestLib(libctest libctest.c)
> +enable_language(CXX)
> +BuildTestLib(libcpptest libcpptest.cpp)
> +
> +add_custom_target(LuaJIT-tests-prepare DEPENDS ${TESTLIBS})
> +
> +# vim: expandtab tabstop=2 shiftwidth=2
Editor configuration should not be there.

> diff --git a/test/LuaJIT-tests/src/cpptest.cpp b/test/LuaJIT-tests/src/libcpptest.cpp
> similarity index 96%
> rename from test/LuaJIT-tests/src/cpptest.cpp
> rename to test/LuaJIT-tests/src/libcpptest.cpp
> index a5893ed6..cdfb5e32 100644
> --- a/test/LuaJIT-tests/src/cpptest.cpp
> +++ b/test/LuaJIT-tests/src/libcpptest.cpp
> @@ -121,9 +121,9 @@ static luaL_Reg ct_funcs[] = {
>  };
>
>  extern "C" {
> -LUA_API int luaopen_cpptest(lua_State *L)
> +LUA_API int luaopen_libcpptest(lua_State *L)
>  {
> -  luaL_register(L, "cpptest", ct_funcs);
> +  luaL_register(L, "libcpptest", ct_funcs);
>    return 1;
>  }
>  }
> diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/libctest.c
> similarity index 99%
> rename from test/LuaJIT-tests/src/ctest.c
> rename to test/LuaJIT-tests/src/libctest.c
> index e99f2306..aa95b57b 100644
> --- a/test/LuaJIT-tests/src/ctest.c
> +++ b/test/LuaJIT-tests/src/libctest.c
> @@ -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..f2450222 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 ~= "libcpptest" then
Let's change the `k` here to a more clear name.
> +      error("Tests should not mutate global state", 3)
> +    end
>    end}
>    local function seal(t)
>      if getmetatable(t) then return end
> --
> 2.43.0
>