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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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

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

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

>  

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

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

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

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