[Tarantool-patches] [PATCH] build: configure parallel jobs
Igor Munkin
imun at tarantool.org
Wed Jul 6 00:22:51 MSK 2022
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
More information about the Tarantool-patches
mailing list