Sergey,
see my comments below
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.
Fixed.because LuaJIT doesn't require CXX compiler and thusTypo: 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/
Fixed.
- Add a check for a value returned by compiler, the value equal toTypo: 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 requiredNit: 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 returnsTypo: 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