Re: [Tarantool-patches] [PATCH luajit 2/3][v4] test: update CMake macro LibRealPath

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

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

Sergey,

see my comments below


On 3/21/24 18:00, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider the several minor suggestions below.
>
> On 21.03.24, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov<sergeyb@tarantool.org>
>>
>> The patch updates CMake macro LibRealPath:
> Typo: s/CMake/the CMake/

Fixed.


>
>> - Replace CMAKE_CXX_COMPILER with CMAKE_C_COMPILER,
> Nit: This line isn't filled enough.


Fixed.


>
>>    because LuaJIT doesn't require CXX compiler and thus
> Typo: s/compiler/compiler,/
Fixed.
>
>>    it can be unavailable on a build system.
>> - Add a check for a code returned by a command with compiler.
> Typo: s/compiler/the compiler/

Fixed.


>
>> - Add a check for a value returned by compiler, the value equal to
> Typo: s/compiler, the/the compiler. A/

Fixed.


>
>>    passed one means that library was not found in a system.
> Typo: s/passed one/a passed one/
> Typo: s/library/the library/
> Typo: s/a system/the system/

Fixed.


>
>> ---
>>   test/LuaJIT-tests/CMakeLists.txt | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
>> index a0fb5440..f744abfe 100644
>> --- a/test/LuaJIT-tests/CMakeLists.txt
>> +++ b/test/LuaJIT-tests/CMakeLists.txt
>> @@ -33,12 +33,27 @@ if(LUAJIT_USE_ASAN)
>>     #https://github.com/google/sanitizers/issues/934.
>>     macro(LibRealPath output lib)
>>       execute_process(
>> -      COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
>> +      # CMAKE_C_COMPILER is used because it has the same behaviour
>> +      # as CMAKE_CXX_COMPILER, but CMAKE_CXX_COMPILER is not required
> Nit: Comment length is more than 66 symbols.

Fixed.


>
>> +      # for building LuaJIT and can be missed in GH Actions.
>> +      COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
>>         OUTPUT_VARIABLE LIB_LINK
>>         OUTPUT_STRIP_TRAILING_WHITESPACE
>> +      RESULT_VARIABLE RES
>>       )
>> +    # GCC and Clang returns a passed filename,
>> +    # when library was not found.
> Nit: Something strange with comment width. You can use more space from
> the first line.

Fixed.


>
>> +    if (${LIB_LINK} STREQUAL ${lib})
> I suppose, it should be:
>
> | if (LIB_LINK STREQUAL ${lib})
>
> Or we may get the following error if the `LIB_LINK` isn't defined:
>
> | cc: error: unrecognized command-line option '-not-an-option=libstdc++.so'
> | cc: fatal error: no input files
> | compilation terminated.
> | CMake Error at test/LuaJIT-tests/CMakeLists.txt:42 (if):
> |   if given arguments:


Fixed.

-- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -40,7 +40,7 @@ if(LUAJIT_USE_ASAN)
      )
      # GCC and Clang returns a passed filename, when library
      # was not found.
-    if (${LIB_LINK} STREQUAL ${lib})
+    if (LIB_LINK STREQUAL ${lib})
        message(FATAL_ERROR "Library '${lib}' is not found")
      endif()
      if (NOT RES EQUAL 0)


>> +      message(FATAL_ERROR "Library '${lib}' is not found")
>> +    endif()
>> +    if (NOT RES EQUAL 0)
>> +      message(FATAL_ERROR "Executing '${CMAKE_C_COMPILER} \
>> +                           -print-file-name=${lib}' has failed")
> Please reformat this code like the following to be consistent with the
> rest of the code base:
>
> | message(FATAL_ERROR
> |   "Executing '${CMAKE_C_COMPILER} -print-file-name=${lib}' has failed"
> | )
>
>
Fixed.


>> +    endif()
>> +
>>       # Fortunately, we are not interested in macOS here, so we can
>> -    # use realpath.
>> +    # use realpath. Beware, builtin commands always returns
> Typo: s/builtin/built-in/
> Typo: s/returns/return/

Fixed.


>
> Also, why do you call it "built-in comands"? Maybe just name it
> `realpath`.

because realpath is a special case of shell built-in

updated to realpath

>
>> +    # an exit code equal to 0, we cannot check is it failed or not.
> Typo: s/, we/, so we/
> Typo: s/is it failed/if it is failed/

Fixed.


>
>>       execute_process(
>>         COMMAND realpath ${LIB_LINK}
>>         OUTPUT_VARIABLE ${output}
>> -- 
>> 2.34.1
>>

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