Re: [Tarantool-patches] [PATCH v2 luajit 01/26] cmake: introduce AddTestLib macro

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

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