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 88DFCA5926B; Thu, 21 Mar 2024 18:04:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 88DFCA5926B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1711033482; bh=TdskNhFWK8MrTa8H6jeSxAhWZy5M4Ni/vvIknDr4AAg=; 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=VJck5jJQSGLZWhvKQRSVCEe79olRKanFIzbS0mTltVepl8uoU1BTMv53QT1J6nVCD ZQQf716/Qb0nmQuZiZTvmG8yTj2VIHb0bf70Ygu+HlknbjIUMHTYRH65td8smAgmNf DM4gBy4iOqtuY6/nJgirh4Kb7gjsOEwIAdF6M3zs= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [95.163.41.89]) (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 0BABAA5926B for ; Thu, 21 Mar 2024 18:04:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 0BABAA5926B Received: by smtp54.i.mail.ru with esmtpa (envelope-from ) id 1rnJyA-0000000DnAd-2yue; Thu, 21 Mar 2024 18:04:39 +0300 Date: Thu, 21 Mar 2024 18:00:40 +0300 To: Sergey Bronnikov Message-ID: References: <6179514206d7197de2097bed75aa46862cb4e651.1711029149.git.sergeyb@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6179514206d7197de2097bed75aa46862cb4e651.1711029149.git.sergeyb@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92CB91DAA594FB6BF67B30F6EBAB4B2B55AC96DE1341DF4CE182A05F5380850405E1588F0BDE33B63AC8EDD30083ED68E6BBEDA72C229A2F94BABF556B6BB7C69D6626D2099F7E75E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AEA4A6B3AFC9B957C2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE74AA616721CED6291EA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD87121CB1D9A3BE484D3EE9D34156F26B336AC68B454AE66EC93A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE764603B5C71CE8B8F9FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A333A05395E4745B117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF894DE5C3FF01FF54BA3038C0950A5D36C8A9BA7A39EFB766D91E3A1F190DE8FDBA3038C0950A5D36D5E8D9A59859A8B64FEBEDC5FC52FA7976E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249D082881546D93491E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6F82A78844E5C6993089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A504783F30E2750BAB5002B1117B3ED6962E845FC3D7ED20DD14DB8790748E3E77823CB91A9FED034534781492E4B8EEADA3FB0D9844EF8EC5BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D34D7D5115130EFC8249EC191351BBDEE52D7EF9FE4F4DF42144572F3C095A04C38034EBCFE4D2FC8481D7E09C32AA3244CF4B4225FB51CC45A459F46583F07963C52561F44CC379EC8EA455F16B58544A21C197AAF4D2E4732A5AE236DF995FB59829709634694AABAED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojt33F1wIes255RqP8TqNWew== X-Mailru-Sender: 520A125C2F17F0B1A9638AD358559B59D9FB38B15D271045AC8EDD30083ED68E6BBEDA72C229A2F9B7CBEF92542CD7C88B0A2698F12F5C9EC77752E0C033A69E86920BD37369036789A8C6A0E60D2BB63A5DB60FBEB33A8A0DA7A0AF5A3A8387 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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/ > > - 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