Re: [Tarantool-patches] [v2][PATCH 2/5] build/ninja: create target with cli binary only once

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

Hi, Sergey!

Thanks for the patch!

LGTM, except a few nits below.

On 02.06.22, Sergey Bronnikov via Tarantool-patches wrote:
> CMakeLists defines target for cli binary two times (for static and
> shared build type) with the same output names. CMake generates two
> targets with same names and it breaks Ninja:
> 
> ninja: Entering directory `build/'
> ninja: error: build.ninja:1656: multiple rules generate src/luajit [-w
> dupbuild=err]
> 
> Patch adds variables required for different build types and create

Typo? s/create/creates/

> target for cli binary only once to avoid conflict.
> ---
>  src/CMakeLists.txt | 40 ++++++++++++++++------------------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 8b651c5a..104ce999 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -313,40 +313,32 @@ target_link_libraries(libluajit_shared ${TARGET_LIBS})
>  
>  # Compiling and linking CLIs.
>  

<snipped>

>  if(NOT BUILDMODE STREQUAL "dynamic")
>    set(LIBLUAJIT_STATIC_DEPS libluajit_static)
>    set(LUAJIT_DEPS luajit_static)
> +  set(LUAJIT_BIN luajit_static)
> +  set(LUAJIT_LIB libluajit_static)
>  endif()
>  if(NOT BUILDMODE STREQUAL "static")
>    set(LIBLUAJIT_SHARED_DEPS libluajit_shared)
>    set(LUAJIT_DEPS luajit_shared)
> +  set(LUAJIT_BIN luajit_shared)
> +  set(LUAJIT_LIB libluajit_shared)
>  endif()
>  set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})

Nit: Looks like the LUAJIT_DEPS variable duplicates work of
LUAJIT_BIN variable now.

>  
> +add_executable(${LUAJIT_BIN} EXCLUDE_FROM_ALL ${CLI_SOURCES})
> +set_target_properties(luajit_shared PROPERTIES
> +  OUTPUT_NAME "${LUAJIT_CLI_NAME}"
> +  COMPILE_FLAGS "${TARGET_C_FLAGS}"
> +  LINK_FLAGS "${TARGET_BIN_FLAGS}"
> +  RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
> +)
> +target_include_directories(${LUAJIT_BIN} PRIVATE
> +  ${CMAKE_CURRENT_BINARY_DIR}
> +)
> +target_link_libraries(${LUAJIT_BIN} ${LUAJIT_LIB} ${TARGET_LIBS})
> +
>  # XXX: The variable is used in testing, so PARENT_SCOPE option
>  # is obligatory.
>  set(LUAJIT_BINARY $<TARGET_FILE:${LUAJIT_DEPS}> PARENT_SCOPE)
> -- 
> 2.25.1
> 

-- 
Best regards,
Sergey Kaplun