Tarantool development patches archive
 help / color / mirror / Atom feed
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
>>

  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