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

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

Hello, Sergey


please see my answers below

Updates applied and force-pushed.


On 11/13/23 10:10, Sergey Kaplun wrote:
> 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/
Fixed.
>
>> 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/
Fixed.
>
>> following commit.
>>
>> The patch removes `TESTLIBS` and adds each test C library as dependence
> Typo: s/dependence/a dependency/
Fixed.
>
>> to CMake target `tarantool-tests` explicitly using `add_dependence`
> Typo: s/using/using the/
Fixed.
>
>> 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.

Reverted the hunk:


--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -132,6 +132,8 @@ 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
+# with dependecies are set in scope of BuildTestLib macro.
  add_custom_command(TARGET tarantool-tests
    COMMENT "Running Tarantool tests"
    COMMAND

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