From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 85D756ECE3; Wed, 15 Jun 2022 17:57:20 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 85D756ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1655305040; bh=r8L89Gy6/UUUBksmeMBGJXniEN3OQABDtQ6KMU6rUbE=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qmYpdz1rfrHZmenlF9AwZ9HXrjB2aO/t3rjpxL4P7uuK4eg8TG6jvbVO5urNENEch tpvACiDvS+hNsKBFcAgR32i1xAK/zA+zOaIzRmn9LP4r3OFvZr1C9nYjED9Eh6kdID JDzGkG/4rY2UEcZbmJJYoFQJuwr94BL+hunWzh2g= Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F34C96ECE3 for ; Wed, 15 Jun 2022 17:57:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F34C96ECE3 Received: by mail-lf1-f46.google.com with SMTP id p18so19380505lfr.1 for ; Wed, 15 Jun 2022 07:57:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=DuW3gOYiEA3qLTw6jLuCe7vCLfvcbMLzwTTmYKBjcMM=; b=Z694+S1zUHVkPcI+wBBgXMYa8BFqM/h8HGdS6l6rOZysKA/WDJbPapwZBPzqFrDR0a ewAWjA11gj4E1Ub/PdsH419AP7BlHmUUyX0ts7g3Dv7C0Sm+2I2nAJtrbVQXVsW0UUGi Z65NrBehIzAAaXx4WMRqy2rJiHSjJvHQ6GECIBA9F7XKc5YEAU9UY8d4l6FKV21uPnU4 4MBnMpg7ff1WIomMEIleBNS7aDU50CsWzHmpVW4BBNZw3cH78nP6c7UjFtcl1urPXz0L I95qaJDOZVeSVChuyvHyT/9UEPIYzNdCWUROKg4l8M/Fzl5+RsOlkqbUB8J90WtNiakv urGA== X-Gm-Message-State: AJIora95cATF+kccEceMboSOnfGB09CoKlStF/SLTSFhdZCKGzrB/ZtY zEwo+qAvw5+JcmhGoWJEfQitgt+8RaM= X-Google-Smtp-Source: AGRyM1v0xFLDXBpsLzSAda8iuvzUOb03GJJ4gHwqczVcS84O4nnRTK0RKDLGyceLHFYYbv8vTjLsCA== X-Received: by 2002:a19:ca0c:0:b0:479:46c:2917 with SMTP id a12-20020a19ca0c000000b00479046c2917mr6513949lfg.160.1655305038278; Wed, 15 Jun 2022 07:57:18 -0700 (PDT) Received: from [10.10.1.140] ([79.164.223.111]) by smtp.gmail.com with ESMTPSA id e20-20020a05651236d400b00477cae4880fsm828502lfs.260.2022.06.15.07.57.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 15 Jun 2022 07:57:17 -0700 (PDT) Message-ID: <14e6aa89-40a7-7412-e3ee-4cb83c0b4334@gmail.com> Date: Wed, 15 Jun 2022 17:57:15 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Content-Language: en-US To: Igor Munkin References: In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [v2][PATCH 0/5] Support building with Ninja X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 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 > 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 $ PARENT_SCOPE) +set(LUAJIT_BINARY $ 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 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 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 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 >>