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

[Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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