From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Igor Munkin <imun@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] build: configure parallel jobs Date: Mon, 4 Jul 2022 17:21:05 +0300 [thread overview] Message-ID: <721f8709-cf2b-b2df-0672-cfd6f9cf1efb@gmail.com> (raw) In-Reply-To: <YsK9IoVji078busi@tarantool.org> 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 >>
next prev parent reply other threads:[~2022-07-04 14:21 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-01 14:36 Sergey Bronnikov via Tarantool-patches 2022-07-04 9:52 ` Sergey Kaplun via Tarantool-patches 2022-07-04 10:06 ` Sergey Bronnikov via Tarantool-patches 2022-07-04 10:12 ` Igor Munkin via Tarantool-patches 2022-07-04 14:21 ` Sergey Bronnikov via Tarantool-patches [this message] 2022-07-05 21:22 ` Igor Munkin via Tarantool-patches 2022-07-06 10:32 ` Sergey Bronnikov via Tarantool-patches 2022-07-13 10:10 ` Igor Munkin via Tarantool-patches
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=721f8709-cf2b-b2df-0672-cfd6f9cf1efb@gmail.com \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=imun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] build: configure parallel jobs' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox