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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.
>
> 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/
> +  # 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.
> +  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.
>    # 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
>