From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id E722CBC3C07; Fri, 22 Mar 2024 15:34:49 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org E722CBC3C07 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1711110890; bh=ptqRIVDUgNr66lAbu+DcQOojJI8DK4kjEbU4q6N9D2M=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=YmgiA1fDDfJGQwuTx6foIcMdVIjGYD2yvbMKIbxFXRaSmLx3WPiKDEVp4Kw/gy1gW INNqYmCkA3HKflHIC9UQJ2F8IqZMab1dHauZYnTY/k9Yb+1haK1nqL6qtuDguyvX/6 njVPBq1H5CweVXTwAbVkME7wpy2F39gus9A5zasU= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [95.163.41.84]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 6DE31461D81 for ; Fri, 22 Mar 2024 15:34:48 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6DE31461D81 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1rne6h-0000000DMYY-2BcT; Fri, 22 Mar 2024 15:34:48 +0300 Content-Type: multipart/alternative; boundary="------------dhVSEQ7xdDEO0TYLw1r8qT6G" Message-ID: <87282b52-0b70-4c93-b8cd-22d50ebb5c1a@tarantool.org> Date: Fri, 22 Mar 2024 15:34:47 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun References: <6179514206d7197de2097bed75aa46862cb4e651.1711029149.git.sergeyb@tarantool.org> In-Reply-To: X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92CB91DAA594FB6BF5D42B8B457FE10AC08FDFA69BF118220182A05F538085040F2A9DE9DF9BE79DFAC8EDD30083ED68ECEE36E02B470BD62214AD56D2724CCC302D7B2802C7B9C8A X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7227E4400968B082FEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637F72512A705E00D288638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D85A96B55A07BE586C8E818C5422F971121DD9362171005649CC7F00164DA146DAFE8445B8C89999728AA50765F79006370BDB19F53EE528DD389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC878444BBB7636F62AF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CF254576263B31EA9AD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C300FD323903581960BA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CF27ED053E960B195E1DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C32695002DD8BDA04435872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5EF9E251F704270D05002B1117B3ED6963CD6D1FF0635E281484B8D70797403F6823CB91A9FED034534781492E4B8EEAD9CFA8CFAC159CE19BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34324A9840C798D5001F79606663CEE4A9905267142CE7F22EC80D82133C57288CF72657DE18AB60651D7E09C32AA3244CF38FA95A36834676459F46583F07963C97E5376654E6F69DEA455F16B58544A21C197AAF4D2E4732A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxpsVEd/A9aNt5DU+gWmZhQ== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B596A3DC191E7BF46A9AC8EDD30083ED68ECEE36E02B470BD62B7CBEF92542CD7C8795FA72BAB74744FC77752E0C033A69EA16A481184E8BB1C9B38E6EA4F046BE03A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit 2/3][v4] test: update CMake macro LibRealPath X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" This is a multi-part message in MIME format. --------------dhVSEQ7xdDEO0TYLw1r8qT6G Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 >> >> 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 >> --------------dhVSEQ7xdDEO0TYLw1r8qT6G Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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



      

      

    

  


--------------dhVSEQ7xdDEO0TYLw1r8qT6G--