Re: [Tarantool-patches] [PATCH luajit 2/3][v2] test: set dependencies to tarantool-tests explicitly

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

Hi, Sergey!
Thanks for the patch!
LGTM, after fixing my comments below.

On 09.11.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> In testsuite `tarantool-tests` we are using CMake macro `BuildTestCLib`

Typo: s/`tarantool-tests`/`tarantool-tests`,/
Typo: s/CMake/the CMake/

> for building test C libraries. This macro builds a list `TESTLIBS` that
> contains all libraries required for tarantool tests. However, CMake
> skips adding C library to `TESTLIBS` on building test C library in the

Typo: s/adding C library/adding the C library/
Nit: s/on/when/
Typo: s/building test C library/building the test C library/

> following commit.
> 
> The patch removes `TESTLIBS` and adds each test C library as dependence

Typo: s/dependence/a dependency/

> to CMake target `tarantool-tests` explicitly using `add_dependence`

Typo: s/using/using the/

> command.
> ---
>  test/tarantool-tests/CMakeLists.txt | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index c15d6037..d46e69a0 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -10,6 +10,10 @@ if(NOT PROVE)
>    return()
>  endif()
>  
> +add_custom_target(tarantool-tests
> +  DEPENDS ${LUAJIT_TEST_BINARY}
> +)
> +
>  macro(BuildTestCLib lib sources)
>    add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources})
>    target_include_directories(${lib} PRIVATE
> @@ -37,12 +41,7 @@ macro(BuildTestCLib lib sources)
>        CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}"
>      )
>    endif()
> -  # XXX: Append the lib to be built to the dependency list.
> -  # Unfortunately, there is no convenient way in CMake to extend
> -  # the list in parent scope other than join two strings with
> -  # semicolon. If one finds the normal way to make it work, feel
> -  # free to reach me.
> -  set(TESTLIBS "${lib};${TESTLIBS}" PARENT_SCOPE)
> +  add_dependencies(tarantool-tests ${lib})
>    # Add the directory where the lib is built to the list with
>    # entries for LUA_CPATH environment variable, so LuaJIT can find
>    # and load it. See the comment about extending the list in the
> @@ -133,11 +132,6 @@ else()
>    list(APPEND LUA_TEST_ENV_MORE LD_LIBRARY_PATH=${LD_LIBRARY_PATH})
>  endif()
>  
> -# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list

I suppose that information about LUA_PATH and LD_LIBRARY_PATH may stay.

> -# with dependecies are set in scope of BuildTestLib macro.
> -add_custom_target(tarantool-tests
> -  DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> -)
>  add_custom_command(TARGET tarantool-tests
>    COMMENT "Running Tarantool tests"
>    COMMAND
> @@ -154,4 +148,3 @@ add_custom_command(TARGET tarantool-tests
>        ${LUA_TEST_FLAGS}
>    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>  )
> -
> -- 
> 2.34.1
> 

-- 
Best regards,
Sergey Kaplun