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

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

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 31.01.24, Maxim Kokryashkin wrote:
> 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.

Renamed.

===================================================================
diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
index 8ecfe3f6..2c933a11 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -4,7 +4,7 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
 add_subdirectory(src)
 
 add_custom_target(LuaJIT-tests
-  DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-prepare
+  DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-libs
 )
 
 make_lua_path(LUA_CPATH
diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
index 0855088f..2f90da86 100644
--- a/test/LuaJIT-tests/src/CMakeLists.txt
+++ b/test/LuaJIT-tests/src/CMakeLists.txt
@@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c)
 enable_language(CXX)
 AddTestLib(libcpptest libcpptest.cpp)
 
-add_custom_target(LuaJIT-tests-prepare DEPENDS ${TESTLIBS})
+add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS})
===================================================================

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

Without them, CMake just add ${LUA_CPATH} as a string (without internal
quotes). Hence, they are needed.

> > +)
> > +

<snipped>

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

Fixed within the patch below.

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

Fixed: see the iterative patch below.

===================================================================
diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt
index fb14d6d9..0855088f 100644
--- a/test/LuaJIT-tests/src/CMakeLists.txt
+++ b/test/LuaJIT-tests/src/CMakeLists.txt
@@ -1,17 +1,9 @@
 # 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()
-
 # Use `lib` prefix for loading via FFI and `require()`.
-BuildTestLib(libctest libctest.c)
+AddTestLib(libctest libctest.c)
 enable_language(CXX)
-BuildTestLib(libcpptest libcpptest.cpp)
+AddTestLib(libcpptest libcpptest.cpp)
 
 add_custom_target(LuaJIT-tests-prepare DEPENDS ${TESTLIBS})
-
-# vim: expandtab tabstop=2 shiftwidth=2
===================================================================

> 

<snipped>

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

Changed to the `key`:

===================================================================
diff --git a/test/LuaJIT-tests/test.lua b/test/LuaJIT-tests/test.lua
index f2450222..ab178331 100644
--- a/test/LuaJIT-tests/test.lua
+++ b/test/LuaJIT-tests/test.lua
@@ -297,9 +297,9 @@ local function append_tree_to_plan(test_tree, opts, plan, prefix)
 end
 
 local function seal_globals()
-  local sealed_mt = {__newindex = function(_, k)
+  local sealed_mt = {__newindex = function(_, key)
     -- Allow to load C/C++ libraries for the test.
-    if k ~= "libctest" and k ~= "libcpptest" then
+    if key ~= "libctest" and key ~= "libcpptest" then
       error("Tests should not mutate global state", 3)
     end
   end}
===================================================================

> > +      error("Tests should not mutate global state", 3)
> > +    end
> >    end}
> >    local function seal(t)
> >      if getmetatable(t) then return end
> > --
> > 2.43.0
> >

-- 
Best regards,
Sergey Kaplun