[Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Sergey Bronnikov via Tarantool-patches]

Patch series support of Ninja to a LuaJIT build system and a new job to
continuous integration pipeline that builds using Ninja.

On my laptop Ninja reduces building time by 14% (with Make it takes 5.7
sec, with Ninja 3.9 sec). It is not so much, but without Ninja support
in LuaJIT it is not possible to build Tarantool with Ninja.

Branch: https://github.com/tarantool/luajit/tree/ligurio/ninja-support
CI status: https://github.com/tarantool/luajit/commit/acfd7552f1b8428242a6b8cbc783ed584c21beef

Sergey Bronnikov (5):
  build/ninja: refactoring
  build/ninja: create target with cli binary only once
  build/ninja: rename default target
  build/ninja: create file lists outside of cmake commands
  ci: add job with build using Ninja on linux-x86_64

 .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
 src/CMakeLists.txt                       | 46 ++++++++-------------
 src/host/CMakeLists.txt                  |  6 ++-
 3 files changed, 72 insertions(+), 31 deletions(-)
 create mode 100644 .github/workflows/linux-x86_64-ninja.yml

-- 
2.25.1

[Tarantool-patches] [v2][PATCH 1/5] build/ninja: refactoring

[Sergey Bronnikov via Tarantool-patches]

Define variables required for cli binary targets in conditions.
Change is needed for the next commit.
---
 src/CMakeLists.txt | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 1a3f106a..8b651c5a 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -339,17 +339,13 @@ target_link_libraries(luajit_shared libluajit_shared ${TARGET_LIBS})
 
 if(NOT BUILDMODE STREQUAL "dynamic")
   set(LIBLUAJIT_STATIC_DEPS libluajit_static)
+  set(LUAJIT_DEPS luajit_static)
 endif()
 if(NOT BUILDMODE STREQUAL "static")
   set(LIBLUAJIT_SHARED_DEPS libluajit_shared)
-endif()
-set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
-
-if(BUILDMODE STREQUAL "dynamic")
   set(LUAJIT_DEPS luajit_shared)
-else()
-  set(LUAJIT_DEPS luajit_static)
 endif()
+set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})
 
 # XXX: The variable is used in testing, so PARENT_SCOPE option
 # is obligatory.
-- 
2.25.1

Re: [Tarantool-patches] [v2][PATCH 1/5] build/ninja: refactoring

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!

Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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
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.
 
-add_executable(luajit_static EXCLUDE_FROM_ALL ${CLI_SOURCES})
-set_target_properties(luajit_static 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_static PRIVATE
-  ${CMAKE_CURRENT_BINARY_DIR}
-)
-target_link_libraries(luajit_static libluajit_static ${TARGET_LIBS})
-
-add_executable(luajit_shared 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_shared PRIVATE
-  ${CMAKE_CURRENT_BINARY_DIR}
-)
-target_link_libraries(luajit_shared libluajit_shared ${TARGET_LIBS})
-
 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})
 
+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

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

[Sergey Kaplun via Tarantool-patches]

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

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

[Sergey Bronnikov via Tarantool-patches]

Thanks for review!

On 15.06.2022 12:10, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
>
> Thanks for the patch!
>
> LGTM, except a few nits below.
>
> On 02.06.22, Sergey Bronnikov via Tarantool-patches wrote:
<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.
Fixed.
>>   
>> +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
>>

[Tarantool-patches] [v2][PATCH 3/5] build/ninja: rename default target

[Sergey Bronnikov via Tarantool-patches]

Default target name and target for building CLI have the same name.
It breaks Ninja build:

ninja: Entering directory `build/'
ninja: error: build.ninja:1656: multiple rules generate src/luajit [-w
dupbuild=err]

Patch renames default target name to avoid conflict.
---
 src/CMakeLists.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 104ce999..4995211f 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -344,7 +344,7 @@ target_link_libraries(${LUAJIT_BIN} ${LUAJIT_LIB} ${TARGET_LIBS})
 set(LUAJIT_BINARY $<TARGET_FILE:${LUAJIT_DEPS}> PARENT_SCOPE)
 
 add_custom_target(libluajit DEPENDS ${LIBLUAJIT_DEPS})
-add_custom_target(luajit ALL DEPENDS libluajit ${LUAJIT_DEPS})
+add_custom_target(luajit-main ALL DEPENDS libluajit ${LUAJIT_DEPS})
 
 # Unfortunately, CMake provides no guarantees for install commands
 # used for the targets excluded from <all> and obliges user to
-- 
2.25.1

Re: [Tarantool-patches] [v2][PATCH 3/5] build/ninja: rename default target

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!

Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

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.

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}
   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}"
 )
 set_target_properties(buildvm PROPERTIES
   COMPILE_FLAGS "${HOST_C_FLAGS} ${TARGET_C_FLAGS}"
-- 
2.25.1

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

[Igor Munkin via Tarantool-patches]

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

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

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!

Thanks for the patch!

LGTM, except a few nits regarding the commit message.

On 02.06.22, Sergey Bronnikov via Tarantool-patches wrote:
> Patch adds a last change required for building LuaJIT with Ninja - using

Typo: s/Patch/The patch/

> glob inside CMake commands (add_custom_command and
> set_source_files_properties) breaks buildng with Ninja.

Typo: s/buildng/building/

> 
> 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].

Typo: s/comparison/the comparison/

> 
> How-to build with Ninja:
> 
> $ cmake -G Ninja -B build -S .
> $ cmake --build build --parallel

Side note: Unfortunately `make test` command (or what should I use for
tests, when build with ninja?) fails with the following error:

| Checking /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua 3 warnings
|
|     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:5:121: (W631) line is too long (595 > 120)
|     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:7:121: (W631) line is too long (613 > 120)
|     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:11:121: (W631) line is too long (273 > 120)

But this is the problem of OOS build (default for ninja), not this
patch, IINM.

> 
> 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

<snipped>

> -- 
> 2.25.1
> 

-- 
Best regards,
Sergey Kaplun

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

[Sergey Bronnikov via Tarantool-patches]

Sergey, thanks for review!

On 15.06.2022 12:19, Sergey Kaplun via Tarantool-patches wrote:
> Hi, Sergey!
>
> Thanks for the patch!
>
> LGTM, except a few nits regarding the commit message.
>
> On 02.06.22, Sergey Bronnikov via Tarantool-patches wrote:
>> Patch adds a last change required for building LuaJIT with Ninja - using
> Typo: s/Patch/The patch/
Fixed, thanks.
>
>> glob inside CMake commands (add_custom_command and
>> set_source_files_properties) breaks buildng with Ninja.
> Typo: s/buildng/building/

Fixed, thanks!


>> 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].
> Typo: s/comparison/the comparison/
Fixed, thanks!
>> How-to build with Ninja:
>>
>> $ cmake -G Ninja -B build -S .
>> $ cmake --build build --parallel
> Side note: Unfortunately `make test` command (or what should I use for
> tests, when build with ninja?)

Use 'ninja' instead:

sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ cmake -G Ninja 
-B . -S .
-- The C compiler identification is GNU 9.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- [SetVersion] Reading version from VCS: v2.1.0-beta3-193-gd8c86e28
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Configuring done
-- Generating done
-- Build files have been written to: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit
sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ ninja -f 
build.ninja
[165/165] Linking C static library src/libluajit.a
sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ ninja -f 
build.ninja test
<snipped>


 >>> closing state <<<

sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ echo $?
0
sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$

or even do everything (confgure and build) using CMake:

sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ cmake -G Ninja 
-B . -S .
-- The C compiler identification is GNU 9.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- [SetVersion] Reading version from VCS: v2.1.0-beta3-193-gd8c86e28
-- The ASM compiler identification is GNU
-- Found assembler: /usr/bin/cc
-- Configuring done
-- Generating done
-- Build files have been written to: 
/home/sergeyb/sources/MRG/tarantool/third_party/luajit
sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$ cmake --build . 
--parallel
[165/165] Linking C static library src/libluajit.a
sergeyb@pony:~/sources/MRG/tarantool/third_party/luajit$

> fails with the following error:
>
> | Checking /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua 3 warnings
> |
> |     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:5:121: (W631) line is too long (595 > 120)
> |     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:7:121: (W631) line is too long (613 > 120)
> |     /home/burii/reviews/luajit/ninja/build/src/jit/vmdef.lua:11:121: (W631) line is too long (273 > 120)
>
> But this is the problem of OOS build (default for ninja), not this
> patch, IINM.

Hmm, it works fine for me (see output above).

But yes, luacheck has a problems with running OOS. See description of 
the problem in [1].

The problem was workarounded in Tarantool source repository, see commit:

commit af448464d15f60b87f1c9ef41a7816911c889459
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Wed Nov 11 10:09:25 2020 +0300

     tools: fix luacheck invocation in different cases

     Now `make luacheck` gracefully handles different cases[^1]: in-source
     and out-of-source build (within the source tree or outside), current
     working directory as a real path or with symlink components.

     As result of looking into those problems I filed the issue [1] against
     luacheck. It seems, there are problems around absolute paths with
     symlinks components.

     [^1]: We have the similar problems with LuaJIT's luacheck rule. They
           will be fixed in a separate patch.

     [1]: https://github.com/mpeterv/luacheck/issues/208

     Reviewed-by: Sergey Bronnikov <sergeyb@tarantool.org>
     Reviewed-by: Igor Munkin <imun@tarantool.org>

1. https://github.com/mpeterv/luacheck/issues/208

>
>> 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
> <snipped>
>
>> -- 
>> 2.25.1
>>

Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Sergey Bronnikov via Tarantool-patches]

In proposed patch series set_target_properties() uses certain target 
name instead of variable.

Fixed with patch below and force-pushed to the branch:

--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -328,7 +328,7 @@ endif()
  set(LIBLUAJIT_DEPS ${LIBLUAJIT_STATIC_DEPS} ${LIBLUAJIT_SHARED_DEPS})

  add_executable(${LUAJIT_BIN} EXCLUDE_FROM_ALL ${CLI_SOURCES})
-set_target_properties(luajit_shared PROPERTIES
+set_target_properties(${LUAJIT_BIN} PROPERTIES
    OUTPUT_NAME "${LUAJIT_CLI_NAME}"
    COMPILE_FLAGS "${TARGET_C_FLAGS}"
    LINK_FLAGS "${TARGET_BIN_FLAGS}"

Sergey


On 02.06.2022 16:22, Sergey Bronnikov wrote:
> Patch series support of Ninja to a LuaJIT build system and a new job to
> continuous integration pipeline that builds using Ninja.
>
> On my laptop Ninja reduces building time by 14% (with Make it takes 5.7
> sec, with Ninja 3.9 sec). It is not so much, but without Ninja support
> in LuaJIT it is not possible to build Tarantool with Ninja.
>
> Branch: https://github.com/tarantool/luajit/tree/ligurio/ninja-support
> CI status: https://github.com/tarantool/luajit/commit/acfd7552f1b8428242a6b8cbc783ed584c21beef
>
> Sergey Bronnikov (5):
>    build/ninja: refactoring
>    build/ninja: create target with cli binary only once
>    build/ninja: rename default target
>    build/ninja: create file lists outside of cmake commands
>    ci: add job with build using Ninja on linux-x86_64
>
>   .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>   src/CMakeLists.txt                       | 46 ++++++++-------------
>   src/host/CMakeLists.txt                  |  6 ++-
>   3 files changed, 72 insertions(+), 31 deletions(-)
>   create mode 100644 .github/workflows/linux-x86_64-ninja.yml
>

[Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Sergey Bronnikov via Tarantool-patches]

---
 .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 .github/workflows/linux-x86_64-ninja.yml

diff --git a/.github/workflows/linux-x86_64-ninja.yml b/.github/workflows/linux-x86_64-ninja.yml
new file mode 100644
index 00000000..833d36d9
--- /dev/null
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -0,0 +1,51 @@
+name: "LuaJIT test workflow with Ninja (Linux/x86_64)"
+
+on:
+  push:
+    branches-ignore:
+      - '**-notest'
+      - 'upstream-**'
+    tags-ignore:
+      - '**'
+
+concurrency:
+  # An update of a developer branch cancels the previously
+  # scheduled workflow run for this branch. However, the default
+  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
+  # etc.) workflow runs are never canceled.
+  #
+  # We use a trick here: define the concurrency group as 'workflow
+  # run ID' + # 'workflow run attempt' because it is a unique
+  # combination for any run. So it effectively discards grouping.
+  #
+  # XXX: we cannot use `github.sha` as a unique identifier because
+  # pushing a tag may cancel a run that works on a branch push
+  # event.
+  group: ${{ (
+    github.ref == 'refs/heads/tarantool' ||
+    startsWith(github.ref, 'refs/heads/tarantool-')) &&
+    format('{0}-{1}', github.run_id, github.run_attempt) ||
+    format('{0}-{1}', github.workflow, github.ref) }}
+  cancel-in-progress: true
+
+jobs:
+  test-linux-x86_64:
+    runs-on: ubuntu-20.04-self-hosted
+    strategy:
+      fail-fast: false
+    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
+    steps:
+      - uses: actions/checkout@v2.3.4
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: setup
+        run: |
+          sudo apt -y update
+          sudo apt -y install cmake ninja-build gcc perl
+      - name: configure
+        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
+      - name: build
+        run: cmake --build . -j $(nproc)
+      - name: test
+        run: cmake --build . -j $(nproc) -t test
-- 
2.25.1

Re: [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Igor Munkin via Tarantool-patches]

Sergey,

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

On 06.06.22, Sergey Bronnikov wrote:
> ---
>  .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 .github/workflows/linux-x86_64-ninja.yml
> 
> diff --git a/.github/workflows/linux-x86_64-ninja.yml b/.github/workflows/linux-x86_64-ninja.yml
> new file mode 100644
> index 00000000..833d36d9
> --- /dev/null
> +++ b/.github/workflows/linux-x86_64-ninja.yml
> @@ -0,0 +1,51 @@
> +name: "LuaJIT test workflow with Ninja (Linux/x86_64)"
> +
> +on:
> +  push:
> +    branches-ignore:
> +      - '**-notest'
> +      - 'upstream-**'
> +    tags-ignore:
> +      - '**'
> +
> +concurrency:
> +  # An update of a developer branch cancels the previously
> +  # scheduled workflow run for this branch. However, the default
> +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
> +  # etc.) workflow runs are never canceled.
> +  #
> +  # We use a trick here: define the concurrency group as 'workflow
> +  # run ID' + # 'workflow run attempt' because it is a unique
> +  # combination for any run. So it effectively discards grouping.
> +  #
> +  # XXX: we cannot use `github.sha` as a unique identifier because
> +  # pushing a tag may cancel a run that works on a branch push
> +  # event.
> +  group: ${{ (
> +    github.ref == 'refs/heads/tarantool' ||
> +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> +    format('{0}-{1}', github.workflow, github.ref) }}
> +  cancel-in-progress: true
> +
> +jobs:
> +  test-linux-x86_64:
> +    runs-on: ubuntu-20.04-self-hosted
> +    strategy:
> +      fail-fast: false
> +    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}

There is no testing matrix, so you can just name the workflow kinda
"Linux/x86_64 Ninja".

> +    steps:
> +      - uses: actions/checkout@v2.3.4
> +        with:
> +          fetch-depth: 0
> +          submodules: recursive
> +      - name: setup
> +        run: |
> +          sudo apt -y update
> +          sudo apt -y install cmake ninja-build gcc perl
> +      - name: configure
> +        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
> +      - name: build
> +        run: cmake --build . -j $(nproc)
> +      - name: test
> +        run: cmake --build . -j $(nproc) -t test

Please, use long options, as we discussed here[1].

> -- 
> 2.25.1
> 

[1]: https://lists.tarantool.org/tarantool-patches/YpjeijSClWO0v82Y@tarantool.org/

-- 
Best regards,
IM

Re: [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!

Thanks for the patch!

LGTM, after fixes mentioned by Igor below and my 2 cents below.

On 15.06.22, Igor Munkin via Tarantool-patches wrote:
> Sergey,
> 
> Thanks for the patch! LGTM as trivial, but please consider nits below.
> 
> On 06.06.22, Sergey Bronnikov wrote:
> > ---
> >  .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> >  create mode 100644 .github/workflows/linux-x86_64-ninja.yml
> > 
> > diff --git a/.github/workflows/linux-x86_64-ninja.yml b/.github/workflows/linux-x86_64-ninja.yml
> > new file mode 100644
> > index 00000000..833d36d9
> > --- /dev/null
> > +++ b/.github/workflows/linux-x86_64-ninja.yml
> > @@ -0,0 +1,51 @@
> > +name: "LuaJIT test workflow with Ninja (Linux/x86_64)"
> > +
> > +on:
> > +  push:
> > +    branches-ignore:
> > +      - '**-notest'
> > +      - 'upstream-**'
> > +    tags-ignore:
> > +      - '**'
> > +
> > +concurrency:
> > +  # An update of a developer branch cancels the previously
> > +  # scheduled workflow run for this branch. However, the default
> > +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
> > +  # etc.) workflow runs are never canceled.
> > +  #
> > +  # We use a trick here: define the concurrency group as 'workflow
> > +  # run ID' + # 'workflow run attempt' because it is a unique
> > +  # combination for any run. So it effectively discards grouping.
> > +  #
> > +  # XXX: we cannot use `github.sha` as a unique identifier because
> > +  # pushing a tag may cancel a run that works on a branch push
> > +  # event.
> > +  group: ${{ (
> > +    github.ref == 'refs/heads/tarantool' ||
> > +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
> > +    format('{0}-{1}', github.run_id, github.run_attempt) ||
> > +    format('{0}-{1}', github.workflow, github.ref) }}
> > +  cancel-in-progress: true
> > +
> > +jobs:
> > +  test-linux-x86_64:

Nit: May be `test-linux-ninja-x86_64` is better?
To be different from the same name for make target building?

> > +    runs-on: ubuntu-20.04-self-hosted
> > +    strategy:
> > +      fail-fast: false

Nit: As far as there is no testing matrix we can omit strategy for it.

> > +    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
> 
> There is no testing matrix, so you can just name the workflow kinda
> "Linux/x86_64 Ninja".
> 
> > +    steps:
> > +      - uses: actions/checkout@v2.3.4
> > +        with:
> > +          fetch-depth: 0
> > +          submodules: recursive
> > +      - name: setup
> > +        run: |
> > +          sudo apt -y update
> > +          sudo apt -y install cmake ninja-build gcc perl
> > +      - name: configure
> > +        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
> > +      - name: build
> > +        run: cmake --build . -j $(nproc)
> > +      - name: test
> > +        run: cmake --build . -j $(nproc) -t test
> 
> Please, use long options, as we discussed here[1].
> 
> > -- 
> > 2.25.1
> > 
> 
> [1]: https://lists.tarantool.org/tarantool-patches/YpjeijSClWO0v82Y@tarantool.org/
> 
> -- 
> Best regards,
> IM

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

thanks for review!

On 15.06.2022 12:27, Sergey Kaplun wrote:
> Hi, Sergey!
>
> Thanks for the patch!
>
> LGTM, after fixes mentioned by Igor below and my 2 cents below.
>
> On 15.06.22, Igor Munkin via Tarantool-patches wrote:
>> Sergey,
>>
>> Thanks for the patch! LGTM as trivial, but please consider nits below.
>>
>> On 06.06.22, Sergey Bronnikov wrote:
>>> ---
>>>   .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>>>   1 file changed, 51 insertions(+)
>>>   create mode 100644 .github/workflows/linux-x86_64-ninja.yml
>>>
>>> diff --git a/.github/workflows/linux-x86_64-ninja.yml b/.github/workflows/linux-x86_64-ninja.yml
>>> new file mode 100644
>>> index 00000000..833d36d9
>>> --- /dev/null
>>> +++ b/.github/workflows/linux-x86_64-ninja.yml
>>> @@ -0,0 +1,51 @@
>>> +name: "LuaJIT test workflow with Ninja (Linux/x86_64)"
>>> +
>>> +on:
>>> +  push:
>>> +    branches-ignore:
>>> +      - '**-notest'
>>> +      - 'upstream-**'
>>> +    tags-ignore:
>>> +      - '**'
>>> +
>>> +concurrency:
>>> +  # An update of a developer branch cancels the previously
>>> +  # scheduled workflow run for this branch. However, the default
>>> +  # branch, and long-term branch (tarantool-1.10, tarantool-2.8,
>>> +  # etc.) workflow runs are never canceled.
>>> +  #
>>> +  # We use a trick here: define the concurrency group as 'workflow
>>> +  # run ID' + # 'workflow run attempt' because it is a unique
>>> +  # combination for any run. So it effectively discards grouping.
>>> +  #
>>> +  # XXX: we cannot use `github.sha` as a unique identifier because
>>> +  # pushing a tag may cancel a run that works on a branch push
>>> +  # event.
>>> +  group: ${{ (
>>> +    github.ref == 'refs/heads/tarantool' ||
>>> +    startsWith(github.ref, 'refs/heads/tarantool-')) &&
>>> +    format('{0}-{1}', github.run_id, github.run_attempt) ||
>>> +    format('{0}-{1}', github.workflow, github.ref) }}
>>> +  cancel-in-progress: true
>>> +
>>> +jobs:
>>> +  test-linux-x86_64:
> Nit: May be `test-linux-ninja-x86_64` is better?
> To be different from the same name for make target building?

Agree, fixed:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -29,10 +29,8 @@ concurrency:
    cancel-in-progress: true

  jobs:
-  test-linux-x86_64:
+  test-linux-ninja-x86_64:
      runs-on: ubuntu-20.04-self-hosted

>
>>> +    runs-on: ubuntu-20.04-self-hosted
>>> +    strategy:
>>> +      fail-fast: false
> Nit: As far as there is no testing matrix we can omit strategy for it.

Seems so [1].

Fixed with patch below:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -31,8 +31,6 @@ concurrency:
  jobs:
    test-linux-x86_64:
      runs-on: ubuntu-20.04-self-hosted
-    strategy:
-      fail-fast: false
      name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
      steps:
        - uses: actions/checkout@v2.3.4

1. 
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstrategyfail-fast

>
>>> +    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
>> There is no testing matrix, so you can just name the workflow kinda
>> "Linux/x86_64 Ninja".
Fixed too.
>>
>>> +    steps:
>>> +      - uses: actions/checkout@v2.3.4
>>> +        with:
>>> +          fetch-depth: 0
>>> +          submodules: recursive
>>> +      - name: setup
>>> +        run: |
>>> +          sudo apt -y update
>>> +          sudo apt -y install cmake ninja-build gcc perl
>>> +      - name: configure
>>> +        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
>>> +      - name: build
>>> +        run: cmake --build . -j $(nproc)
>>> +      - name: test
>>> +        run: cmake --build . -j $(nproc) -t test
>> Please, use long options, as we discussed here[1].

Fixed:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -44,6 +44,6 @@ jobs:
        - name: configure
          run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
        - name: build
-        run: cmake --build . -j $(nproc)
+        run: cmake --build . --parallel $(($(nproc) + 1))
        - name: test
-        run: cmake --build . -j $(nproc) -t test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test

>>
>>> -- 
>>> 2.25.1
>>>
>> [1]: https://lists.tarantool.org/tarantool-patches/YpjeijSClWO0v82Y@tarantool.org/
>>
>> -- 
>> Best regards,
>> IM

Re: [Tarantool-patches] [v2][PATCH 5/5] ci: add job with build using Ninja on linux-x86_64

[Sergey Bronnikov via Tarantool-patches]

Igor, thanks for review!

On 15.06.2022 11:48, Igor Munkin wrote:
> Sergey,
>
> Thanks for the patch! LGTM as trivial, but please consider nits below.
>
> On 06.06.22, Sergey Bronnikov wrote:
<snipped>
>> +
>> +jobs:
>> +  test-linux-x86_64:
>> +    runs-on: ubuntu-20.04-self-hosted
>> +    strategy:
>> +      fail-fast: false
>> +    name: Linux/x86_64 ${{ matrix.BUILDTYPE }}
> There is no testing matrix, so you can just name the workflow kinda
> "Linux/x86_64 Ninja".
Renamed to "Linux/x86_64 Ninja".
>> +    steps:
>> +      - uses: actions/checkout@v2.3.4
>> +        with:
>> +          fetch-depth: 0
>> +          submodules: recursive
>> +      - name: setup
>> +        run: |
>> +          sudo apt -y update
>> +          sudo apt -y install cmake ninja-build gcc perl
>> +      - name: configure
>> +        run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
>> +      - name: build
>> +        run: cmake --build . -j $(nproc)
>> +      - name: test
>> +        run: cmake --build . -j $(nproc) -t test
> Please, use long options, as we discussed here[1].


Fixed:

--- a/.github/workflows/linux-x86_64-ninja.yml
+++ b/.github/workflows/linux-x86_64-ninja.yml
@@ -44,6 +44,6 @@ jobs:
        - name: configure
          run: cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -G Ninja
        - name: build
-        run: cmake --build . -j $(nproc)
+        run: cmake --build . --parallel $(($(nproc) + 1))
        - name: test
-        run: cmake --build . -j $(nproc) -t test
+        run: cmake --build . --parallel $(($(nproc) + 1)) --target test

>
>> -- 
>> 2.25.1
>>
> [1]: https://lists.tarantool.org/tarantool-patches/YpjeijSClWO0v82Y@tarantool.org/
>

Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Igor Munkin via Tarantool-patches]

Sergey,

Thanks for the series! I've left some nits for 4th and 5th patches in
the relevant threads. Everything related to the first three patches you
can find below.

Thanks for the series! You can find my comments in the relevant patches.
However, I have one more: since we're going to use Ninja, please add
Ninja-specific output to .gitignore to not spoil <git status> output.

I tried to find more elegant solution for this case, but failed. Look
like we definitely have no option other than define all target names
conditionally. The related patches look good, but I propose some minor
changes:
* Please merge the first three patches into one, since they all related
  to the same fix: provide unique names for targets building both LuaJIT
  library and binary.
* LUAJIT_DEPS looks irrelevant if LUAJIT_BIN is introduced: the original
  purpose of this variable was to template the dependency of <luajit>
  target.
* Please adjust install components, since you've changed the default
  target name to <luajit-main>

The last general nits:
* We don't use complex prefixes for commit subjects (like
  'build/ninja'), so please use just 'build', since Ninja specifics are
  mentioned in the commit message.
* Since we're going to use Ninja, please add Ninja-specific output to
  .gitignore to not spoil <git status> output.


On 02.06.22, Sergey Bronnikov wrote:
> Patch series support of Ninja to a LuaJIT build system and a new job to
> continuous integration pipeline that builds using Ninja.
> 
> On my laptop Ninja reduces building time by 14% (with Make it takes 5.7
> sec, with Ninja 3.9 sec). It is not so much, but without Ninja support
> in LuaJIT it is not possible to build Tarantool with Ninja.
> 
> Branch: https://github.com/tarantool/luajit/tree/ligurio/ninja-support
> CI status: https://github.com/tarantool/luajit/commit/acfd7552f1b8428242a6b8cbc783ed584c21beef
> 
> Sergey Bronnikov (5):
>   build/ninja: refactoring
>   build/ninja: create target with cli binary only once
>   build/ninja: rename default target
>   build/ninja: create file lists outside of cmake commands
>   ci: add job with build using Ninja on linux-x86_64
> 
>  .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>  src/CMakeLists.txt                       | 46 ++++++++-------------
>  src/host/CMakeLists.txt                  |  6 ++-
>  3 files changed, 72 insertions(+), 31 deletions(-)
>  create mode 100644 .github/workflows/linux-x86_64-ninja.yml
> 
> -- 
> 2.25.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Sergey Bronnikov via Tarantool-patches]

Thanks for review!

Comments inline.

On 15.06.2022 11:47, Igor Munkin wrote:
> Sergey,
>
> Thanks for the series! I've left some nits for 4th and 5th patches in
> the relevant threads. Everything related to the first three patches you
> can find below.
>
> Thanks for the series! You can find my comments in the relevant patches.
> However, I have one more: since we're going to use Ninja, please add
> Ninja-specific output to .gitignore to not spoil <git status> output.
Added.
>
> I tried to find more elegant solution for this case, but failed. Look
> like we definitely have no option other than define all target names
> conditionally. The related patches look good, but I propose some minor
> changes:
> * Please merge the first three patches into one, since they all related
>    to the same fix: provide unique names for targets building both LuaJIT
>    library and binary.
Done
> * LUAJIT_DEPS looks irrelevant if LUAJIT_BIN is introduced: the original
>    purpose of this variable was to template the dependency of <luajit>
>    target.

Removed LUAJIT_DEPS:

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 9003a4cf..0aae55a1 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -315,13 +315,11 @@ target_link_libraries(libluajit_shared ${TARGET_LIBS})

  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()
@@ -341,10 +339,10 @@ 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)
+set(LUAJIT_BINARY $<TARGET_FILE:${LUAJIT_BIN}> PARENT_SCOPE)

  add_custom_target(libluajit DEPENDS ${LIBLUAJIT_DEPS})
-add_custom_target(luajit-main ALL DEPENDS libluajit ${LUAJIT_DEPS})
+add_custom_target(luajit-main ALL DEPENDS libluajit ${LUAJIT_BIN})

  # Unfortunately, CMake provides no guarantees for install commands
  # used for the targets excluded from <all> and obliges user to
@@ -352,8 +350,8 @@ add_custom_target(luajit-main ALL DEPENDS libluajit 
${LUAJIT_DEPS})
  # targets used below are presented for the chosen build mode.
  # See more info in CMake docs below:
  # https://cmake.org/cmake/help/v3.1/prop_tgt/EXCLUDE_FROM_ALL.html
-if(TARGET ${LUAJIT_DEPS})
-  install(TARGETS ${LUAJIT_DEPS}
+if(TARGET ${LUAJIT_BIN})
+  install(TARGETS ${LUAJIT_BIN}
      RUNTIME
      DESTINATION bin
      COMPONENT luajit-main

> * Please adjust install components, since you've changed the default
>    target name to <luajit-main>

Fixed

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index c3fa27cf..9003a4cf 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -356,7 +356,7 @@ if(TARGET ${LUAJIT_DEPS})
    install(TARGETS ${LUAJIT_DEPS}
      RUNTIME
      DESTINATION bin
-    COMPONENT luajit
+    COMPONENT luajit-main
    )
  endif()

@@ -364,7 +364,7 @@ if(TARGET ${LIBLUAJIT_STATIC_DEPS})
    install(TARGETS ${LIBLUAJIT_STATIC_DEPS}
      ARCHIVE
      DESTINATION lib
-    COMPONENT luajit
+    COMPONENT luajit-main
    )
  endif()

@@ -372,7 +372,7 @@ if(TARGET ${LIBLUAJIT_SHARED_DEPS})
    install(TARGETS ${LIBLUAJIT_SHARED_DEPS}
      LIBRARY
      DESTINATION lib
-    COMPONENT luajit
+    COMPONENT luajit-main
    )
  endif()

@@ -389,7 +389,7 @@ install(FILES
      OWNER_READ OWNER_WRITE
      GROUP_READ
      WORLD_READ
-  COMPONENT luajit
+  COMPONENT luajit-main
  )

  install(FILES
@@ -415,5 +415,5 @@ install(FILES
      OWNER_READ OWNER_WRITE
      GROUP_READ
      WORLD_READ
-  COMPONENT luajit
+  COMPONENT luajit-main
  )

>
> The last general nits:
> * We don't use complex prefixes for commit subjects (like
>    'build/ninja'), so please use just 'build', since Ninja specifics are
>    mentioned in the commit message.
Fixed.
> * Since we're going to use Ninja, please add Ninja-specific output to
>    .gitignore to not spoil <git status> output.
Added.
>
> On 02.06.22, Sergey Bronnikov wrote:
>> Patch series support of Ninja to a LuaJIT build system and a new job to
>> continuous integration pipeline that builds using Ninja.
>>
>> On my laptop Ninja reduces building time by 14% (with Make it takes 5.7
>> sec, with Ninja 3.9 sec). It is not so much, but without Ninja support
>> in LuaJIT it is not possible to build Tarantool with Ninja.
>>
>> Branch: https://github.com/tarantool/luajit/tree/ligurio/ninja-support
>> CI status: https://github.com/tarantool/luajit/commit/acfd7552f1b8428242a6b8cbc783ed584c21beef
>>
>> Sergey Bronnikov (5):
>>    build/ninja: refactoring
>>    build/ninja: create target with cli binary only once
>>    build/ninja: rename default target
>>    build/ninja: create file lists outside of cmake commands
>>    ci: add job with build using Ninja on linux-x86_64
>>
>>   .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>>   src/CMakeLists.txt                       | 46 ++++++++-------------
>>   src/host/CMakeLists.txt                  |  6 ++-
>>   3 files changed, 72 insertions(+), 31 deletions(-)
>>   create mode 100644 .github/workflows/linux-x86_64-ninja.yml
>>
>> -- 
>> 2.25.1
>>

Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Igor Munkin via Tarantool-patches]

Sergey,

I've checked the patch into all long-term branches in tarantool/luajit
and bumped a new version in master, 2.10 and 1.10.

On 02.06.22, Sergey Bronnikov wrote:
> Patch series support of Ninja to a LuaJIT build system and a new job to
> continuous integration pipeline that builds using Ninja.
> 
> On my laptop Ninja reduces building time by 14% (with Make it takes 5.7
> sec, with Ninja 3.9 sec). It is not so much, but without Ninja support
> in LuaJIT it is not possible to build Tarantool with Ninja.
> 
> Branch: https://github.com/tarantool/luajit/tree/ligurio/ninja-support
> CI status: https://github.com/tarantool/luajit/commit/acfd7552f1b8428242a6b8cbc783ed584c21beef
> 
> Sergey Bronnikov (5):
>   build/ninja: refactoring
>   build/ninja: create target with cli binary only once
>   build/ninja: rename default target
>   build/ninja: create file lists outside of cmake commands
>   ci: add job with build using Ninja on linux-x86_64
> 
>  .github/workflows/linux-x86_64-ninja.yml | 51 ++++++++++++++++++++++++
>  src/CMakeLists.txt                       | 46 ++++++++-------------
>  src/host/CMakeLists.txt                  |  6 ++-
>  3 files changed, 72 insertions(+), 31 deletions(-)
>  create mode 100644 .github/workflows/linux-x86_64-ninja.yml
> 
> -- 
> 2.25.1
> 

-- 
Best regards,
IM

Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja

[Sergey Bronnikov via Tarantool-patches]

On 20.06.2022 15:48, Igor Munkin wrote:
> Sergey,
>
> I've checked the patch into all long-term branches in tarantool/luajit
> and bumped a new version in master, 2.10 and 1.10.
>
Nice! Thank you!