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

[Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Igor,

thanks for review!


On 06.07.2022 00:22, Igor Munkin wrote:
> 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.

Moved to a separate cmake module cmake/SetBuildParallelLevel.cmake.


diff --git a/CMakeLists.txt b/CMakeLists.txt
index 659ee879..1a7f62aa 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -30,10 +30,13 @@ endif()
  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")

  include(LuaJITUtils)
+include(SetBuildParallelLevel)
  include(SetVersion)

  # --- Variables to be exported to child scopes 
---------------------------------

+SetBuildParallelLevel()
+message(STATUS "[CMakeBuildParallelLevel] CMAKE_BUILD_PARALLEL_LEVEL is 
${CMAKE_BUILD_PARALLEL_LEVEL}")
  SetVersion(
    LUAJIT_VERSION
    LUAJIT_VERSION_MAJOR
diff --git a/cmake/SetBuildParallelLevel.cmake 
b/cmake/SetBuildParallelLevel.cmake
new file mode 100644
index 00000000..97d7b7b0
--- /dev/null
+++ b/cmake/SetBuildParallelLevel.cmake
@@ -0,0 +1,14 @@
+function(SetBuildParallelLevel)
+  set(CMAKE_BUILD_PARALLEL_LEVEL $ENV{CMAKE_BUILD_PARALLEL_LEVEL} 
PARENT_SCOPE)
+  if(NOT CMAKE_BUILD_PARALLEL_LEVEL)
+    set(CMAKE_BUILD_PARALLEL_LEVEL 1 PARENT_SCOPE)
+    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} PARENT_SCOPE)
+    endif()
+  endif()
+endfunction()
diff --git a/test/lua-Harness-tests/CMakeLists.txt 
b/test/lua-Harness-tests/CMakeLists.txt
index ec08a19b..12476171 100644
--- a/test/lua-Harness-tests/CMakeLists.txt
+++ b/test/lua-Harness-tests/CMakeLists.txt
@@ -28,6 +28,7 @@ add_custom_command(TARGET lua-Harness-tests
      LUA_PATH="${LUA_PATH}\;"
      ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
        --exec '${LUAJIT_TEST_COMMAND} -l profile_luajit21'
+      --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
        ${LUA_TEST_FLAGS}
    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
  )
diff --git a/test/tarantool-tests/CMakeLists.txt 
b/test/tarantool-tests/CMakeLists.txt
index 5708822e..ecda2e63 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -136,6 +136,7 @@ add_custom_command(TARGET tarantool-tests
      ${PROVE} ${CMAKE_CURRENT_SOURCE_DIR}
        --exec 'env ${LUA_TEST_ENV_MORE} ${LUAJIT_TEST_COMMAND}'
        --ext ${LUA_TEST_SUFFIX}
+      --jobs ${CMAKE_BUILD_PARALLEL_LEVEL}
        ${LUA_TEST_FLAGS}
    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
  )


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

Added in a separate commit.


Also added a commit with fixup with replacing hw.ncpu to hw.logicalcpu.

>