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

[Sergey Bronnikov]

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:
>> lua-Harness and tarantool testsuites uses a prove(1) for running tests.
> Typo: s/testsuites/test suites/.
Already spotted by Sergey and fixed.
>
> Typo: s/a prove/prove/ since it's program name.
Fixed.

>
>> prove(1) allows to run tests in parallel with option "--jobs" [1].
> Minor: It's worth to mention that this option requires an argument, so
> we can't rely on the default behaviour (since there is none).
Added.
>> In CMake it is not possible to get a number of parallel jobs in CMake
> Typo: Let's leave only the latter "in CMake" occurrence.
Fixed.
>
>> passed by user with option "-j", but it allows to pass a number of
> Typo: s/"-j"/"--parallel"/, am I right?

Yes and no :) "-j" is for build tools like make and ninja and 
"--parallel" is for cmake.

Updated to make it clear.

>
>> parallel jobs with environment variable CMAKE_BUILD_PARALLEL_LEVEL [2]
>> on configuration phase. We use a value set by that environment variable
> Minor: It's better (but not obligatory) use the passive voice here, to
> get something like "The value set by that environment variable is used
> as a number of CPU threads when it was not specified by user."
>
Updated, thanks.
>> and set it to a number of CPU threads when it was not specified by user.
>> Number of CPU threads detected using builtin CMake function [3].
>>
>> NOTE: CMAKE_BUILD_PARALLEL_LEVEL has been added in a version 3.12.
> Minor: It's also worth to mention whether this envvar is just ignored or
> the error is raised.
Ignore. Added it to the sentence.
>> 1. https://perldoc.perl.org/prove
>> 2. https://cmake.org/cmake/help/latest/envvar/CMAKE_BUILD_PARALLEL_LEVEL.html
>> 3. https://cmake.org/cmake/help/latest/module/ProcessorCount.html
>> ---
>>
>> Branch: https://github.com/tarantool/luajit/tree/ligurio/prove-in-parallel
>> CI: https://github.com/tarantool/luajit/commit/62acf609d9e1ea43d95cb27213e1b8f1331b57a7
>>
>>   test/CMakeLists.txt                   | 13 +++++++++++++
>>   test/lua-Harness-tests/CMakeLists.txt |  1 +
>>   test/tarantool-tests/CMakeLists.txt   |  1 +
>>   3 files changed, 15 insertions(+)
>>
>> 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.

>> +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?


>
>> +  endif()
>> +endif()
>> +message(STATUS "Using CMAKE_BUILD_PARALLEL_LEVEL: ${CMAKE_BUILD_PARALLEL_LEVEL}")
>> +
>>   add_subdirectory(LuaJIT-tests)
>>   add_subdirectory(PUC-Rio-Lua-5.1-tests)
>>   add_subdirectory(lua-Harness-tests)
> <snipped>
>
>> -- 
>> 2.25.1
>>