Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <sergeyb@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [v2][PATCH 4/5] build/ninja: create file lists outside of cmake commands
Date: Wed, 15 Jun 2022 11:48:13 +0300	[thread overview]
Message-ID: <YqmczQVas7C/BU70@tarantool.org> (raw)
In-Reply-To: <aead7667b25d80ed0bba135dc9a8135ea9b98fee.1654175362.git.sergeyb@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

  reply	other threads:[~2022-06-15  8:55 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-02 13:22 [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja Sergey Bronnikov via Tarantool-patches
2022-06-02 13:22 ` [Tarantool-patches] [v2][PATCH 1/5] build/ninja: refactoring Sergey Bronnikov via Tarantool-patches
2022-06-15  8:56   ` Sergey Kaplun via Tarantool-patches
2022-06-02 13:22 ` [Tarantool-patches] [v2][PATCH 2/5] build/ninja: create target with cli binary only once Sergey Bronnikov via Tarantool-patches
2022-06-15  9:10   ` Sergey Kaplun via Tarantool-patches
2022-06-15 16:03     ` Sergey Bronnikov via Tarantool-patches
2022-06-02 13:22 ` [Tarantool-patches] [v2][PATCH 3/5] build/ninja: rename default target Sergey Bronnikov via Tarantool-patches
2022-06-15  9:11   ` Sergey Kaplun via Tarantool-patches
2022-06-02 13:22 ` [Tarantool-patches] [v2][PATCH 4/5] build/ninja: create file lists outside of cmake commands Sergey Bronnikov via Tarantool-patches
2022-06-15  8:48   ` Igor Munkin via Tarantool-patches [this message]
2022-06-15  9:19   ` Sergey Kaplun via Tarantool-patches
2022-06-15 14:31     ` Sergey Bronnikov via Tarantool-patches
2022-06-03 13:29 ` [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja Sergey Bronnikov via Tarantool-patches
2022-06-06 11:24 ` [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64 Sergey Bronnikov via Tarantool-patches
2022-06-15  8:48   ` Igor Munkin via Tarantool-patches
2022-06-15  9:27     ` Sergey Kaplun via Tarantool-patches
2022-06-15 14:09       ` Sergey Bronnikov via Tarantool-patches
2022-06-15 14:15     ` Sergey Bronnikov via Tarantool-patches
2022-06-15  8:47 ` [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja Igor Munkin via Tarantool-patches
2022-06-15 14:57   ` Sergey Bronnikov via Tarantool-patches
2022-06-20 12:48 ` Igor Munkin via Tarantool-patches
2022-06-20 13:04   ` Sergey Bronnikov via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YqmczQVas7C/BU70@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches] [v2][PATCH 4/5] build/ninja: create file lists outside of cmake commands' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox