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

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

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/

> 
> - Replace CMAKE_CXX_COMPILER with CMAKE_C_COMPILER,

Nit: This line isn't filled enough.

>   because LuaJIT doesn't require CXX compiler and thus

Typo: s/compiler/compiler,/

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

> - Add a check for a value returned by compiler, the value equal to

Typo: s/compiler, the/the compiler. A/

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

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

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

> +    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:

> +      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"
| )


> +    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/

Also, why do you call it "built-in comands"? Maybe just name it
`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/

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

-- 
Best regards,
Sergey Kaplun