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

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

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