[Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja
Sergey Bronnikov
estetus at gmail.com
Wed Jun 15 17:57:15 MSK 2022
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
>>
More information about the Tarantool-patches
mailing list