Re: [Tarantool-patches] [PATCH v2 luajit 5/7] test: shrink LUA_CPATH and {DY}LD_LIBRARY_PATH

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

[-- Attachment #1: Type: text/plain, Size: 2760 bytes --]

Thanks, LGTM.

On 24.09.2024 14:28, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comment and force-pushed the branch.
>
> On 24.09.24, Sergey Bronnikov wrote:
>> Hi, Sergey,
>>
>> thanks for the updated patch!
>>
>> I'm happy to see how much code was removed.
>>
>> See my comments below.
>>
>> On 24.09.2024 13:29, Sergey Kaplun wrote:
>>> This patch sets the unique value of each of these variables for each of
>>> the tests and only where they are needed. Also, it drops the comment
>>> about SIP [1] tricks since it is obsolete after
>>> 29897567ee5ed57e961c730432c056a3dbaa8f09 ("test: stop using
>>> utils.selfrun in tests").
>>>
>>> [1]:https://support.apple.com/en-us/HT204899
>>>
>>> Resolves tarantool/tarantool#9898
>>> ---
> <snipped>
>
>>> +
>>> +  if(LIBS_${test_name})
>>> +    foreach(path ${LIBS_${test_name}})
>>> +      # Add the directory where the library is built to the list
>>> +      # with entries for the LUA_CPATH environment variable, so
>>> +      # LuaJIT can find and load it.
>>> +      AppendTestEnvVar(${test_title}
>>> +        LUA_CPATH "${path}/?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
>>> +      )
>>> +      # Also, add this directory to the LD_LIBRARY_PATH environment
>>> +      # variable so FFI machinery can find and load it.
>>> +      # XXX: Be noticed that we shouldn't use `"` here to wrap the
>>> +      # variable's content. If we do this, the variable value will
>>> +      # contain `"` at the beginning and the end, so this `"` at the
>>> +      # beginning will be treated as the directory for the entry.
>>> +      # XXX: Since the auxiliary libraries are built as dynamically
>>> +      # loaded modules on MacOS instead of shared libraries as it is
>>> +      # done on Linux and BSD, another environment variable should
>>> +      # be used to guide <ffi.load> while searching the extension.
>>> +      if(CMAKE_SYSTEM_NAME STREQUAL "Darwin")
>>> +        AppendTestEnvVar(${test_title} DYLD_LIBRARY_PATH ${path}:)
>>> +      else()
>>> +        AppendTestEnvVar(${test_title} LD_LIBRARY_PATH ${path}:)
>>> +      endif()
>> please add unset(LIBS_${test_name})
> ===================================================================
> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
> index 74086ca4..297d5801 100644
> --- a/test/tarantool-tests/CMakeLists.txt
> +++ b/test/tarantool-tests/CMakeLists.txt
> @@ -146,6 +146,8 @@ foreach(test_path ${tests})
>           AppendTestEnvVar(${test_title} LD_LIBRARY_PATH ${path}:)
>         endif()
>       endforeach()
> +
> +    unset(LIBS_${test_name})
>     endif()
>   endforeach()
>   
> ===================================================================
>
>>> +    endforeach()
> <snipped>
>

[-- Attachment #2: Type: text/html, Size: 3797 bytes --]