Re: [Tarantool-patches] [PATCH v2 luajit 02/30] test: add compiling for C libs from PUC-Rio-Lua5.1

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

Igor,

Thanks for the review!

On 31.03.21, Igor Munkin wrote:
> Sergey,
> 
> Thanks for the patch! LGTM, except the nits below.
> 
> I propose the following rewording for commit subject:
> | test: build auxiliary C libs from PUC-Rio-Lua5.1

Applied, thanks.

> 
> On 26.03.21, Sergey Kaplun wrote:
> > This patch adds commands to create additional LuaC libraries for tests
> 
> Minor: these are not "commands" but "rules".

Fixed.

> 
> > in <attrib.lua>. Also, it renames `luaL_reg` to `luaL_Reg` in <lib1.c>
> > and <lib2.c> to be consistent with the current LuaJIT's LuaC API.
> > 
> > Part of tarantool/tarantool#5845
> > Part of tarantool/tarantool#4473
> > ---
> >  test/PUC-Lua-5.1-tests/CMakeLists.txt      |  3 +-
> >  test/PUC-Lua-5.1-tests/libs/CMakeLists.txt | 48 +++++++++++++++++++++-
> >  test/PUC-Lua-5.1-tests/libs/lib1.c         |  2 +-
> >  test/PUC-Lua-5.1-tests/libs/lib2.c         |  2 +-
> >  4 files changed, 51 insertions(+), 4 deletions(-)
> > 
> > diff --git a/test/PUC-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Lua-5.1-tests/CMakeLists.txt
> > index 773db0d..3c31aae 100644
> > --- a/test/PUC-Lua-5.1-tests/CMakeLists.txt
> > +++ b/test/PUC-Lua-5.1-tests/CMakeLists.txt
> > @@ -16,7 +16,8 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
> >  # variable as proposed in the first case.
> >  set(LUA_PATH "?\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua")
> 
> Side note: I see no LUA_CPATH, but I grepped the spots where lib*.so are
> required and found that package.cpath is tweaked right there. Hence
> LUA_CPATH is excess, isn't it?

Yep. Also, `LUA_CPATH` isn't mentioned in suite's README.

> 
> >  
> 
> <snipped>
> 
> > diff --git a/test/PUC-Lua-5.1-tests/libs/CMakeLists.txt b/test/PUC-Lua-5.1-tests/libs/CMakeLists.txt
> > index f24e7f3..aa64a44 100644
> > --- a/test/PUC-Lua-5.1-tests/libs/CMakeLists.txt
> > +++ b/test/PUC-Lua-5.1-tests/libs/CMakeLists.txt
> > @@ -4,11 +4,57 @@
> >  # See the rationale in the root CMakeLists.txt.
> >  cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
> >  
> > +# Build additional C libraries for tests.
> > +macro(BuildTestCLib lib sources)
> 
> <snipped>
> 
> > +  elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux")
> 
> Minor: I personally would strip this option out for all remaining
> systems (i.e. *BSD too), since we have no guarantee the CMake defaults
> won't be broken on other distros sometime. Feel free to ignore (but
> it'll be on your conscience).

I prefer not to do this -- if there is another one system with the same
behaviour, it should be mentioned explicitly here, to avoid questions
in the future.
Ignoring.

> 
> > +    # 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()
> 
> <snipped>
> 
> > +endmacro()
> > +
> > +BuildTestCLib(lib1  lib1.c)
> 
> Typo: excess whitespace.
> 
> > +BuildTestCLib(lib11 lib1.c lib11.c)
> > +BuildTestCLib(lib2  lib2.c)
> 
> Typo: excess whitespace.

Strictly saying they are not excess -- I want to align library
sources for easier reading :). Removed.

> 
> > +BuildTestCLib(lib21 lib2.c lib21.c)
> > +
> > +# Create exact copy of the lib2 library for tests in attrib.lua.
> 
> Typo: s/create exact copy/create the exact copy/.

Fixed.

> 
> > +set(LIB2ORIG "${CMAKE_CURRENT_BINARY_DIR}/lib2${CMAKE_SHARED_LIBRARY_SUFFIX}")
> > +set(LIB2COPY "${CMAKE_CURRENT_BINARY_DIR}/-lib2${CMAKE_SHARED_LIBRARY_SUFFIX}")
> 
> <snipped>
> 
> > -- 
> > 2.31.0
> > 
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun