Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Sergey Bronnikov <estetus@gmail.com>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] build: configure parallel jobs
Date: Wed, 6 Jul 2022 00:22:51 +0300	[thread overview]
Message-ID: <YsSrq0DZ05eVvFL9@tarantool.org> (raw)
In-Reply-To: <721f8709-cf2b-b2df-0672-cfd6f9cf1efb@gmail.com>

Sergey,

Thanks for the fixes. LGTM, with a couple more nits.

On 04.07.22, Sergey Bronnikov wrote:
> Igor, thanks for review!
> 
> On 04.07.2022 13:12, Igor Munkin wrote:
> > Sergey,
> >
> > Thanks for the patch! Please consider my several nits below.
> >
> > On 01.07.22, Sergey Bronnikov wrote:

<snipped>

> >> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> >> index ba25af54..fc4ffc51 100644
> >> --- a/test/CMakeLists.txt
> >> +++ b/test/CMakeLists.txt
> >> @@ -43,6 +43,19 @@ endif()
> >>   set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")
> >>   separate_arguments(LUAJIT_TEST_COMMAND)
> >>   
> > I guess all code below can be moved to a function somewhere in cmake/
> > and its usage -- to the root CMakeLists.txt. Thoughts?
> 
> Agree. Initial idea was to parallelize tests only,
> 
> but with CMAKE_BUILD_PARALLEL_LEVEL it works for building process too.
> 
> Moved hunk to the root CMakeLists.txt right after building compile options
> 
> and flags and before adding subdirectories, because code in lower levels 
> can depend on this option.

I still believe this should be moved to a separate module in cmake/
directory (like it's done for SetVersion). Please, also move it's usage
to "Fine-tuning cmake environment" section in root CMakeLists.txt to
group most of includes there.

> 
> >> +set(CMAKE_BUILD_PARALLEL_LEVEL $ENV{CMAKE_BUILD_PARALLEL_LEVEL})
> >> +if(NOT CMAKE_BUILD_PARALLEL_LEVEL)
> >> +  include(ProcessorCount)
> >> +  ProcessorCount(N)
> >> +  # Function ProcessorCount() is guaranteed to return a positive integer (>=1)
> >> +  # if it succeeds. It returns 0 if there's a problem determining the processor
> >> +  # count.
> >> +  if(NOT N EQUAL 0)
> >> +    set(CMAKE_BUILD_PARALLEL_LEVEL ${N})
> > I believe we need else branch here to set CMAKE_BUILD_PARALLEL_LEVEL, if
> > ProcessorCount yields zero.
> 
> I would do it without additional branch. Like this:
> 
>    set(CMAKE_BUILD_PARALLEL_LEVEL 1)
>    if(NOT N EQUAL 0)
>      set(CMAKE_BUILD_PARALLEL_LEVEL ${N})
>    endif()
> 
> Are you ok with this?

Great, thanks!

> 
> 

<snipped>

> >> -- 
> >> 2.25.1
> >>

All in all, if you've introduced the machinery to control parallel level
of CMake jobs, we can also fix our workflows in a separate commit by
changing --parallel parameter with CMAKE_BUILD_PARALLEL_LEVEL envvar. So
the following approach...
| $ cmake .
| $ cmake . --build --parallel $(nproc) --target test
... is replaced with the next one...
| $ CMAKE_BUILD_PARALLEL_LEVEL=$(nproc) cmake .
| $ cmake . --build --parallel
... considering the comment[1] in CMAKE_BUILD_PARALLEL_LEVEL manual.

[1]: https://cmake.org/cmake/help/latest/envvar/CMAKE_BUILD_PARALLEL_LEVEL.html

-- 
Best regards,
IM

  reply	other threads:[~2022-07-05 21:22 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-01 14:36 Sergey Bronnikov via Tarantool-patches
2022-07-04  9:52 ` Sergey Kaplun via Tarantool-patches
2022-07-04 10:06   ` Sergey Bronnikov via Tarantool-patches
2022-07-04 10:12 ` Igor Munkin via Tarantool-patches
2022-07-04 14:21   ` Sergey Bronnikov via Tarantool-patches
2022-07-05 21:22     ` Igor Munkin via Tarantool-patches [this message]
2022-07-06 10:32       ` Sergey Bronnikov via Tarantool-patches
2022-07-13 10:10 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YsSrq0DZ05eVvFL9@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=imun@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] build: configure parallel jobs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox