Re: [Tarantool-patches] [PATCH] build: configure parallel jobs

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

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