Re: [Tarantool-patches] [v2][PATCH 4/5] build/ninja: create file lists outside of cmake commands

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Sergey,

Thanks for the patch! LGTM as trivial, but please consider nits below.

JFYI, we're not using complex prefixes in LuaJIT, so feel free to use
just build and mention Ninja specifics within the rest of commit
subject.

On 02.06.22, Sergey Bronnikov wrote:
> Patch adds a last change required for building LuaJIT with Ninja - using
> glob inside CMake commands (add_custom_command and
> set_source_files_properties) breaks buildng with Ninja.
> 

I believe, the part below is much more suitable for the last patch in
the series, since we want to enable Ninja especially for CI purposes.

> By default CMake generates files suitable for building a project with
> Make. However, it allows to generate files for Ninja too. Ninja [1] may
> build project a bit faster than Make, see comparison in [2].
> 
> How-to build with Ninja:
> 
> $ cmake -G Ninja -B build -S .
> $ cmake --build build --parallel
> 
> 1. https://ninja-build.org/
> 2. https://mesonbuild.com/Simple-comparison.html
> ---
>  src/host/CMakeLists.txt | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/host/CMakeLists.txt b/src/host/CMakeLists.txt
> index e01db87d..1ce3e224 100644
> --- a/src/host/CMakeLists.txt
> +++ b/src/host/CMakeLists.txt
> @@ -25,11 +25,12 @@ set(DYNASM_DIR ${PROJECT_SOURCE_DIR}/dynasm)
>  set(DYNASM_DASC "${LUAJIT_SOURCE_DIR}/vm_${DYNASM_ARCH}.dasc")
>  set(DYNASM ${HOST_LUA} ${DYNASM_DIR}/dynasm.lua)
>  
> +file(GLOB DYNASM_LUA ${DYNASM_DIR}/*.lua)
>  add_custom_command(
>    COMMENT "Generating buildvm_arch.h"
>    OUTPUT buildvm_arch.h
>    COMMAND ${DYNASM} ${DYNASM_FLAGS} -o buildvm_arch.h ${DYNASM_DASC}
> -  DEPENDS ${MINILUA} ${DYNASM_DASC} ${DYNASM_DIR}/*.lua
> +  DEPENDS ${MINILUA} ${DYNASM_DASC} ${DYNASM_LUA}

Minor: IMHO, DYNASM_LUA_FILES looks better here.

>    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
>  )
>  
> @@ -47,8 +48,9 @@ add_executable(buildvm EXCLUDE_FROM_ALL
>    # *sources* list.
>    buildvm_arch.h
>  )
> +file(GLOB DASM_HEADERS ${DYNASM_DIR}/dasm_*.h)
>  set_source_files_properties(buildvm.c PROPERTIES
> -  OBJECT_DEPENDS ${DYNASM_DIR}/dasm_*.h
> +  OBJECT_DEPENDS "${DASM_HEADERS}"

Minor: Why have you enclosed this variable in the quotes, but the one
above have not?

>  )
>  set_target_properties(buildvm PROPERTIES
>    COMPILE_FLAGS "${HOST_C_FLAGS} ${TARGET_C_FLAGS}"
> -- 
> 2.25.1
> 

-- 
Best regards,
IM