[Tarantool-patches] [PATCH] build: configure parallel jobs
Igor Munkin
imun at tarantool.org
Mon Jul 4 13:12:50 MSK 2022
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
More information about the Tarantool-patches
mailing list