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
next prev parent 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