Tarantool development patches archive
 help / color / mirror / Atom feed
From: Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: "Sergey Bronnikov" <sergeyb@tarantool.org>
Cc: "Sergey Bronnikov" <estetus@gmail.com>,
	max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches]  [PATCH luajit][v2] cmake: replace prove with CTest
Date: Thu, 16 Nov 2023 19:15:41 +0300	[thread overview]
Message-ID: <1700151341.971708902@f56.i.mail.ru> (raw)
In-Reply-To: <7d138847-b21b-4de4-9608-86deff7b38db@tarantool.org>

[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Суббота, 11 ноября 2023, 20:05 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>Hi, Igor
>
>On 10/31/23 22:17, Igor Munkin wrote:
>> Sergey,
>>
>> Thanks for the patch! I have some general comments prior to make more
>> precise review for the particular changeset.
>>
>> 1. Unfortunately, the old scenario is broken:
>> The recipe I used to run for building and running LuaJIT test is the
>> following one:
>> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON && make -j test
>> As a result of the changes, <test> target doesn't build the
>> dependencies (neither luajit per se, nor particular test helpers
>> required to be compiled). So the recipe is transformed to this one:
>> | $ cmake . -DCMAKE_BUILD_TYPE=Debug -DLUA_USE_ASSERT=ON -DLUA_USE_APICHECK=ON
>> | $ make -j # build LuaJIT
>> | $ make -j tarantool-tests # run the particular suite to build the requirements
>> | $ make -j tarantool-c-tests # ditto
>> | $ make -j PUC-Rio-Lua-5.1-tests # some binary artefacts for Lua suite
>> | $ make -j test
>> Looks not such convenient comparing to the old mantra.
>
>Updated patch a bit. Now steps are the same:
>
>     $ cmake -S . -B build
>     $ cmake --build build
>     $ cmake --build --target LuaJIT-test
>
>ctest and test target works too.
>
>
>>
>> 2. The new CTest targets should follow the existing naming practice
>> (i.e. use the directory name, or its distinctive prefix), since the
>> following command runs three suites.
>> | $ ctest -L lua # runs lua, luajit and lua-harness
>Updated labels, now labels are equals to prefix of appropriate testsuite.
>>
>> 3. It looks like the logs will not be showed in CI to debug a
>> hard-reproducible issue if one occurs. <--output-on-failure> can be a
>> saviour here for us.
>
>This option is already used by test targets, see TEST_FLAGS in
>test/CMakeLists.txt.
>
>If you run tests by ctest then you should set env variable
>CTEST_OUTPUT_ON_FAILURE=1.
>
>Another option is setting default ctest options via
>CMAKE_CTEST_ARGUMENTS, but it is available since CMake 3.17.
>
>>
>> 4. I'm afraid the current changes break integration with Tarantool
>> tests. The reason is duplication of <test> target both in LuaJIT and
>> Tarantool build systems. AFAIU, moving <enable_testing> under the
>> condition similar to the one handled with LUAJIT_USE_TEST option
>> doesn't solve the issue, since no test will be run as a result.
>> Please check this part: it might be the main technical blocker for
>> this changeset.
>
>No, it doesn't break integration. Letter with patch has  a link to PR to
>tarantool repository,
>
>all CI tests are passed.
>
>> 5. I remember that we're talking regarding running particular tests for
>> all suites, but I see this is done only for tarantool* and lua-Harness.
>> Is it experimental and you'll fix it later in scope of this PR? There
>> is definitely the way to run a particular test in LuaJIT suite
>> (AFAIR, you can just use its number). Regarding PUC-Rio-Lua-5.1 we
>> can think about replacing all.lua as a runner as it's done in uJIT.
>
>The patch replaces prove with ctest. Updating test runner for PUC Rio
>Lua and LuaJIT testsuites
>
>is out of scope for this patch. This could be done later and by someone
>else.
>
>>
>> I believe that's all for the starters.
 

[-- Attachment #2: Type: text/html, Size: 4262 bytes --]

  reply	other threads:[~2023-11-16 16:15 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-23  9:48 Sergey Bronnikov via Tarantool-patches
2023-10-24 13:41 ` Maxim Kokryashkin via Tarantool-patches
2023-10-31 10:42   ` Sergey Bronnikov via Tarantool-patches
2023-10-31 19:17 ` Igor Munkin via Tarantool-patches
2023-11-11 17:05   ` Sergey Bronnikov via Tarantool-patches
2023-11-16 16:15     ` Maxim Kokryashkin via Tarantool-patches [this message]
2024-03-10 20:03 ` Igor Munkin via Tarantool-patches
2024-03-12  7:09   ` Sergey Bronnikov 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=1700151341.971708902@f56.i.mail.ru \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=estetus@gmail.com \
    --cc=m.kokryashkin@tarantool.org \
    --cc=max.kokryashkin@gmail.com \
    --cc=sergeyb@tarantool.org \
    --subject='Re: [Tarantool-patches]  [PATCH luajit][v2] cmake: replace prove with CTest' \
    /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