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

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

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/.

Typo: s/a prove/prove/ since it's program name.

> 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).

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

> passed by user with option "-j", but it allows to pass a number of

Typo: s/"-j"/"--parallel"/, am I right?

> 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."

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

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

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

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

-- 
Best regards,
IM