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 2F3589E35C3; Tue, 6 Feb 2024 14:27:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2F3589E35C3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1707218878; bh=YMXE7ooEZyfZ/cR74WI/4T4lnBpOVHFJv/X6q51a+l0=; 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=wMvt10nOVRE837VqAin8I+YarvPwb7G37R4qcVYyfQnjaRQqm2dtyHGa8vGVFHZPE g+e988CWy3AxCUcvVEguviwxMzp23FTLNXE58pk6HcUuHPs2+uAOAHXYk/yqOwoe+6 jOKfYefTpCUZRxHqfJBdA49/hJQLmABAOCA2wGbo= Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [95.163.41.86]) (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 271F8812DF8 for ; Tue, 6 Feb 2024 14:27:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 271F8812DF8 Received: by smtp48.i.mail.ru with esmtpa (envelope-from ) id 1rXJcI-00000004mFA-464P; Tue, 06 Feb 2024 14:27:55 +0300 Date: Tue, 6 Feb 2024 14:23:57 +0300 To: Maxim Kokryashkin Message-ID: References: <9075481da3fd5f4d2d4f7b5bb99eb3590b104693.1706520765.git.skaplun@tarantool.org> <5iuttl6zigwuep2rvkmt7e6e5fnl47j22mzwrz2lr36pradelz@ti7zceeeqmcn> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5iuttl6zigwuep2rvkmt7e6e5fnl47j22mzwrz2lr36pradelz@ti7zceeeqmcn> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD94C460F083DF69F511846634448A0B09D48B4D6D920496B8E182A05F538085040F4F3136A4D87146ED4FF92D56319F197B001AC21989C0E9B44A232330862CEEA43DCCEE5F6362163 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74323F140F3EE5B6AC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7A609AE8E79ADB41CEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38B73AB1701401CD871C66843738B67B5A7533ACB56B1E30E27FB9FA91CFC1DE04DA471835C12D1D9774AD6D5ED66289B5278DA827A17800CE74A95F4E53E8DCE969FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C327ED053E960B195E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947C2FFDA4F57982C5F42E808ACE2090B5E1725E5C173C3A84C3C5EA940A35A165FF2DBA43225CD8A89FB26E97DCB74E6252C6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5746AB64ED53966195002B1117B3ED6969C2608ED25348F7022DFD5397F446790823CB91A9FED034534781492E4B8EEAD01200A96CB7104FFBDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF7078E7AEE91E1360636DF81EA985DA6095128AAA0381C0E67BD4F38B401758634905B00F5DFE93A2807D692F32DC70327C242178DDCA373D44240E53B6A6FE1F19942BCC115CA1045F4332CA8FE04980913E6812662D5F2A5EAB5682573093F7837F15F2B5E4A70B33F2C28C22F508233FCF178C6DD14203 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojh/iSjD2txnNw0vCoCc657w== X-DA7885C5: F2536C8EAAE3FC7AF255D290C0D534F920E3ED0B1442295C89713A525818959FE9390D0DF9B851C05B1A4C17EAA7BC4BEF2421ABFA55128DAF83EF9164C44C7E X-Mailru-Sender: 689FA8AB762F7393590D8C940224AE33951803140A7E6259D1BE1C7B6F6E70257C74C4E4C70E97C9E49D44BB4BD9522A059A1ED8796F048DB274557F927329BE89D5A3BC2B10C37545BD1C3CC395C826B4A721A3011E896F X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 luajit 01/26] cmake: introduce AddTestLib macro 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, Maxim! Thanks for the review! Fixed your comments and force-pushed the branch. On 31.01.24, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the patch! > Please consider my comments below. > > On Mon, Jan 29, 2024 at 01:45:01PM +0300, Sergey Kaplun wrote: > > This patch removes copy-pasting in macros used to add a new library for > > testing and places this in the include test file. > > > > Needed for tarantool/tarantool#9398 > > --- > > test/CMakeLists.txt | 3 ++ > > .../PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt | 25 +---------------- > > test/cmake/AddTestLib.cmake | 28 +++++++++++++++++++ > > test/tarantool-tests/CMakeLists.txt | 27 +----------------- > > 4 files changed, 33 insertions(+), 50 deletions(-) > > create mode 100644 test/cmake/AddTestLib.cmake > > > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > > index 8afc42df..3ad5d15f 100644 > > --- a/test/CMakeLists.txt > > +++ b/test/CMakeLists.txt > > @@ -74,6 +74,9 @@ add_custom_target(${PROJECT_NAME}-lint DEPENDS > > set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") > > separate_arguments(LUAJIT_TEST_COMMAND) > > > > +set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") > > +include(AddTestLib) > > + > > add_subdirectory(LuaJIT-tests) > > add_subdirectory(PUC-Rio-Lua-5.1-tests) > > add_subdirectory(lua-Harness-tests) > > diff --git a/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt > > index 8501b767..e68e6aef 100644 > > --- a/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt > > +++ b/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt > > @@ -6,30 +6,7 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) > > > > # Build additional C libraries for tests. > > macro(BuildTestCLib lib sources) > > - add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources}) > > - target_include_directories(${lib} PRIVATE > > - ${LUAJIT_SOURCE_DIR} > > - ) > > - set_target_properties(${lib} PROPERTIES > > - LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" > > - PREFIX "" > > - ) > > - # XXX: The dynamic libraries are loaded with LuaJIT binary and > > - # use symbols from it. So it is totally OK to have unresolved > > - # symbols at build time. > > - if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > > - set_target_properties(${lib} PROPERTIES > > - LINK_FLAGS "-undefined dynamic_lookup" > > - ) > > - elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") > > - # XXX: This is necessary mostly for openSUSE builds, see also > > - # https://bugzilla.suse.com/show_bug.cgi?id=1012388. > > - # Just strip out the linker flag to suppress this linker > > - # option. > > - string(REPLACE "-Wl,--no-undefined" "" > > - CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" > > - ) > > - endif() > > + AddTestLib(${lib} ${sources}) > > list(APPEND TESTLIBS ${lib}) > > endmacro() > This macro now seems redundant, maybe we should get rid of it. Fixed with the following patch: =================================================================== diff --git a/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt index e68e6aef..b496cbab 100644 --- a/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt +++ b/test/PUC-Rio-Lua-5.1-tests/libs/CMakeLists.txt @@ -5,15 +5,10 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) # Build additional C libraries for tests. -macro(BuildTestCLib lib sources) - AddTestLib(${lib} ${sources}) - list(APPEND TESTLIBS ${lib}) -endmacro() - -BuildTestCLib(lib1 lib1.c) -BuildTestCLib(lib11 lib1.c lib11.c) -BuildTestCLib(lib2 lib2.c) -BuildTestCLib(lib21 lib2.c lib21.c) +AddTestLib(lib1 lib1.c) +AddTestLib(lib11 lib1.c lib11.c) +AddTestLib(lib2 lib2.c) +AddTestLib(lib21 lib2.c lib21.c) # Create the exact copy of the lib2 library for tests # in . diff --git a/test/cmake/AddTestLib.cmake b/test/cmake/AddTestLib.cmake index a31f632c..c5733d99 100644 --- a/test/cmake/AddTestLib.cmake +++ b/test/cmake/AddTestLib.cmake @@ -1,3 +1,5 @@ +# Add a test library to build and add it to the `TESTLIBS` +# variable. macro(AddTestLib lib sources) add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources}) target_include_directories(${lib} PRIVATE @@ -25,4 +27,5 @@ macro(AddTestLib lib sources) CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" ) endif() + list(APPEND TESTLIBS ${lib}) endmacro() diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt index 80d94a78..8c7c3ae8 100644 --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -12,12 +12,7 @@ endif() macro(BuildTestCLib lib sources) AddTestLib(${lib} ${sources}) - # XXX: Append the lib to be built to the dependency list. - # Unfortunately, there is no convenient way in CMake to extend - # the list in parent scope other than join two strings with - # semicolon. If one finds the normal way to make it work, feel - # free to reach me. - set(TESTLIBS "${lib};${TESTLIBS}" PARENT_SCOPE) + set(TESTLIBS "${TESTLIBS}" PARENT_SCOPE) # Add the directory where the lib is built to the list with # entries for LUA_CPATH environment variable, so LuaJIT can find # and load it. See the comment about extending the list in the =================================================================== > > > > diff --git a/test/cmake/AddTestLib.cmake b/test/cmake/AddTestLib.cmake > > new file mode 100644 > > index 00000000..3764ee5c > > --- /dev/null > > +++ b/test/cmake/AddTestLib.cmake > > @@ -0,0 +1,28 @@ > > +macro(AddTestLib lib sources) > > + add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources}) > > + target_include_directories(${lib} PRIVATE > > + ${LUAJIT_SOURCE_DIR} > > + ${CMAKE_CURRENT_SOURCE_DIR} > > + ) > > + set_target_properties(${lib} PROPERTIES > > + LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" > > + PREFIX "" > > + ) > > + > > + # XXX: This change affects the current cmake variable scope and > Typo: s/cmake/CMake/ > Typo: s/scope and/scope,/ > > + # so a user should care to don't use it in a top level scope. > Typo: s/should care to don't use/shouldn't use/ > Typo: s/top level/top-level/ Fixed: =================================================================== diff --git a/test/cmake/AddTestLib.cmake b/test/cmake/AddTestLib.cmake index 4450a950..a31f632c 100644 --- a/test/cmake/AddTestLib.cmake +++ b/test/cmake/AddTestLib.cmake @@ -9,8 +9,8 @@ macro(AddTestLib lib sources) PREFIX "" ) - # XXX: This change affects the current cmake variable scope and - # so a user should care to don't use it in a top level scope. + # XXX: This change affects the current CMake variable scope, so + # a user shouldn't use it in a top-level scope. # The dynamic libraries are loaded with LuaJIT binary and use # symbols from it. So it is totally OK to have unresolved # symbols at build time. =================================================================== > > + # The dynamic libraries are loaded with LuaJIT binary and use > > + # symbols from it. So it is totally OK to have unresolved > > + # symbols at build time. > > + if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > > + set_target_properties(${lib} PROPERTIES > > + LINK_FLAGS "-undefined dynamic_lookup" > > + ) > > + else() > > + # FIXME: Unfortunately, there is no another way to suppress > > + # this linker option, so just strip it out from the flags. > > + string(REPLACE "-Wl,--no-undefined" "" > > + CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" > > + ) > I wonder why you've omitted the condition along with the comment here. Added the condition. =================================================================== diff --git a/test/cmake/AddTestLib.cmake b/test/cmake/AddTestLib.cmake index 3764ee5c..4450a950 100644 --- a/test/cmake/AddTestLib.cmake +++ b/test/cmake/AddTestLib.cmake @@ -18,7 +18,7 @@ macro(AddTestLib lib sources) set_target_properties(${lib} PROPERTIES LINK_FLAGS "-undefined dynamic_lookup" ) - else() + elseif(CMAKE_SYSTEM_NAME STREQUAL "Linux") # FIXME: Unfortunately, there is no another way to suppress # this linker option, so just strip it out from the flags. string(REPLACE "-Wl,--no-undefined" "" =================================================================== > > + endif() > > +endmacro() > > diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt > > index 6d686876..80d94a78 100644 > > --- a/test/tarantool-tests/CMakeLists.txt > > +++ b/test/tarantool-tests/CMakeLists.txt > > @@ -11,32 +11,7 @@ if(NOT PROVE) > > endif() > > > > macro(BuildTestCLib lib sources) > > - add_library(${lib} SHARED EXCLUDE_FROM_ALL ${sources}) > > - target_include_directories(${lib} PRIVATE > > - ${LUAJIT_SOURCE_DIR} > > - ${CMAKE_CURRENT_SOURCE_DIR} > > - ) > > - set_target_properties(${lib} PROPERTIES > > - LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" > > - PREFIX "" > > - ) > > - > > - # XXX: This change affects the current cmake variable scope and > > - # so a user should care to don't use it in a top level scope. > > - # The dynamic libraries are loaded with LuaJIT binary and use > > - # symbols from it. So it is totally OK to have unresolved > > - # symbols at build time. > > - if(CMAKE_SYSTEM_NAME STREQUAL "Darwin") > > - set_target_properties(${lib} PROPERTIES > > - LINK_FLAGS "-undefined dynamic_lookup" > > - ) > > - else() > > - # FIXME: Unfortunately, there is no another way to suppress > > - # this linker option, so just strip it out from the flags. > > - string(REPLACE "-Wl,--no-undefined" "" > > - CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}" > > - ) > > - endif() > > + AddTestLib(${lib} ${sources}) > Same comments as for the above section. See the patch above. > > # XXX: Append the lib to be built to the dependency list. > > # Unfortunately, there is no convenient way in CMake to extend > > # the list in parent scope other than join two strings with > > -- > > 2.43.0 > > -- Best regards, Sergey Kaplun