From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id DE29D9C55AB; Wed, 20 Mar 2024 22:44:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DE29D9C55AB DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1710963846; bh=JuwqtpRqz1A/Rvdtd7sFPOjXTbS3LhSCbBQoptCmQrY=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=IMXXg02U6TTgFhPVr+Y9ac+hbdmiX1Lb0nvfPExAlqAZS4OZPvR5fB3cEQVTxqTnL Mv+tcq7cPF49Xvd4Z5ZqPXY7OnPjsd7rnbBQ5GRZ9W9SI22IoO0C5ck1e3ETDYzuF3 IhvqJjyStndSMTf14xd/3PokaHdV8fmKbD8fnT3Q= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [95.163.41.100]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id EC9486B9F20 for ; Wed, 20 Mar 2024 22:44:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EC9486B9F20 Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1rn1r1-0000000CaVV-0kVh; Wed, 20 Mar 2024 22:44:03 +0300 Message-ID: Date: Wed, 20 Mar 2024 22:44:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: Sergey Kaplun , Sergey Bronnikov References: <62428a61c98eee87dfc91f2d8972988d80589d3e.1710226958.git.sergeyb@tarantool.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD987C0EE6E7F0A597D77DAD88DB19A0E871D0E83388B9166C4182A05F538085040117CF330883D17C20578E6996F3834130B4ED8E048C6EB3A4ECF044C966F7DED28D9CF984B353DE0 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE760302A529BCAAAFCEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370C23C96D810E96D08638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D883A801D3D5B95F4823E79127FB208330ADDED392FACF4C8ACC7F00164DA146DAFE8445B8C89999728AA50765F7900637D0FEED2715E18529389733CBF5DBD5E9C8A9BA7A39EFB766F5D81C698A659EA7CC7F00164DA146DA9985D098DBDEAEC81D471462564A2E19F6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CB355ED1E20F5346AAD7EC71F1DB884274AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C30F2F4F1D5A14369CBA3038C0950A5D36B5C8C57E37DE458B330BD67F2E7D9AF16D1867E19FE14079C09775C1D3CA48CFE478A468B35FE7671DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3CDD845A7E6742F4735872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-C1DE0DAB: 0D63561A33F958A5CC3E086D778C423F5002B1117B3ED696691303A940AC78F78B25839F35DFE037823CB91A9FED034534781492E4B8EEAD86106675DE625196BDAD6C7F3747799A X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D349A949488F6BF46DDCBB9CB717B49A823E62A309B8B4B139CF0BF53696157A7C3581A350CFF5656391D7E09C32AA3244C977A2DA53BC0EC204EB7DF677E51682D0E43AEBB008F8348EA455F16B58544A2D06CB91D864A7BD2A5AE236DF995FB59978A700BF655EAEEED6A17656DB59BCAD427812AF56FC65B X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojxQwOrI9ubONKVevhJHKe1g== X-Mailru-Sender: C4F68CFF4024C8867DFDF7C7F2588458B99E73ACEA671B49AA70CB78A12CDBB514434905F0762870E4690EE79E431205645D15D82EE4B272BD6E4642A116CA93524AA66B5ACBE6721EF430B9A63E2A504198E0F3ECE9B5443453F38A29522196 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit][v3] cmake: replace prove with CTest X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Sergey, thanks for review! see my comments below On 3/18/24 18:54, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > I'll proceed with a review within the original letter. > > I look forward to applying these changes! > I hope we can use the full power of CTest. It will be nice to configure > heavy-memory tests to avoid running them together in parallel and > hanging the system:). > > Please, consider my comments below. > > On 12.03.24, Sergey Bronnikov wrote: >> From: Sergey Bronnikov >> >> The patch replaces the main test runner `prove(1)` with CTest, >> see [1] and [2]. >> >> Build and test steps looks the same as before: >> >> $ cmake -S . -B build >> $ cmake --build build --parallel >> $ cmake --build build --target [LuaJIT-test, test] >> >> CMake targets lua-Harness-tests, LuaJIT-tests, PUC-Rio-Lua-5.1-tests, >> tarantool-tests and tarantool-c-tests are still functional. >> >> One could use `ctest` tool: >> >> $ ctest --print-labels >> Test project >> All Labels: >> LuaJIT >> PUC-Rio-Lua-5.1 >> lua-Harness >> tarantool >> tarantool-c >> $ ctest # Run all tests. >> $ ctest -L tarantool # Run tests in tarantool-tests dir. >> $ ctest -L tarantool-c # Run tests in tarantool-c-tests dir. >> $ ctest -L lua-Harness # Run tests in lua-Harness-tests dir. >> $ ctest -L luajit # Run tests in LuaJIT-tests dir. >> $ ctest -L PUC-Rio-Lua-5.1 # Run tests in PUC-Rio-Lua-5.1-tests dir. > Please, remove the excess amount of spaces (9) in the formatting to make > the commit message fits 72 symbols.  Fixed. > > Same for the paragraphs below. Fixed. >> $ ctest --rerun-fail >> $ ctest --output-on-failure >> >> The benefits are: >> >> - less external dependencies, `prove(1)` is gone, `ctest` is a part of CMake >> - running tests by test title >> - extended test running options in comparison to prove(1) >> - unified test output (finally!) >> >> Note that it is not possible to attach targets to the automatically >> generated `test` target. It means that target `test` is now executing >> `LuaJIT-test`, but not `LuaJIT-lint`. > Is it possible to introduce a new target like test-all to join them? Discussed target name in a private conversation and decided to introduce "LuaJIT-check-all". --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -140,3 +140,8 @@ add_custom_target(${PROJECT_NAME}-test            PUC-Rio-Lua-5.1-tests-deps            LuaJIT-tests-deps  ) + +add_custom_target(${PROJECT_NAME}-check-all +  DEPENDS ${PROJECT_NAME}-test +          ${PROJECT_NAME}-lint +) > >> Note that the testsuites in `test/LuaJIT-tests` and >> `test/PUC-Rio-Lua-5.1` directories have their own test runners written >> in Lua and currently CTest runs these testsuites as single tests. In the > Typo: s/Lua/Lua,/ > Added comma. >> future we could change these test runners to specify tests from outside, > Typo: s/future/future,/ Added comma. > >> and after this we could run tests separately by CTest in these test > Typo: s/this/this,/ Added comma. > >> suites. >> >> Note that it is not possible to add dependencies to `add_test()` in >> CMake, see [3]. CMake 3.7 introduces FIXTURES_REQUIRED [4] and >> FIXTURES_SETUP [5], but these test properties cannot be used - used >> CMake version is lower. This workarounded by introducing a special test > Typo: s/workarounded/is workaraounded/ Typo is in "workaraounded". Fixed. > >> for each testsuite that executes a target that builds test dependencies. >> >> 1. https://cmake.org/cmake/help/latest/manual/ctest.1.html >> 2. https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html >> 3. https://gitlab.kitware.com/cmake/cmake/-/issues/8774 >> 4. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html >> 5. https://cmake.org/cmake/help/latest/prop_test/FIXTURES_SETUP.html >> --- >> Changes v3: >> - rebased to master >> - applied fixes suggested by Igor Munkin >> >> PR: https://github.com/tarantool/tarantool/pull/9286 >> Branch: https://github.com/tarantool/luajit/tree/ligurio/enable_test_target >> >> .gitignore | 2 + >> CMakeLists.txt | 11 ++++ >> test/CMakeLists.txt | 78 +++++++++++++++-------- >> test/LuaJIT-tests/CMakeLists.txt | 40 +++++++----- >> test/LuaJIT-tests/src/CMakeLists.txt | 2 +- >> test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt | 25 +++++--- >> test/lua-Harness-tests/CMakeLists.txt | 48 ++++++++------ >> test/tarantool-c-tests/CMakeLists.txt | 56 ++++++++-------- >> test/tarantool-tests/CMakeLists.txt | 62 ++++++++---------- >> 9 files changed, 194 insertions(+), 130 deletions(-) >> >> diff --git a/.gitignore b/.gitignore >> index dc5ea5fc..dd1f8888 100644 >> --- a/.gitignore >> +++ b/.gitignore > > >> diff --git a/CMakeLists.txt b/CMakeLists.txt >> index 7f5e2afb..6b2f855f 100644 >> --- a/CMakeLists.txt >> +++ b/CMakeLists.txt >> @@ -345,6 +345,13 @@ set(LUAJIT_TEST_BINARY ${LUAJIT_BINARY} CACHE STRING >> "Lua implementation to be used for tests. Default is 'luajit'." >> ) >> >> +# If LuaJIT is used in a parent project, provide an option >> +# to choose what dependencies to be used for running and building >> +# LuaJIT tests. >> +set(LUAJIT_TEST_DEPS "luajit-main" CACHE STRING >> + "A list of target dependencies to be used for tests." >> +) > Why do we use luajit-main as a target dependency for tests? > Why can't LUAJIT_TEST_BINARY be used instead where it is necessary (not > in tarantool-c-tests)? > >> + >> # FIXME: If LuaJIT is used in parent project, provide an option >> # to pass Lua code to be run before tests are started. >> # XXX: Attentive reader might point to LUA_INIT environment >> @@ -362,6 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR >> "Lua code need to be run before tests are started." >> ) >> >> +# XXX: Enable testing via CTest. Since CTest expects to find a >> +# test file in the build directory root, this command should be in > Nit: I suggest to mentioning that this command generates "test" target. > Something like the following: > | # XXX: Enable testing via CTest. This generates `test` target. Added: --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -369,9 +369,10 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR    "Lua code need to be run before tests are started."  ) -# XXX: Enable testing via CTest. Since CTest expects to find a -# test file in the build directory root, this command should be in -# the source directory root (hence, in this CMakeLists.txt). +# XXX: Enable testing via CTest. This automatically generates +# `test` target. Since CTest expects to find a test file in +# the build directory root, this command should be in the source +# directory root (hence, in this CMakeLists.txt).  enable_testing()  add_subdirectory(test) > >> +# the source directory root (hence, in this CMakeLists.txt). >> +enable_testing() > Should we generate the `test` target only if the `LUAJIT_USE_TEST` > option is set, like it was done before (see relevant changes in the > )? Added option LUAJIT_USE_TEST back: --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -373,7 +373,9 @@ set(LUAJIT_TEST_INIT "${PROJECT_SOURCE_DIR}/test/luajit-test-init.lua" CACHE STR  # `test` target. Since CTest expects to find a test file in  # the build directory root, this command should be in the source  # directory root (hence, in this CMakeLists.txt). -enable_testing() +if (LUAJIT_USE_TEST) +  enable_testing() +endif()  add_subdirectory(test)  # --- Misc rules --------------------------------------------------------------- >> add_subdirectory(test) >> >> # --- Misc rules --------------------------------------------------------------- >> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >> index 3ad5d15f..90aaead2 100644 >> --- a/test/CMakeLists.txt >> +++ b/test/CMakeLists.txt >> @@ -77,35 +77,63 @@ separate_arguments(LUAJIT_TEST_COMMAND) >> set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") >> include(AddTestLib) >> >> +# TEST_FLAGS is used by CMake targets in test suites. >> +# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains CTest options > Nit: the comment line is wider than 66 symbols. Already fixed. > >> +# and is used by `test` target. >> +set(TEST_FLAGS > Minor: Maybe it is better to name this variable as `CTEST_FLAGS`, to > avoid confusion that these flags are arguments for some internal test > runner? diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 899bf31a..ca298118 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -77,16 +77,16 @@ separate_arguments(LUAJIT_TEST_COMMAND)  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")  include(AddTestLib) -# TEST_FLAGS is used by CMake targets in test suites. +# CTEST_FLAGS is used by CMake targets in test suites.  # XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains  # CTest options and is used by `test` target. -set(TEST_FLAGS +set(CTEST_FLAGS    --output-on-failure    --schedule-random    --parallel ${CMAKE_BUILD_PARALLEL_LEVEL}  )  if(CMAKE_VERBOSE_MAKEFILE) -  list(APPEND TEST_FLAGS --verbose) +  list(APPEND CTEST_FLAGS --verbose)  endif()  function(add_test_suite_target target) @@ -119,7 +119,7 @@ function(add_test_suite_target target)    message(STATUS "Add test suite ${ARG_LABEL}")    add_custom_target(${target} -    COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS} +    COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${CTEST_FLAGS}      DEPENDS ${_DEPS_TARGET}    ) @@ -133,7 +133,7 @@ add_subdirectory(tarantool-c-tests)  add_subdirectory(tarantool-tests)  add_custom_target(${PROJECT_NAME}-test -  COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS} +  COMMAND ${CMAKE_CTEST_COMMAND} ${CTEST_FLAGS}    DEPENDS tarantool-c-tests-deps            tarantool-tests-deps            lua-Harness-tests-deps > > Unfortunately, this flag does nothing for `make test` command. > Is there the way to fix it? Yes, in CMake 3.17+. This version introduces CMAKE_CTEST_ARGUMENTS that contains CTest options and is used by `test` target. As a comment says. BTW you can fix it by using CMake workflows and specify any CTest arguments you want in CMakePreset.json. > BTW `make LuaJIT-tests` or `make tarantool-tests` work fine. Sure, these are our own targets and we pass desired CTest options to it. >> + --output-on-failure >> + --schedule-random >> + --parallel ${CMAKE_BUILD_PARALLEL_LEVEL} > I suppose it will be good to add the `--timeout` option here too. > 5 minutes as a timeout for the single test sounds more than enough. > Added. >> +) >> +if(CMAKE_VERBOSE_MAKEFILE) >> + list(APPEND TEST_FLAGS --verbose) >> +endif() >> + >> +function(add_test_suite_target target) >> + set(prefix ARG) >> + set(noValues) >> + set(singleValues LABEL) >> + set(multiValues DEPENDS) >> + >> + # FIXME: if we update to CMake >= 3.5, can remove this line. >> + include(CMakeParseArguments) >> + cmake_parse_arguments(${prefix} >> + "${noValues}" >> + "${singleValues}" >> + "${multiValues}" >> + ${ARGN}) >> + >> + set(_DEPS_TARGET ${target}-deps) >> + >> + add_custom_target(${_DEPS_TARGET} DEPENDS ${ARG_DEPENDS}) >> + >> + add_test(${_DEPS_TARGET} > Should the NAME parameter be ${target} instead? Added. > Or, maybe this line > should be ommitted since it is also used for normal test targets with > several tests. CMake test for a target with dependencies must be generated, otherwise dependencies will not be built. > > For now `ctest -N` output looks a little bit inconsistent: > > | Test project /home/burii/reviews/luajit/ctest > | Test #1: LuaJIT-tests-deps > | Test #2: test/LuaJIT-tests > | Test #3: PUC-Rio-Lua-5.1-tests-deps > | Test #4: test/PUC-Rio-Lua-5.1 > | Test #5: lua-Harness-tests-deps > | Test #6: test/lua-Harness-tests/000-sanity.t > | ... > | Test #56: tarantool-c-tests-deps > | Test #57: test/tarantool-c-tests/fix-yield-c-hook.c_test > | ... > | Test #67: tarantool-tests-deps > | Test #68: test/tarantool-tests/arm64-ccall-fp-convention.test.lua > | ... > > If tests like the following are needed to run tests, I suggest naming > them like `LuaJIT-tests-build` to avoid confusion. AFAIR this naming was suggested by Igor. What kind of confusion you observe? For me something like `LuaJIT-tests-build`  much more confusing than "xxx-deps". > | Test #1: LuaJIT-tests-deps > > Also, as discussed with Sergey offline: > For now, we observe an error since dependencies (C libraries) for tests > are not built if we build LuaJIT and try to run ctest target like the > following (without using make for a specific test target): > > | cmake . && make -j > | ctest -L tarantool-tests --verbose > | 68: /home/burii/reviews/luajit/ctest/src/luajit: .../test/tarantool-tests/arm64-ccall-fp-convention.test.lua:4: libfficcall.so: cannot open shared object file: No such file or directory > it was fixed >> + ${CMAKE_COMMAND} >> + --build ${CMAKE_BINARY_DIR} >> + --target ${_DEPS_TARGET} >> + ) >> + >> + message(STATUS "Add test suite ${ARG_LABEL}") > Note: If we don't run this function for multi-test targets after > applying the changes required above, this STATUS message should be > copied to the correponding CMakeLists.txt. This macro executed one time for each test suite. > >> + >> + add_custom_target(${target} >> + COMMAND ${CMAKE_CTEST_COMMAND} -L ${ARG_LABEL} ${TEST_FLAGS} >> + DEPENDS ${_DEPS_TARGET} >> + ) >> + >> + unset(_DEPS_TARGET) > There is no need in `unset()` since we are using `function()` here, > which opens a new scope. Removed. > >> +endfunction() >> + >> add_subdirectory(LuaJIT-tests) >> add_subdirectory(PUC-Rio-Lua-5.1-tests) >> add_subdirectory(tarantool-c-tests) >> add_subdirectory(tarantool-tests) >> >> -add_custom_target(${PROJECT_NAME}-test DEPENDS >> - LuaJIT-tests >> - PUC-Rio-Lua-5.1-tests >> - lua-Harness-tests >> - tarantool-c-tests >> - tarantool-tests >> +add_custom_target(${PROJECT_NAME}-test >> + COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS} > Why do we need to use custom command for this test? > IINM, all "subtargets" are used the same ${TEST_FLAGS} > See also Igor's comment about this test. Answered to Igor. because output will be ugly in that case, just change as you suggest and run LuaJIT-test. > >> + DEPENDS tarantool-c-tests-deps >> + tarantool-tests-deps >> + lua-Harness-tests-deps >> + PUC-Rio-Lua-5.1-tests-deps >> + LuaJIT-tests-deps >> ) >> - >> -if(LUAJIT_USE_TEST) > Within this change the `test` target is always generated. > See the comment above. Generated when LUAJIT_USE_TEST is ON. > >> - if(POLICY CMP0037) > > >> diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt >> index a0fb5440..1080987a 100644 >> --- a/test/LuaJIT-tests/CMakeLists.txt >> +++ b/test/LuaJIT-tests/CMakeLists.txt >> @@ -3,17 +3,13 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR) >> >> add_subdirectory(src) >> >> -add_custom_target(LuaJIT-tests >> - DEPENDS ${LUAJIT_TEST_BINARY} LuaJIT-tests-libs >> -) >> - >> make_lua_path(LUA_CPATH >> PATHS >> ${CMAKE_CURRENT_BINARY_DIR}/src/?${CMAKE_SHARED_LIBRARY_SUFFIX} >> ) >> >> set(LUAJIT_TESTS_ENV >> - "LUA_CPATH=\"${LUA_CPATH}\"" >> + "LUA_CPATH=${LUA_CPATH}\;\;" >> ) > These semicolons look excessive, see also Igor's comment. > > Side note: I suppose that double quotes are used to avoid escaping > various special symbols or spaces. But for now, all these special cases > of directory names are just not working, so I see nothing crucial in > removing these quotes. already fixed and force-pushed >> >> set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:") >> @@ -50,10 +46,11 @@ if(LUAJIT_USE_ASAN) >> if(CMAKE_C_COMPILER_ID STREQUAL "GNU") >> LibRealPath(LIB_ASAN libasan.so) >> # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`. >> - # ";" will expand to " " as we want. >> - list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}") >> + # ";" will expand to " " as we want after wrapping > This is not true. After these changes the environment variable > `LD_PRELOAD` is the following: > | LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0;/usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32 > But it should be: > | LD_PRELOAD="/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32" > This is why this quotes is used. > For some reason, with the debug flags, paths are separated via \n: > | 2: LD_PRELOAD=/usr/lib/gcc/x86_64-pc-linux-gnu/13/libasan.so.8.0.0 > | 2: /usr/lib/gcc/x86_64-pc-linux-gnu/13/libstdc++.so.6.0.32 > > So, I got the following error, when I build LuaJIT with GCC and use the > flag `-DLUAJIT_ENABLE_ASAN`: > | AddressSanitizer: CHECK failed: asan_interceptors.cpp:335 "((__interception::real___cxa_throw)) != (0)" (0x0, 0x0) (tid=28264) > | #0 0x7f0c5d13b545 in CheckUnwind /var/tmp/portage/sys-devel/gcc-13.2.1_p20240113-r1/work/gcc-13-20240113/libsanitizer/asan/asan_rtl.cpp:69 > Fixed. Unfortunately there is a yet another test that will never worked in configuration GCC + ASAN. Currently it is disabled when LUAJIT_USE_ASAN is enabled. I can try to fix it, but as it was never worked before we don't need to do it in scope of ctest support. diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt index 0c85f15c..4c9cf901 100644 --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -29,27 +29,39 @@ if(LUAJIT_USE_ASAN)    # https://github.com/google/sanitizers/issues/934.    macro(LibRealPath output lib)      execute_process( -      COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib} +      COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}        OUTPUT_VARIABLE LIB_LINK        OUTPUT_STRIP_TRAILING_WHITESPACE +      TIMEOUT 5 +      RESULT_VARIABLE RES      ) +    if (${LIB_LINK} STREQUAL ${lib}) +      message(FATAL_ERROR "library ${lib} is not found") +    endif() +    if (NOT RES EQUAL 0) +      message(FATAL_ERROR "Executing ${CMAKE_C_COMPILER} has failed") +    endif() +      # Fortunately, we are not interested in macOS here, so we can      # use realpath. +    # Beware, builtin command returns exit code equal to 0, +    # we cannot check is it failed or not.      execute_process(        COMMAND realpath ${LIB_LINK}        OUTPUT_VARIABLE ${output}        OUTPUT_STRIP_TRAILING_WHITESPACE +      TIMEOUT 5      )    endmacro()    LibRealPath(LIB_STDCPP libstdc++.so) -  # XXX: GCC requires both. Clang requires only libstdc++.    if(CMAKE_C_COMPILER_ID STREQUAL "GNU")      LibRealPath(LIB_ASAN libasan.so) -    # XXX: Don't use " " (separator in LD_PRELOAD) in `list()`. -    # ";" will expand to " " as we want. -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_ASAN};${LIB_STDCPP}") +    # XXX: The items of the list in LD_PRELOAD can be separated +    # by spaces or colons, and there is no support for escaping +    # either separator, see ld.so(8). +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN}:${LIB_STDCPP})    else() -    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}") +    list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP})    endif()  endif() >> + # LUAJIT_TESTS_ENV by double quotes. >> + list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_ASAN};${LIB_STDCPP}) >> else() >> - list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD="${LIB_STDCPP}") >> + list(APPEND LUAJIT_TESTS_ENV LD_PRELOAD=${LIB_STDCPP}) >> endif() >> endif() >> >> @@ -64,12 +61,25 @@ if(LUAJIT_NO_UNWIND) > > >> +set(TEST_SUITE_NAME "LuaJIT-tests") >> + >> +# XXX: The call produces both test and target >> +# as a side effect. > Nit: It looks like the quotation may be placed at > the previous line, fitting 66 symbols of the comment width. Ok, added to the previous line >> +add_test_suite_target(LuaJIT-tests >> + LABEL ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} LuaJIT-tests-deps-libs > Why don't use `${LUAJIT_TEST_BINARY}` here? AFAIR we decided with Igor in a verbal conversation to use ${LUAJIT_TEST_DEPS} instead of ${LUAJIT_TEST_BINARY}. LUAJIT_TEST_BINARY is a single target that can be used as a command, see for example tests in test/tarantool-tests/. LUAJIT_TEST_DEPS is a list of targets that should be build before running tests. > >> +) >> + >> +# The test suite has its own test runner >> +# (test/LuaJIT-tests/test.lua), it is not possible to run these >> +# tests one-by-one by CTest. >> +add_test(NAME "test/LuaJIT-tests" > Why don't use just LuaJIT-tests as a test name here? For consistency with other test titles. > The same question to the tests below. > >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/test.lua >> + +slow +ffi +bit +jit ${LUAJIT_TEST_TAGS_EXTRA} >> WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} >> ) >> +set_tests_properties("test/LuaJIT-tests" PROPERTIES >> + LABELS ${TEST_SUITE_NAME} >> + ENVIRONMENT "${LUAJIT_TESTS_ENV}" >> + DEPENDS LuaJIT-tests-deps >> +) >> diff --git a/test/LuaJIT-tests/src/CMakeLists.txt b/test/LuaJIT-tests/src/CMakeLists.txt >> index 2f90da86..f0e639e1 100644 >> --- a/test/LuaJIT-tests/src/CMakeLists.txt >> +++ b/test/LuaJIT-tests/src/CMakeLists.txt >> @@ -6,4 +6,4 @@ AddTestLib(libctest libctest.c) >> enable_language(CXX) >> AddTestLib(libcpptest libcpptest.cpp) >> >> -add_custom_target(LuaJIT-tests-libs DEPENDS ${TESTLIBS}) >> +add_custom_target(LuaJIT-tests-deps-libs DEPENDS ${TESTLIBS}) > Why do you prefer to rename this target? I see no conflict between this > name and the previous version. Seems this a change made by Igor. I'm totally ok with a previous name. reverted > >> diff --git a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> index 98277f9a..af89bfe5 100644 >> --- a/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> +++ b/test/PUC-Rio-Lua-5.1-tests/CMakeLists.txt >> @@ -34,17 +34,26 @@ add_subdirectory(libs) >> # But, unfortunately, depends on specific PUC-Rio >> # Lua 5.1 internal headers and should be adapted for LuaJIT. >> >> -add_custom_target(PUC-Rio-Lua-5.1-tests >> - DEPENDS ${LUAJIT_TEST_BINARY} PUC-Rio-Lua-5.1-tests-prepare >> +set(TEST_SUITE_NAME "PUC-Rio-Lua-5.1-tests") >> + >> +# XXX: The call produces both test and target >> +# as a side effect. >> +add_test_suite_target(PUC-Rio-Lua-5.1-tests >> + LABEL ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} PUC-Rio-Lua-5.1-tests-prepare > Why don't use `${LUAJIT_TEST_BINARY}` here? > Here and below. the same as above > >> ) >> >> -add_custom_command(TARGET PUC-Rio-Lua-5.1-tests >> - COMMENT "Running PUC-Rio Lua 5.1 tests" >> - COMMAND >> - env >> - LUA_PATH="${LUA_PATH}" >> - ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua >> +# The test suite has its own test runner >> +# (test/PUC-Rio-Lua-5.1-tests/all.lua), it is not possible >> +# to run these tests one-by-one by CTest. >> +add_test(NAME "test/PUC-Rio-Lua-5.1" >> + COMMAND ${LUAJIT_TEST_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/all.lua >> WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> ) >> +set_tests_properties("test/PUC-Rio-Lua-5.1" PROPERTIES >> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;\;" > Typo: These semicolons look excessive. Removed. > >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS PUC-Rio-Lua-5.1-tests-deps >> +) >> >> # vim: expandtab tabstop=2 shiftwidth=2 >> diff --git a/test/lua-Harness-tests/CMakeLists.txt b/test/lua-Harness-tests/CMakeLists.txt >> index f748a8fd..d454ba41 100644 >> --- a/test/lua-Harness-tests/CMakeLists.txt >> +++ b/test/lua-Harness-tests/CMakeLists.txt >> @@ -4,12 +4,6 @@ > > >> +# XXX: The call produces both test and target >> +# as a side effect. >> +add_test_suite_target(lua-Harness-tests >> + LABEL ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} >> ) >> >> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.t") >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME) >> + # FIXME: By default GLOB lists directories. > Typo: s/By default/By default,/ Fixed. > >> + # Directories are omitted in the result if LIST_DIRECTORIES >> + # is set to false. New in version CMake 3.3. >> + if (${test_name} STREQUAL ${TEST_SUITE_NAME}) >> + continue() >> + endif (${test_name} STREQUAL ${TEST_SUITE_NAME}) > Typo: s/endif (/endif(/ Already fixed. > >> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}") > I suppose, that general part "test/" may be ommited from the title. I would left as is. The main idea is to map test title to a file/dir on the filesystem. > >> + add_test(NAME ${test_title} >> + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} >> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} > Minor: Looks like misindent. It is ok in a current version. > >> + ) >> + set_tests_properties(${test_title} PROPERTIES >> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" > Minor: Semicolon looks excess here. It is ok in a current version. > >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS lua-Harness-tests-deps >> + ) >> +endforeach() >> + >> # vim: expandtab tabstop=2 shiftwidth=2 >> diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt >> index 4b1d8832..2de4a0c8 100644 >> --- a/test/tarantool-c-tests/CMakeLists.txt >> +++ b/test/tarantool-c-tests/CMakeLists.txt >> @@ -1,15 +1,4 @@ > > >> # Build libtest. >> >> @@ -49,20 +38,35 @@ foreach(test_source ${tests}) >> LIST(APPEND TESTS_COMPILED ${exe}) >> endforeach() >> >> -add_custom_target(tarantool-c-tests >> - DEPENDS libluajit libtest ${TESTS_COMPILED} >> -) >> +set(TEST_SUITE_NAME "tarantool-c-tests") >> >> -add_custom_command(TARGET tarantool-c-tests >> - COMMENT "Running Tarantool C tests" >> - COMMAND >> - ${PROVE} >> - ${CMAKE_CURRENT_BINARY_DIR} >> - --ext ${C_TEST_SUFFIX} >> - --jobs ${CMAKE_BUILD_PARALLEL_LEVEL} >> - # Report any TAP parse errors, if any, since test module is >> - # maintained by us. >> - --parse >> - ${C_TEST_FLAGS} >> - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> +# XXX: The call produces both test and target >> +# as a side effect. > Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/ It is ok in a current version. >> +add_test_suite_target(tarantool-c-tests >> + LABEL ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED} >> ) >> + >> +# XXX: Note that we cannot glob generated files in CMake, see >> +# https://stackoverflow.com/questions/44076307/cmake-globbing-generated-files > Nit: Missed dot at the end of the sentence. Added a dot to the end of sentence. > >> +# To workaround this, we glob source files and generated tests >> +# with paths to executable binaries. >> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c") > Please use `${CTEST_SRC_SUFFIX}` instead of "*.test.c". > > You may use the cycle above, which adds build targets. This is a good idea, but anyway we need two loops. First should build a list TESTS_COMPILED that is used in add_test_suite_target. The second one should generate  CMake tests. If you are still want to have a single loop for everything then we need to build -deps target and CMake test for each tarantool C test. Left without changes. > >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME_WE) >> + # FIXME: By default GLOB lists directories. >> + # Directories are omitted in the result if LIST_DIRECTORIES >> + # is set to false. New in version CMake 3.3. >> + if (${test_name} STREQUAL ${TEST_SUITE_NAME}) >> + continue() >> + endif (${test_name} STREQUAL ${TEST_SUITE_NAME}) > Typo: s/endif (/endif(/ this is ok in a current version > >> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") > I suppose, that general part "test/" may be ommited from the title. No, explained above. > >> + add_test(NAME ${test_title} >> + COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX} >> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> + ) >> + set_tests_properties(${test_title} PROPERTIES >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS tarantool-c-tests-deps >> + ) >> +endforeach() >> diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt >> index e6d12984..bb41e53b 100644 >> --- a/test/tarantool-tests/CMakeLists.txt >> +++ b/test/tarantool-tests/CMakeLists.txt >> @@ -4,19 +4,13 @@ > > >> -add_custom_target(tarantool-tests >> +add_custom_target(tarantool-tests-deps-libs > Why do you prefer to rename this target? I see no conflict between this > name and the previous version. As I said before I'm totally ok with old name. The change was introduced when "Igor automatically squashed all the tweaks made to the original commit." Reverted > >> DEPENDS ${LUAJIT_TEST_BINARY} >> ) >> >> macro(BuildTestCLib lib sources) >> AddTestLib(${lib} ${sources}) >> - add_dependencies(tarantool-tests ${lib}) >> + add_dependencies(tarantool-tests-deps-libs ${lib}) >> # Add the directory where the lib is built to the list with >> # entries for LUA_CPATH environment variable, so LuaJIT can find >> # and load it. See the comment about extending the list in the >> @@ -60,21 +54,14 @@ make_lua_path(LUA_PATH >> ${PROJECT_SOURCE_DIR}/tools/?.lua >> ${LUAJIT_SOURCE_DIR}/?.lua >> ${LUAJIT_BINARY_DIR}/?.lua >> + ${PROJECT_BINARY_DIR}/src/?.lua > This is the same directory as a ${LUAJIT_SOURCE_DIR}/?.lua. fixed in a current version > >> ) >> + >> # Update LUA_CPATH with the library paths collected within >> # macro. >> make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS}) >> > > >> +set(TEST_SUITE_NAME "tarantool-tests") >> + >> +# XXX: The call produces both test and target >> +# as a side effect. >> +add_test_suite_target(tarantool-tests >> + LABEL ${TEST_SUITE_NAME} >> + DEPENDS ${LUAJIT_TEST_DEPS} tarantool-tests-deps-libs >> ) >> >> +# LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list >> +# with dependencies are set in scope of BuildTestLib macro. >> +set(TEST_ENV "LUA_PATH=${LUA_PATH};\;\;;LUA_CPATH=${LUA_CPATH}\;\;;${LUA_TEST_ENV_MORE}") >> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") > Why do we need GLOB_RECURSE here, instead of GLOB? it is better to search test files recursively, no one can guarantee that files always will be in a top testsuite dir. > >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME) >> + set(test_title "test/tarantool-tests/${test_name}") > I suppose, that general part "test/" may be ommited from the title. answered above > >> + add_test(NAME ${test_title} >> + COMMAND ${LUAJIT_TEST_COMMAND} ${test_path} >> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} >> + ) >> + set_tests_properties(${test_title} PROPERTIES >> + ENVIRONMENT "${TEST_ENV}" >> + LABELS ${TEST_SUITE_NAME} >> + DEPENDS tarantool-tests-deps >> + ) >> +endforeach() >> -- >> 2.34.1 >>