Hi!

Thanks for the patch!

LGTM.

Sergos

> On 16 Mar 2021, at 15:02, Sergey Kaplun <skaplun@tarantool.org> wrote:
> 
> Igor,
> 
> Thanks for the feedback!
> 
> On 16.03.21, Igor Munkin wrote:
>> Sergey,
>> 
>> Thanks for the fixes!
>> 
>> On 16.03.21, Sergey Kaplun wrote:
>>> Igor,
>>> 
>>> On 16.03.21, Igor Munkin wrote:
>>>> Sergey,
>>>> 
>> 
>> <snipped>
>> 
>>>> 
>>>>>>> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..9b35e5a
>>>>>>> --- /dev/null
>>>>>>> +++ b/test/lua-Harness-tests/CMakeLists.txt
>>>>>>> @@ -0,0 +1,49 @@
>>>>>>> +# Test suite that has been added from lua-Harness test suite
>>>>>>> +# in scope of https://github.com/tarantool/tarantool/issues/4473.
>>>>>>> +
>>>>>>> +# See the rationale in the root CMakeLists.txt
>>>>>>> +cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
>>>>>>> +
>>>>>>> +find_program(PROVE prove)
>>>>>>> +if(NOT PROVE)
>>>>>>> +  message(WARNING "`prove' is not found, so lua-Harness-tests target is not generated")
>>>>>>> +  return()
>>>>>>> +endif()
>>>>>>> +
>>>>>>> +set(LUA_TEST_FLAGS --failures --shuffle)
>>>>>> 
>>>>>> Why did you drop TEST_DEPS variable containing the dependencies?
>>>>> 
>>>>> Sorry, don't get it. What do you mean?
>>>> 
>>>> If you look onto my iterative patch in the first review iteration,
>>>> you'll find TEST_DEPS variable similar to the one in tarantool-tests
>>>> CMakeLists.txt. I forgot to add it into CMakeLists.txt for LuaJIT-tests
>>>> and remembered about it in this patch.
>>> 
>>> Sorry, I missed it while looks through your patch -- I supposed that
>>> it does two things:
>>> * removes tap renaming
>>> * removes list of test usage, as far as 5040 is resolved
>> 
>> Yeah, that's my fault.
>> 
>>> 
>>> For now I don't understand, why do we need this TEST_DEPS?
>>> What is its mission?
>> 
>> To control the target dependencies: the binary and test chunks. However,
>> it was required when *-tests targets were not .PHONY, and now it's more
>> for self-check that nothing global is broken.
> 
> OK, looks like we came to agreement offline so I just repeate this here:
> 
> * All test chunks are `.PHONY`, so all tests will rerun always,
>  even without test sources changing. That's why we need only build
>  dependencies here.
> * We are planing to make all test suites more consistent,
>  so TEST_DEPS will be removed from Tarantool's suite too later.
> 
>> 
>>> 
>>>> 
>>>>> 
>>>>>> 
>>>>>>> +if(CMAKE_VERBOSE_MAKEFILE)
>>>>>>> +  list(APPEND LUA_TEST_FLAGS --verbose)
>>>>>>> +endif()
>>>>>>> +
>>>>>>> +string(CONCAT LUA_PATH
>>>>>>> +  "./?.lua\;"
>>>>>>> +  "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;"
>>>>>>> +  "${LUAJIT_SOURCE_DIR}/?.lua\;"
>>>>>>> +)
>>>>>> 
>>>>>> There is not a word regarding such complex LUA_PATH configuration.
>>>>> 
>>>>> Ok, it can be the one long string, but, in my opinion, this format is
>>>>> more readable, plus, as a bonus, with it line length is less than 120
>>>>> characters. I'll join these lines into one if you insist.
>>>> 
>>>> This is not the issue (and might be much better that my approach in
>>>> tarantool-tests and I will patch the test runners to make it conform to
>>>> one style later). I'm talking about the purpose for each entry. E.g. why
>>>> do you need both "./?.lua" and "${CMAKE_CURRENT_SOURCE_DIR}/?.lua"?  If
>>>> you need "${LUAJIT_SOURCE_DIR}/?.lua" for jit.* modules, then why it's
>>>> not required for other suites? Mention this explicitly, please.
>>> 
>>> Got it now, thanks!
>>> Looks like it really is not necessary. See the iterative patch below:
>>> Checked result in CI -- it is green, except known freebsd issue.
>>> 
>>> ===================================================================
>>> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
>>> index b844788..fd5c8ce 100644
>>> --- a/test/lua-Harness-tests/CMakeLists.txt
>>> +++ b/test/lua-Harness-tests/CMakeLists.txt
>>> @@ -15,16 +15,7 @@ if(CMAKE_VERBOSE_MAKEFILE)
>>>   list(APPEND LUA_TEST_FLAGS --verbose)
>>> endif()
>>> 
>>> -string(CONCAT LUA_PATH
>>> -  "./?.lua\;"
>>> -  "${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;"
>>> -  "${LUAJIT_SOURCE_DIR}/?.lua\;"
>>> -)
>>> -
>>> -string(CONCAT LUA_CPATH
>>> -  "./?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
>>> -  "${LUAJIT_SOURCE_DIR}/?${CMAKE_SHARED_LIBRARY_SUFFIX}\;"
>>> -)
>>> +set(LUA_PATH "./?.lua\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;")
>> 
>> Still not a word, why do you need both "./?.lua" and
>> "${CMAKE_CURRENT_SOURCE_DIR}/?.lua".
> 
> Sorry, add corresponding comment:
> ===================================================================
> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt
> index a639553..336dc03 100644
> --- a/test/lua-Harness-tests/CMakeLists.txt
> +++ b/test/lua-Harness-tests/CMakeLists.txt
> @@ -15,6 +15,9 @@ if(CMAKE_VERBOSE_MAKEFILE)
>   list(APPEND LUA_TEST_FLAGS --verbose)
> endif()
> 
> +# Tests create temporary files (see 303-package.t for example)
> +# to require. Also, they require some files from original
> +# test source directory.
> set(LUA_PATH "./?.lua\;${CMAKE_CURRENT_SOURCE_DIR}/?.lua\;")
> 
> add_custom_target(lua-Harness-tests DEPENDS ${LUAJIT_TEST_BINARY})
> ===================================================================
> 
>> 
>>> 
>>> add_custom_target(lua-Harness-tests DEPENDS ${LUAJIT_TEST_BINARY})
>>> 
>>> @@ -33,7 +24,6 @@ add_custom_command(TARGET lua-Harness-tests
>>>   COMMAND
>>>   env
>>>     LUA_PATH="${LUA_PATH}\;"
>>> -    LUA_CPATH="${LUA_CPATH}\;"
>>>     # XXX: 309-os.t checks os.getenv() function by examining of
>>>     # USERNAME or LOGNAME environment variable.
>>>     # These variables might not be set in the environment, so
>>> ===================================================================
>>> 
>> 
>> <snipped>
>> 
>>> 
>>> -- 
>>> Best regards,
>>> Sergey Kaplun
>> 
>> -- 
>> Best regards,
>> IM
> 
> -- 
> Best regards,
> Sergey Kaplun