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 <sergeyb@tarantool.org> > > 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 <snipped> > 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. Same for the paragraphs below. > $ 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? > > 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,/ > future we could change these test runners to specify tests from outside, Typo: s/future/future,/ > and after this we could run tests separately by CTest in these test Typo: s/this/this,/ > 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/ > 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 <snipped> > 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. > +# 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 <test/CMakeLists.txt>)? > 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. > +# 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? Unfortunately, this flag does nothing for `make test` command. Is there the way to fix it? BTW `make LuaJIT-tests` or `make tarantool-tests` work fine. > + --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. > +) > +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? Or, maybe this line should be ommitted since it is also used for normal test targets with several tests. 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. | 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 > + ${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. > + > + 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. > +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. > + 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. > - if(POLICY CMP0037) <snipped> > 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. > > 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 > + # 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) <snipped> > +set(TEST_SUITE_NAME "LuaJIT-tests") > + > +# XXX: The call produces both test and target > +# <LuaJIT-tests-deps> as a side effect. Nit: It looks like the <LuaJIT-tests-deps> quotation may be placed at the previous line, fitting 66 symbols of the comment width. > +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? > +) > + > +# 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? 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. > 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, <ltests.c> 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 > +# <PUC-Rio-Lua-5.1-tests-deps> 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. > ) > > -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. > + 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 @@ <snipped> > +# XXX: The call produces both test and target > +# <lua-Harness-tests-deps> 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,/ > + # 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(/ > + set(test_title "test/${TEST_SUITE_NAME}/${test_name}") I suppose, that general part "test/" may be ommited from the title. > + add_test(NAME ${test_title} > + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} > + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} Minor: Looks like misindent. > + ) > + set_tests_properties(${test_title} PROPERTIES > + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" Minor: Semicolon looks excess here. > + 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 @@ <snipped> > # 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 > +# <LuaJIT-tests-deps> as a side effect. Typo: s/LuaJIT-tests-deps/tarantool-c-tests-deps/ > +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. > +# 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. > +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(/ > + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") I suppose, that general part "test/" may be ommited from the title. > + 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 @@ <snipped> > -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. > 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. > ) > + > # Update LUA_CPATH with the library paths collected within > # <BuildTestLib> macro. > make_lua_path(LUA_CPATH PATHS ${LUA_CPATHS}) > <snipped> > +set(TEST_SUITE_NAME "tarantool-tests") > + > +# XXX: The call produces both test and target > +# <tarantool-tests-deps> 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? > +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. > + 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 > -- Best regards, Sergey Kaplun
Igor, thanks for the next review iteration. See my comments below. On 3/16/24 20:12, Igor Munkin wrote: > Sergey, > > Thanks for the fixes! The patch L(almost)GTM now, but I still make some > comments. You can see my changes here below. > > On 12.03.24, Sergey Bronnikov wrote: >> From: Sergey Bronnikov <sergeyb@tarantool.org> >> >> 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 <snipped> >> All Labels: >> LuaJIT >> PUC-Rio-Lua-5.1 >> lua-Harness >> tarantool >> tarantool-c > Typo: Labels are updated since v2. Updated to: $ ctest --print-labels Test project <snipped> All Labels: LuaJIT-tests PUC-Rio-Lua-5.1-tests lua-Harness-tests tarantool-c-tests tarantool-tests > >> $ 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. >> $ 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`. > Side note: I guees something like <test-all> can save the behaviour > broken by this change. However, do we really need this? I left the final > decision to skaplun@ here. > >> 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 >> future we could change these test runners to specify tests from outside, >> and after this we could run tests separately by CTest in these test >> 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 >> for each testsuite that executes a target that builds test dependencies. > Minor: Maybe it's worth to mention <add_test_suite_target> macro here? Rephrased to: 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 introduced a macro `add_test_suite_target` that adds a CMake-test 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/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." >> +) > This is worth to be mentioned in the commit message (at least). Added: The commit introduce a CMake option LUAJIT_TEST_DEPS that set dependencies to be used for running and building LuaJIT 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 > <snipped> > >> 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 > Typo: We still follows 66 symbol limit for comments in tarantool/luajit, > but I also left the final decision to skaplun@ here. Fixed: --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -78,8 +78,8 @@ 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 -# and is used by `test` target. +# XXX: CMake 3.17 introduces CMAKE_CTEST_ARGUMENTS that contains +# CTest options and is used by `test` target. set(TEST_FLAGS --output-on-failure --schedule-random > >> +# and is used by `test` target. > <snipped> > >> -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} >> + DEPENDS tarantool-c-tests-deps >> + tarantool-tests-deps >> + lua-Harness-tests-deps >> + PUC-Rio-Lua-5.1-tests-deps >> + LuaJIT-tests-deps > I suggest to make LuaJIT-test an umbrella rule without the particular > implementation. See the changes below: > > ================================================================================ > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 90aaead2..ad447e72 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -130,10 +130,9 @@ add_subdirectory(tarantool-c-tests) > add_subdirectory(tarantool-tests) > > add_custom_target(${PROJECT_NAME}-test > - COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS} > - DEPENDS tarantool-c-tests-deps > - tarantool-tests-deps > - lua-Harness-tests-deps > - PUC-Rio-Lua-5.1-tests-deps > - LuaJIT-tests-deps > + DEPENDS tarantool-c-tests > + tarantool-tests > + lua-Harness-tests > + PUC-Rio-Lua-5.1-tests > + LuaJIT-tests > ) > > ================================================================================ Here we go again. Already discussed it verbally some time ago. With proposed patch ctest splits output for each testsuite: $ ninja LuaJIT-test [9/15] cd /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/t...ctest -L tarantool-c-tests --output-on-failure --schedule-random --parallel 8 Test project /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/tarantool-c-tests Start 7: test/tarantool-c-tests/lj-962-premature-stack-overflow.c_test <snipped> 10/10 Test #9: test/tarantool-c-tests/misclib-getmetrics-capi.c_test ................... Passed 0.06 sec 100% tests passed, 0 tests failed out of 10 Label Time Summary: tarantool-c-tests = 0.67 sec*proc (10 tests) Total Test time (real) = 0.13 sec [12/15] cd /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/...ctest -L lua-Harness-tests --output-on-failure --schedule-random --parallel 8 Test project /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/lua-Harness-tests <snipped> 0/50 Test #33: test/lua-Harness-tests/241-standalone.t .... Passed 1.18 sec [0/6793] 100% tests passed, 0 tests failed out of 50 Label Time Summary: lua-Harness-tests = 5.22 sec*proc (50 tests) Total Test time (real) = 1.53 sec [13/15] cd /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/.../bin/ctest -L LuaJIT-tests --output-on-failure --schedule-random --parallel 8 Test project /home/sergeyb/sources/MRG/tarantool/third_party/luajit/build/gc64/test/LuaJIT-tests Start 2: test/LuaJIT-tests 1/1 Test #2: test/LuaJIT-tests ................ Passed 3.65 sec <snipped> This output looks ugly and is not convenient in daily use. Left it without changes. > >> ) > <snipped> > >> 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}\;\;" > The trailing semicolons are automatically added by <make_lua_path>. My > bad, remove it please. --- a/test/LuaJIT-tests/CMakeLists.txt +++ b/test/LuaJIT-tests/CMakeLists.txt @@ -9,7 +9,7 @@ make_lua_path(LUA_CPATH ) set(LUAJIT_TESTS_ENV - "LUA_CPATH=${LUA_CPATH}\;\;" + "LUA_CPATH=${LUA_CPATH}" ) set(LD_LIBRARY_PATH "${CMAKE_CURRENT_BINARY_DIR}/src/:") > >> ) >> >> 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 >> + # 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() > I really do not understand, why this quotation is required in the > original series and breaks everything now. If skaplun@ is bothered more > than me, he can spend more time with CMake to understand this. Sad, but > OK, so I'm out here. > >> > <snipped> > >> 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 > <snipped> > >> +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. >> + # 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: Excess whitespace and condition in <endif>. --- a/test/lua-Harness-tests/CMakeLists.txt +++ b/test/lua-Harness-tests/CMakeLists.txt @@ -32,7 +32,7 @@ foreach(test_path ${tests}) # 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}) + endif() set(test_title "test/${TEST_SUITE_NAME}/${test_name}") add_test(NAME ${test_title} COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} > >> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}") >> + add_test(NAME ${test_title} >> + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} >> + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} > Typo: Looks like misindent, doesn't it? Fixed: set(test_title "test/${TEST_SUITE_NAME}/${test_name}") add_test(NAME ${test_title} - COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} - WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) set_tests_properties(${test_title} PROPERTIES ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" > >> + ) >> + set_tests_properties(${test_title} PROPERTIES >> + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" > Typo: The semicolon looks excess here. --- a/test/lua-Harness-tests/CMakeLists.txt +++ b/test/lua-Harness-tests/CMakeLists.txt @@ -39,7 +39,7 @@ foreach(test_path ${tests}) WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} ) set_tests_properties(${test_title} PROPERTIES - ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" + ENVIRONMENT "LUA_PATH=${LUA_PATH}" LABELS ${TEST_SUITE_NAME} DEPENDS lua-Harness-tests-deps ) > >> + 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 > <snipped> > >> @@ -49,20 +38,35 @@ foreach(test_source ${tests}) > <snipped> > >> +# XXX: The call produces both test and target >> +# <LuaJIT-tests-deps> as a side effect. > Typo: Miscopypasting :) Fixed, thanks! --- a/test/tarantool-c-tests/CMakeLists.txt +++ b/test/tarantool-c-tests/CMakeLists.txt @@ -41,7 +41,7 @@ endforeach() set(TEST_SUITE_NAME "tarantool-c-tests") # XXX: The call produces both test and target -# <LuaJIT-tests-deps> as a side effect. +# <tarantool-c-tests-deps> as a side effect. add_test_suite_target(tarantool-c-tests LABEL ${TEST_SUITE_NAME} DEPENDS ${LUAJIT_TEST_DEPS} libluajit libtest ${TESTS_COMPILED} > >> +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 >> +# To workaround this, we glob source files and generated tests >> +# with paths to executable binaries. >> +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c") >> +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: Excess whitespace and condition in <endif>. --- a/test/tarantool-c-tests/CMakeLists.txt +++ b/test/tarantool-c-tests/CMakeLists.txt @@ -59,7 +59,7 @@ foreach(test_path ${tests}) # 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}) + endif() set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") add_test(NAME ${test_title} COMMAND ${CMAKE_CURRENT_BINARY_DIR}/${test_name}${C_TEST_SUFFIX} > >> + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") > <snipped> > >> 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 > <snipped> > >> @@ -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_BINARY_DIR}/?.lua. Fixed: --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -54,7 +54,6 @@ make_lua_path(LUA_PATH ${PROJECT_SOURCE_DIR}/tools/?.lua ${LUAJIT_SOURCE_DIR}/?.lua ${LUAJIT_BINARY_DIR}/?.lua - ${PROJECT_BINARY_DIR}/src/?.lua ) # Update LUA_CPATH with the library paths collected within >> ) > <snipped> > >> @@ -110,22 +97,29 @@ else() > <snipped> > >> +# 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}") > The trailing semicolons are automatically added by <make_lua_path>. > Please, remove it. Fixed: --- a/test/tarantool-tests/CMakeLists.txt +++ b/test/tarantool-tests/CMakeLists.txt @@ -107,7 +107,7 @@ add_test_suite_target(tarantool-tests # 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}") +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}") foreach(test_path ${tests}) get_filename_component(test_name ${test_path} NAME) > >> +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") > Minor: Why is <GLOB_RECURSE> used here, and the hack with manual > directory name filtering is omitted? it is better to search test files recursively, no one can guarantee that files always will be in a top testsuite dir. However in lua-Harness suite we cannot use GLOB_RECURSE because suite has a number of files with equal filenames: $ find test/lua-Harness-tests/ -name lexico.t test/lua-Harness-tests/lexico53/lexico.t test/lua-Harness-tests/lexicojit/lexico.t test/lua-Harness-tests/lexico52/lexico.t test/lua-Harness-tests/lexico54/lexico.t due to this CMake configuration has failed with "add_test given test NAME "test/lua-Harness-tests/lexico.t" which already exists in this directory." > >> +foreach(test_path ${tests}) >> + get_filename_component(test_name ${test_path} NAME) >> + set(test_title "test/tarantool-tests/${test_name}") >> + 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 >>
Hi, Sergey! Thanks for the patch! Please consider my comments below. On Thu, Mar 14, 2024 at 02:39:51PM +0300, Sergey Bronnikov wrote: > From: sergeyb@tarantool.org > > Thanks to Carlo Cabrera. > > (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a) > > Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect Typo: s/FAR/FAT/ Typo: s/arm64/ARM64/ > format because FFI structure used for generation was wrong: Typo: s/FFI/The FFI/ > `mach_fat_obj` instead of `mach_fat_obj_64`. > > Sergey Bronnikov: > * added the description and a test for the problem Typo: s/a test/the test/ > > Part of tarantool/tarantool#9595 > --- > src/jit/bcsave.lua | 14 +++++++++++++- > ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua > index 7aec1555..069cf1a3 100644 > --- a/src/jit/bcsave.lua > +++ b/src/jit/bcsave.lua > @@ -491,6 +491,18 @@ typedef struct { > mach_nlist sym_entry; > uint8_t space[4096]; > } mach_fat_obj; > +typedef struct { > + mach_fat_header fat; > + mach_fat_arch fat_arch[2]; > + struct { > + mach_header_64 hdr; > + mach_segment_command_64 seg; > + mach_section_64 sec; > + mach_symtab_command sym; > + } arch[2]; > + mach_nlist_64 sym_entry; > + uint8_t space[4096]; > +} mach_fat_obj_64; > ]] > local symname = '_'..LJBC_PREFIX..ctx.modname > local isfat, is64, align, mobj = false, false, 4, "mach_obj" > @@ -499,7 +511,7 @@ typedef struct { > elseif ctx.arch == "arm" then > isfat, mobj = true, "mach_fat_obj" > elseif ctx.arch == "arm64" then > - is64, align, isfat, mobj = true, 8, true, "mach_fat_obj" > + is64, align, isfat, mobj = true, 8, true, "mach_fat_obj_64" > else > check(ctx.arch == "x86", "unsupported architecture for OSX") > end > diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > index 0519e134..0a11f163 100644 > --- a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({ > ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL, > }) > > -test:plan(4) > +test:plan(8) > > -- Test creates an object file in Mach-O format with LuaJIT bytecode > -- and checks validness of the object file fields. > @@ -267,5 +267,7 @@ end > > -- ARM > build_and_check_mach_o(false) > +-- ARM64 > +build_and_check_mach_o(true) These `true/false` should be explained as platform toggle. An even better solution would be to pass the platform name explicitly. > > test:done(true) > -- > 2.34.1 >
Sorry folks, I forgot to mention one more thing, that concerns me: we have no way to make sure that we test this case on an AVX512f platform. We don't have out own runners with AVX512f support, and there is no way to specify them in GitHub Actions. I believe that we should mention that in the commit message, or in a comment in the workflow file.
Hi, Sergey, thanks for the patch! Please consider my comments below. On Thu, Mar 14, 2024 at 02:39:49PM +0300, Sergey Bronnikov wrote: > From: sergeyb@tarantool.org > > Thanks to Carlo Cabrera. > > (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f) > > Mach-O FAT object constructed by LuaJIT had an incorrect format. The Typo: s/Mach-O FAT/The Mach-O FAT/ > problem is reproduced when target hardware platform has AVX512F and Typo:s/target/the target/ > LuaJIT is compiled with enabled AVX512F instructions. > > The problem is arise because LuaJIT FFI code for Mach-O file generation Typo: s/is arise/arises/ > in `bcsave.lua` relied on undefined behavior for conversions to Typo: s/relied/relies/ > `uint32_t`. AVX512F has the `vcvttsd2usi` instruction which converts Typo: s/instruction/instruction,/ > `double`/`float` to `uint32_t/uint64_t`. Earlier architectures (SSE2, > AVX2) are sorely lacking such an instruction, as they only support > signed conversions. Unsigned conversions are done with a signed convert > and range shifting - the exact algorithm depends on the compiler. > A side-effect of these workarounds is that negative `double`/`float` > often inadvertently convert 'as expected', even though this is invoking > undefined behavior. Whereas `vcvttsd2usi` always returns 0x80000000 or > 0x8000000000000000 for out-of-range inputs. > > The patch fixes the problem, however, the real issue remains unfixed. > > Sergey Bronnikov: > * added the description and a test for the problem Typo: s/a test/the test/ > > Part of tarantool/tarantool#9595 > --- > .github/workflows/exotic-builds-testing.yml | 5 +- > src/jit/bcsave.lua | 6 +- > .../lj-366-strtab-correct-size.test.lua | 10 +- > ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++ > test/tarantool-tests/utils/tools.lua | 8 + > 5 files changed, 287 insertions(+), 13 deletions(-) > create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > > diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml > index a9ba5fd5..df4bc2e9 100644 > --- a/.github/workflows/exotic-builds-testing.yml > +++ b/.github/workflows/exotic-builds-testing.yml > @@ -32,6 +32,7 @@ jobs: > fail-fast: false > matrix: > BUILDTYPE: [Debug, Release] > + OS: [Linux, macOS] > ARCH: [ARM64, x86_64] > GC64: [ON, OFF] > FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind] > @@ -50,13 +51,15 @@ jobs: > FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON https://github.com/tarantool/luajit/actions/runs/8279362128 > - FLAVOR: nounwind > FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON > + - FLAVOR: avx512 > + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc > exclude: > - ARCH: ARM64 > GC64: OFF > # DUALNUM is default for ARM64, no need for additional testing. > - FLAVOR: dualnum > ARCH: ARM64 > - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}'] > + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }'] The matrix.OS variable should be wrapped with double curly braces, instead of singular ones. So, it should be like this: | '${{ matrix.OS }}' Currently, exotic build testing fails to start because of this mistake. https://github.com/tarantool/luajit/actions/runs/8279362128 > name: > > LuaJIT ${{ matrix.FLAVOR }} > (Linux/${{ matrix.ARCH }}) > diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua > index a287d675..7aec1555 100644 > --- a/src/jit/bcsave.lua > +++ b/src/jit/bcsave.lua > @@ -446,18 +446,18 @@ typedef struct { > uint32_t value; > } mach_nlist; > typedef struct { > - uint32_t strx; > + int32_t strx; > uint8_t type, sect; > uint16_t desc; > uint64_t value; > } mach_nlist_64; > typedef struct > { > - uint32_t magic, nfat_arch; > + int32_t magic, nfat_arch; > } mach_fat_header; > typedef struct > { > - uint32_t cputype, cpusubtype, offset, size, align; > + int32_t cputype, cpusubtype, offset, size, align; > } mach_fat_arch; > typedef struct { > struct { > diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua > index 8a97a441..0bb92da6 100644 > --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua > +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua Let's move this update to a separate patch alongside with the added utility function. Currently, this change in unrelated test file is a bit confusing. > @@ -138,14 +138,6 @@ local function create_obj_file(name) > return elf_filename > end > > --- Reads a file located in a specified path and returns its content. > -local function read_file(path) > - local file = assert(io.open(path), 'cannot open an object file') > - local content = file:read('*a') > - file:close() > - return content > -end > - > -- Parses a buffer in an ELF format and returns an offset and a size of strtab > -- and symtab sections. > local function read_elf(elf_content) > @@ -172,7 +164,7 @@ end > test:plan(3) > > local elf_filename = create_obj_file(MODULE_NAME) > -local elf_content = read_file(elf_filename) > +local elf_content = require('utils').tools.read_file(elf_filename) > assert(#elf_content ~= 0, 'cannot read an object file') > > local strtab, symtab = read_elf(elf_content) > diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > new file mode 100644 > index 00000000..0519e134 > --- /dev/null > +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua > @@ -0,0 +1,271 @@ > +local tap = require('tap') > +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({ > + -- XXX: Tarantool doesn't use default LuaJIT loaders, and Lua > + -- bytecode can't be loaded from the shared library. For more Typo: s/the shared/a shared/ > + -- info: https://github.com/tarantool/tarantool/issues/9671. > + -- luacheck: no global > + ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL, Typo: s/uses exotic/uses an exotic/ > +}) > + > +test:plan(4) > + > +-- Test creates an object file in Mach-O format with LuaJIT bytecode Typo: s/Test/The test/ > +-- and checks validness of the object file fields. Typo: s/validness/the validity/ > +-- > +-- The original problem is reproduced with LuaJIT that built with > +-- enabled AVX512F instructions. The support of AVX512F could be Typo: s/support of/support for/ > +-- checked in `/proc/cpuinfo` on Linux and > +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be > +-- implicitly enabled in a C compiler by passing CPU codename. > +-- Please consult for available model architecture on GCC Online Typo: s/for/the/ > +-- Documentation [1] for available CPU codenames. To detect > +-- CPU codename execute `gcc -march=native -Q --help=target | grep march`. > +-- > +-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html > +-- > +-- Manual steps for reproducing are the following: > +-- > +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original > +-- $ echo > test.lua > +-- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o > +-- $ file test.o > +-- empty.o: DOS executable (block device driver) > + > +local ffi = require('ffi') > + > +-- Format of the Mach-O is described in a document Typo: s/a document/the document/ > +-- "OS X ABI Mach-O File Format Reference", published by Apple company. > +-- Copy of the (now removed) official documentation in [1]. Let's replace "in [1]" with "can be found here [1].". > +-- Yet another source of thruth is a XNU headers, see the definition Typo: s/is a XNU/are XNU/ Typo: s/the definition/definitions/ > +-- of C-structures in: [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`). > + > +-- 1. https://github.com/aidansteele/osx-abi-macho-file-format-reference > +-- 2. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/nlist.h > +-- 3. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/fat.h > +-- 4. https://developer.apple.com/documentation/apple-silicon/addressing-architectural-differences-in-your-macos-code > +-- > +-- Using the same declarations as defined in <src/jit/bcsave.lua>. > +ffi.cdef[[ > +typedef struct > +{ > + uint32_t magic, cputype, cpusubtype, filetype, ncmds, sizeofcmds, flags; > +} mach_header; > + > +typedef struct > +{ > + mach_header; uint32_t reserved; > +} mach_header_64; > + > +typedef struct { > + uint32_t cmd, cmdsize; > + char segname[16]; > + uint32_t vmaddr, vmsize, fileoff, filesize; > + uint32_t maxprot, initprot, nsects, flags; > +} mach_segment_command; > + > +typedef struct { > + uint32_t cmd, cmdsize; > + char segname[16]; > + uint64_t vmaddr, vmsize, fileoff, filesize; > + uint32_t maxprot, initprot, nsects, flags; > +} mach_segment_command_64; > + > +typedef struct { > + char sectname[16], segname[16]; > + uint32_t addr, size; > + uint32_t offset, align, reloff, nreloc, flags; > + uint32_t reserved1, reserved2; > +} mach_section; > + > +typedef struct { > + char sectname[16], segname[16]; > + uint64_t addr, size; > + uint32_t offset, align, reloff, nreloc, flags; > + uint32_t reserved1, reserved2, reserved3; > +} mach_section_64; > + > +typedef struct { > + uint32_t cmd, cmdsize, symoff, nsyms, stroff, strsize; > +} mach_symtab_command; > + > +typedef struct { > + int32_t strx; > + uint8_t type, sect; > + int16_t desc; > + uint32_t value; > +} mach_nlist; > + > +typedef struct { > + uint32_t strx; > + uint8_t type, sect; > + uint16_t desc; > + uint64_t value; > +} mach_nlist_64; > + > +typedef struct > +{ > + uint32_t magic, nfat_arch; > +} mach_fat_header; > + > +typedef struct > +{ > + uint32_t cputype, cpusubtype, offset, size, align; > +} mach_fat_arch; > + > +typedef struct { > + mach_fat_header fat; > + mach_fat_arch fat_arch[2]; > + struct { > + mach_header hdr; > + mach_segment_command seg; > + mach_section sec; > + mach_symtab_command sym; > + } arch[2]; > + mach_nlist sym_entry; > + uint8_t space[4096]; > +} mach_fat_obj; > + > +typedef struct { > + mach_fat_header fat; > + mach_fat_arch fat_arch[2]; > + struct { > + mach_header_64 hdr; > + mach_segment_command_64 seg; > + mach_section_64 sec; > + mach_symtab_command sym; > + } arch[2]; > + mach_nlist_64 sym_entry; > + uint8_t space[4096]; > +} mach_fat_obj_64; > +]] > + > +local function create_obj_file(name, arch) > + local mach_o_path = os.tmpname() .. '.o' > + local lua_path = os.getenv('LUA_PATH') > + local lua_bin = require('utils').exec.luacmd(arg):match('%S+') > + local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -o osx -a %s -e "print()" %s' > + local cmd = (cmd_fmt):format(lua_path, lua_bin, name, arch, mach_o_path) > + local ret = os.execute(cmd) > + assert(ret == 0, 'cannot create an object file') > + return mach_o_path > +end > + > +-- Parses a buffer in an Mach-O format and returns Typo: s/in an/in the/ > +-- an fat magic number and nfat_arch. Typo: s/an fat/the FAT/ Typo: s/nfat_arch/`nfat_arch` > +local function read_mach_o(buf, is64) > + local res = { > + header = { > + magic = 0, > + nfat_arch = 0, > + }, > + fat_arch = { > + }, I guess, that the formatting below is a bit better here: | fat_arch = {}, > + } > + > + -- Mach-O FAT object. > + local mach_fat_obj_type = ffi.typeof(is64 and 'mach_fat_obj_64 *' or 'mach_fat_obj *') The line is longer than 80 symbols. > + local obj = ffi.cast(mach_fat_obj_type, buf) > + > + -- Mach-O FAT object header. > + local mach_fat_header_type = ffi.typeof('mach_fat_header *') > + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat) > + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE. > + res.header.magic = be32(mach_fat_header.magic) > + res.header.nfat_arch = be32(mach_fat_header.nfat_arch) > + > + -- Mach-O FAT object archs. Typo: s/archs/arches/ Side note: I feel like the comments for the sections are not elaborate enough for unprepared reader. I think you should briefly desribe the basic structure of a FAT object (FAT header, then array of per-segment headers, then object files) > + local mach_fat_arch_type = ffi.typeof('mach_fat_arch *') > + for i = 0, res.header.nfat_arch - 1 do > + local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i]) > + arch = { > + cputype = be32(fat_arch.cputype), > + cpusubtype = be32(fat_arch.cpusubtype), > + } > + table.insert(res.fat_arch, arch) > + end > + > + return res > +end > + > +-- Defined in <src/jit/bcsave.lua:bcsave_machobj>. > +local sum_cputype = { > + x86 = 7, > + x64 = 0x01000007, > + arm = 7 + 12, > + arm64 = 0x01000007 + 0x0100000c, > +} > +local sum_cpusubtype = { > + x86 = 3, > + x64 = 3, > + arm = 3 + 9, > + arm64 = 3 + 0, > +} It would be nice to have an explanation for these magic numbers. > + > +-- The function builds Mach-O FAT object file and retrieves > +-- its header fields (magic and nfat_arch) > +-- and fields of the each arch (cputype, cpusubtype). > +-- > +-- Mach-O FAT object header could be retrieved with `otool` on macOS: Typo: s/could be/can be/ > +-- > +-- $ otool -f empty.o > +-- Fat headers > +-- fat_magic 0xcafebabe > +-- nfat_arch 2 > +-- <snipped> > +-- > +-- CPU type and subtype could be retrieved with `lipo` on macOS: Typo: s/could be/can be/ > +-- > +-- $ luajit -b -o osx -a arm empty.lua empty.o > +-- $ lipo -archs empty.o > +-- i386 armv7 > +-- $ luajit -b -o osx -a arm64 empty.lua empty.o > +-- $ lipo -archs empty.o > +-- x86_64 arm64 > +local function build_and_check_mach_o(is64) > + local arch = is64 and 'arm64' or 'arm' > + > + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in > + -- big-endian byte order format. On a big-endian host CPU, > + -- this can be validated using the constant FAT_MAGIC; > + -- on a little-endian host CPU, it can be validated using > + -- the constant FAT_CIGAM. > + -- > + -- FAT_NARCH is an integer specifying the number of fat_arch > + -- data structures that follow. This is the number of > + -- architectures contained in this binary. > + -- > + -- See aforementioned "OS X ABI Mach-O File Format Reference". > + -- > + local FAT_MAGIC = '0xffffffffcafebabe' > + local FAT_NARCH = 2 > + > + local MODULE_NAME = 'lango_team' > + > + local mach_o_obj_path = create_obj_file(MODULE_NAME, arch) > + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path) > + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file') > + > + local mach_o = read_mach_o(mach_o_buf, is64) > + > + -- Teardown. > + local retcode = os.remove(mach_o_obj_path) > + assert(retcode == true, 'remove an object file') > + > + local magic_str = string.format('0x%02x', mach_o.header.magic) > + test:is(magic_str, FAT_MAGIC, 'fat_magic is correct in Mach-O, ' .. arch) > + test:is(mach_o.header.nfat_arch, FAT_NARCH, 'nfat_arch is correct in Mach-O, ' .. arch) > + > + local total_cputype = 0 > + local total_cpusubtype = 0 > + for i = 1, mach_o.header.nfat_arch do > + total_cputype = total_cputype + mach_o.fat_arch[i].cputype > + total_cpusubtype = total_cpusubtype + mach_o.fat_arch[i].cpusubtype > + end > + test:is(total_cputype, sum_cputype[arch], 'cputype is correct in Mach-O, ' .. arch) > + test:is(total_cpusubtype, sum_cpusubtype[arch], 'cpusubtype is correct in Mach-O, ' .. arch) > +end > + > +-- ARM > +build_and_check_mach_o(false) > + > +test:done(true) Please mention that alongside with test and fix for the issue you've added this tool. IMO, it would be even better to do that in a separate commit, to avoid confusion because of the updates in test files unrelated to the patch. > diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua > index f35c6922..26b8c08d 100644 > --- a/test/tarantool-tests/utils/tools.lua > +++ b/test/tarantool-tests/utils/tools.lua > @@ -12,4 +12,12 @@ function M.profilename(name) > return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern)) > end > > +-- Reads a file located in a specified path and returns its content. > +function M.read_file(path) > + local file = assert(io.open(path), 'cannot open an object file') > + local content = file:read('*a') > + file:close() > + return content > +end > + > return M > -- > 2.34.1 >
Sergey, Thanks for the fixes! The patch L(almost)GTM now, but I still make some comments. You can see my changes here below. On 12.03.24, Sergey Bronnikov wrote: > From: Sergey Bronnikov <sergeyb@tarantool.org> > > 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 <snipped> > All Labels: > LuaJIT > PUC-Rio-Lua-5.1 > lua-Harness > tarantool > tarantool-c Typo: Labels are updated since v2. > $ 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. > $ 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`. Side note: I guees something like <test-all> can save the behaviour broken by this change. However, do we really need this? I left the final decision to skaplun@ here. > > 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 > future we could change these test runners to specify tests from outside, > and after this we could run tests separately by CTest in these test > 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 > for each testsuite that executes a target that builds test dependencies. Minor: Maybe it's worth to mention <add_test_suite_target> macro here? > > 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/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." > +) This is worth to be mentioned in the commit message (at least). > + > # 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 <snipped> > 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 Typo: We still follows 66 symbol limit for comments in tarantool/luajit, but I also left the final decision to skaplun@ here. > +# and is used by `test` target. <snipped> > -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} > + DEPENDS tarantool-c-tests-deps > + tarantool-tests-deps > + lua-Harness-tests-deps > + PUC-Rio-Lua-5.1-tests-deps > + LuaJIT-tests-deps I suggest to make LuaJIT-test an umbrella rule without the particular implementation. See the changes below: ================================================================================ diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 90aaead2..ad447e72 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -130,10 +130,9 @@ add_subdirectory(tarantool-c-tests) add_subdirectory(tarantool-tests) add_custom_target(${PROJECT_NAME}-test - COMMAND ${CMAKE_CTEST_COMMAND} ${TEST_FLAGS} - DEPENDS tarantool-c-tests-deps - tarantool-tests-deps - lua-Harness-tests-deps - PUC-Rio-Lua-5.1-tests-deps - LuaJIT-tests-deps + DEPENDS tarantool-c-tests + tarantool-tests + lua-Harness-tests + PUC-Rio-Lua-5.1-tests + LuaJIT-tests ) ================================================================================ > ) <snipped> > 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}\;\;" The trailing semicolons are automatically added by <make_lua_path>. My bad, remove it please. > ) > > 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 > + # 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() I really do not understand, why this quotation is required in the original series and breaks everything now. If skaplun@ is bothered more than me, he can spend more time with CMake to understand this. Sad, but OK, so I'm out here. > <snipped> > 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 <snipped> > +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. > + # 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: Excess whitespace and condition in <endif>. > + set(test_title "test/${TEST_SUITE_NAME}/${test_name}") > + add_test(NAME ${test_title} > + COMMAND ${LUAJIT_TEST_COMMAND} -l profile_luajit21 ${test_path} > + WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR} Typo: Looks like misindent, doesn't it? > + ) > + set_tests_properties(${test_title} PROPERTIES > + ENVIRONMENT "LUA_PATH=${LUA_PATH}\;" Typo: The semicolon looks excess here. > + 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 <snipped> > @@ -49,20 +38,35 @@ foreach(test_source ${tests}) <snipped> > +# XXX: The call produces both test and target > +# <LuaJIT-tests-deps> as a side effect. Typo: Miscopypasting :) > +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 > +# To workaround this, we glob source files and generated tests > +# with paths to executable binaries. > +file(GLOB tests ${CMAKE_CURRENT_SOURCE_DIR} "*.test.c") > +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: Excess whitespace and condition in <endif>. > + set(test_title "test/${TEST_SUITE_NAME}/${test_name}${C_TEST_SUFFIX}") <snipped> > 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 <snipped> > @@ -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_BINARY_DIR}/?.lua. > ) <snipped> > @@ -110,22 +97,29 @@ else() <snipped> > +# 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}") The trailing semicolons are automatically added by <make_lua_path>. Please, remove it. > +file(GLOB_RECURSE tests ${CMAKE_CURRENT_SOURCE_DIR} "*${LUA_TEST_SUFFIX}") Minor: Why is <GLOB_RECURSE> used here, and the hack with manual directory name filtering is omitted? > +foreach(test_path ${tests}) > + get_filename_component(test_name ${test_path} NAME) > + set(test_title "test/tarantool-tests/${test_name}") > + 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 > -- Best regards, IM
This patch makes use of reusable workflows from the Tarantool repository, so now LuaJIT CI tests integration in all relevant workflows. Also, this patch disables MacOS testing for Tarantool integration since those runs were made nightly in the Tarantool repo. As noted in the documentation [1], it is impossible for a single workflow call tree to include more than 20 workflows. To overcome this limitation, workflows are started in a bunch of separate files. This makes it impossible to depend on LuaJIT-only jobs for integration workflows such as `tarantool-ecosystem.yml` or `tarantool-exotic.yml` since it is impossible to make cross-dependencies between workflow files. The name of a callee workflow cannot be substituted using a matrix [2], so workflow calls are copied and pasted instead. Table below shows which workflows are enabled and why. Workflow name |+/-| Reason ----------------------------------------------------------------- codeql | - | Not relevant to LuaJIT. coverage | + | Long tests for profilers. coverity | - | Cron workflow. debug | + | Tarantool debug build. debug_aarch64 | + | Tarantool debug build. debug_asan_clang | + | Tarantool debug build. default_gcc_centos_7 | + | Old gcc version (7). freebsd | - | Nightly build. fuzzing | - | Impossible to bump LuaJIT. integration | + | Tarantool ecosystem. jepsen-cluster-txm | - | Manual workflow. jepsen-cluster | - | Manual workflow. jepsen-single-instance-txm | - | Cron workflow. jepsen-single-instance | - | Cron workflow. lango-stale-reviews | - | Cron workflow. lint | - | LuaJIT has its own lint. luajit-integration | + | Run tests under Tarantool. memtx_allocator_based_on_malloc | - | Not relevant to LuaJIT. osx | - | Nightly build. out_of_source | + | Out of source build. packaging | - | No LuaJIT-relevant variety. perf_cbench | - | Not enabled for PRs. perf_linkbench_ssd | - | Not enabled for PRs. perf_micro | - | Not relevant to LuaJIT. perf_nosqlbench_hash | - | Not enabled for PRs. perf_nosqlbench_tree | - | Not enabled for PRs. perf_sysbench | - | Not enabled for PRs. perf_tpcc | - | Not enabled for PRs. perf_tpch | - | Not enabled for PRs. perf_ycsb_hash | - | Not enabled for PRs. perf_ycsb_tree | - | Not enabled for PRs. publish-module-api-doc | - | No Doxygen in LuaJIT. release | + | Tarantool release build. release_asan_clang | + | Tarantool release build. release_clang | + | Tarantool release build. release_lto | + | Tarantool release build. release_lto_clang | + | Tarantool release build. reusable_build | - | Utility for integration. source | - | Not enabled for PRs. static_build | + | Tarantool static build. static_build_cmake_linux | - | Just an OOS static build. static_build_pack_test_deploy | - | Utility for packaging. submodule_update | - | Not enabled for PRs. [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations [2]: https://github.com/orgs/community/discussions/45342#discussioncomment-4779360 (cherry picked from commit 0dfa2cec5febc7a1796388b6e96a7b58b9006d51) --- Branch: https://github.com/tarantool/luajit/tree/mkokryashkin/integration-testing-3.0 .../integration-tarantool-ecosystem.yml | 41 ++++++ .github/workflows/integration-tarantool.yml | 118 ++++++++++++++++++ .github/workflows/testing.yml | 9 ++ 3 files changed, 168 insertions(+) create mode 100644 .github/workflows/integration-tarantool-ecosystem.yml create mode 100644 .github/workflows/integration-tarantool.yml diff --git a/.github/workflows/integration-tarantool-ecosystem.yml b/.github/workflows/integration-tarantool-ecosystem.yml new file mode 100644 index 00000000..0297e8d8 --- /dev/null +++ b/.github/workflows/integration-tarantool-ecosystem.yml @@ -0,0 +1,41 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# This workflow file was created to avoid this limitation. +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations +name: 'Integration / Tarantool ecosystem' + +on: + push: + branches-ignore: + - '**-notest' + - '**-nointegration' + - 'upstream-**' + tags-ignore: + - '**' + +concurrency: + # An update of a developer branch cancels the previously + # scheduled workflow run for this branch. However, the default + # branch, and long-term branch (tarantool/release/2.11, + # tarantool/release/2.10, etc) workflow runs are never canceled. + # + # We use a trick here: define the concurrency group as 'workflow + # run ID' + # 'workflow run attempt' because it is a unique + # combination for any run. So it effectively discards grouping. + # + # XXX: we cannot use `github.sha` as a unique identifier because + # pushing a tag may cancel a run that works on a branch push + # event. + group: ${{ startsWith(github.ref, 'refs/heads/tarantool/') + && format('{0}-{1}', github.run_id, github.run_attempt) + || format('{0}-{1}', github.workflow, github.ref) }} + cancel-in-progress: true + +jobs: + test-tarantool-integration: + uses: tarantool/tarantool/.github/workflows/integration.yml@release/3.0 + secrets: inherit + with: + submodule: luajit + revision: ${{ github.sha }} diff --git a/.github/workflows/integration-tarantool.yml b/.github/workflows/integration-tarantool.yml new file mode 100644 index 00000000..178ef2a4 --- /dev/null +++ b/.github/workflows/integration-tarantool.yml @@ -0,0 +1,118 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# This workflow file was created to avoid this limitation. +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations +name: 'Integration / Tarantool' + +on: + push: + branches-ignore: + - '**-notest' + - '**-nointegration' + - 'upstream-**' + tags-ignore: + - '**' + +concurrency: + # An update of a developer branch cancels the previously + # scheduled workflow run for this branch. However, the default + # branch, and long-term branch (tarantool/release/2.11, + # tarantool/release/2.10, etc) workflow runs are never canceled. + # + # We use a trick here: define the concurrency group as 'workflow + # run ID' + # 'workflow run attempt' because it is a unique + # combination for any run. So it effectively discards grouping. + # + # XXX: we cannot use `github.sha` as a unique identifier because + # pushing a tag may cancel a run that works on a branch push + # event. + group: ${{ startsWith(github.ref, 'refs/heads/tarantool/') + && format('{0}-{1}', github.run_id, github.run_attempt) + || format('{0}-{1}', github.workflow, github.ref) }} + cancel-in-progress: true + +jobs: + test-tarantool-coverage: + name: coverage + uses: tarantool/tarantool/.github/workflows/coverage.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug: + name: debug + uses: tarantool/tarantool/.github/workflows/debug.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug_aarch64: + name: debug aarch64 + uses: tarantool/tarantool/.github/workflows/debug_aarch64.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug_asan_clang: + name: debug ASAN clang + uses: tarantool/tarantool/.github/workflows/debug_asan_clang.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-default_gcc_centos_7: + name: gcc centos 7 + uses: tarantool/tarantool/.github/workflows/default_gcc_centos_7.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-out_of_source: + name: out of source + uses: tarantool/tarantool/.github/workflows/out_of_source.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release: + name: release + uses: tarantool/tarantool/.github/workflows/release.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_asan_clang: + name: release ASAN clang + uses: tarantool/tarantool/.github/workflows/release_asan_clang.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_clang: + name: release clang + uses: tarantool/tarantool/.github/workflows/release_clang.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_lto: + name: release lto + uses: tarantool/tarantool/.github/workflows/release_lto.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_lto_clang: + name: release lto clang + uses: tarantool/tarantool/.github/workflows/release_lto_clang.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-static_build: + name: static build + uses: tarantool/tarantool/.github/workflows/static_build.yml@release/3.0 + with: + submodule: luajit + revision: ${{ github.sha }} diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index cb4ba57b..1581c8fc 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -1,3 +1,11 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# Some other jobs are started in separate workflow files to +# overcome the limitation. The jobs are Tarantool ecosystem +# integration testing (tarantool-ecosystem.yml) and exotic +# Tarantool builds (tarantool-exotic.yml). +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations name: Testing on: @@ -108,6 +116,7 @@ jobs: ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }} needs: test-luajit + # XXX: Only LuaJIT-tests are running in this workflow. uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master with: CMAKE_EXTRA_PARAMS: > -- 2.44.0
This patch makes use of reusable workflows from the Tarantool repository, so now LuaJIT CI tests integration in all relevant workflows. Also, this patch disables MacOS testing for Tarantool integration since those runs were made nightly in the Tarantool repo. As noted in the documentation [1], it is impossible for a single workflow call tree to include more than 20 workflows. To overcome this limitation, workflows are started in a bunch of separate files. This makes it impossible to depend on LuaJIT-only jobs for integration workflows such as `tarantool-ecosystem.yml` or `tarantool-exotic.yml` since it is impossible to make cross-dependencies between workflow files. The name of a callee workflow cannot be substituted using a matrix [2], so workflow calls are copied and pasted instead. Table below shows which workflows are enabled and why. Workflow name |+/-| Reason ----------------------------------------------------------------- almalinux_8_aarch64 | - | No LuaJIT-relevant variety. almalinux_8 | - | No LuaJIT-relevant variety. almalinux_9_aarch64 | - | No LuaJIT-relevant variety. almalinux_9 | - | No LuaJIT-relevant variety. alpine_3_16_aarch64 | - | No LuaJIT-relevant variety. alpine_3_16 | - | No LuaJIT-relevant variety. centos_7_aarch64 | - | No LuaJIT-relevant variety. centos_7 | - | No LuaJIT-relevant variety. centos_8_aarch64 | - | No LuaJIT-relevant variety. centos_8 | - | No LuaJIT-relevant variety. codeql | - | Not relevant to LuaJIT. coverage | + | Long tests for profilers. coverity | - | Cron workflow. debian_10_aarch64 | - | No LuaJIT-relevant variety. debian_10 | - | No LuaJIT-relevant variety. debian_11_aarch64 | - | No LuaJIT-relevant variety. debian_11 | - | No LuaJIT-relevant variety. debian_12_aarch64 | - | No LuaJIT-relevant variety. debian_12 | - | No LuaJIT-relevant variety. debian_9 | - | No LuaJIT-relevant variety. debug | + | Tarantool debug build. debug_aarch64 | + | Tarantool debug build. debug_asan_clang | + | Tarantool debug build. default_gcc_centos_7 | + | Old gcc version (7). fedora_34_aarch64 | - | No LuaJIT-relevant variety. fedora_34 | - | No LuaJIT-relevant variety. fedora_35_aarch64 | - | No LuaJIT-relevant variety. fedora_35 | - | No LuaJIT-relevant variety. fedora_36_aarch64 | - | No LuaJIT-relevant variety. fedora_36 | - | No LuaJIT-relevant variety. fedora_37_aarch64 | - | No LuaJIT-relevant variety. fedora_37 | - | No LuaJIT-relevant variety. fedora_38_aarch64 | - | No LuaJIT-relevant variety. fedora_38 | - | No LuaJIT-relevant variety. fedora_39_aarch64 | - | No LuaJIT-relevant variety. fedora_39 | - | No LuaJIT-relevant variety. freebsd | - | Nightly build. fuzzing | - | Impossible to bump LuaJIT. integration | + | Tarantool ecosystem. jepsen-cluster-txm | - | Manual workflow. jepsen-cluster | - | Manual workflow. jepsen-single-instance-txm | - | Cron workflow. jepsen-single-instance | - | Cron workflow. lango-stale-reviews | - | Cron workflow. lint | - | LuaJIT has its own lint. luajit-integration | + | Run tests under Tarantool. memtx_allocator_based_on_malloc | - | Not relevant to LuaJIT. osx | - | Nightly build. out_of_source | + | Out of source build. packaging | - | No LuaJIT-relevant variety. perf_cbench | - | Not enabled for PRs. perf_linkbench_ssd | - | Not enabled for PRs. perf_micro | - | Not relevant to LuaJIT. perf_nosqlbench_hash | - | Not enabled for PRs. perf_nosqlbench_tree | - | Not enabled for PRs. perf_sysbench | - | Not enabled for PRs. perf_tpcc | - | Not enabled for PRs. perf_tpch | - | Not enabled for PRs. perf_ycsb_hash | - | Not enabled for PRs. perf_ycsb_tree | - | Not enabled for PRs. publish-module-api-doc | - | No Doxygen in LuaJIT. redos_7_3.yaml | - | No LuaJIT-relevant variety. release | + | Tarantool release build. release_asan_clang | + | Tarantool release build. release_clang | + | Tarantool release build. release_lto | + | Tarantool release build. release_lto_clang | + | Tarantool release build. reusable_build | - | Utility for integration. source | - | Not enabled for PRs. static_build | + | Tarantool static build. static_build_cmake_linux | - | Just an OOS static build. static_build_pack_test_deploy | - | Utility for packaging. submodule_update | - | Not enabled for PRs. ubuntu_16_04 | - | No LuaJIT-relevant variety. ubuntu_18_04 | - | No LuaJIT-relevant variety. ubuntu_20_04_aarch64 | - | No LuaJIT-relevant variety. ubuntu_20_04 | - | No LuaJIT-relevant variety. ubuntu_22_04_aarch64 | - | No LuaJIT-relevant variety. ubuntu_22_04 | - | No LuaJIT-relevant variety. [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations [2]: https://github.com/orgs/community/discussions/45342#discussioncomment-4779360 (cherry picked from commit 0dfa2cec5febc7a1796388b6e96a7b58b9006d51) --- Branch: https://github.com/tarantool/luajit/tree/mkokryashkin/integration-testing-2.11 .../integration-tarantool-ecosystem.yml | 41 ++++++ .github/workflows/integration-tarantool.yml | 118 ++++++++++++++++++ .github/workflows/testing.yml | 9 ++ 3 files changed, 168 insertions(+) create mode 100644 .github/workflows/integration-tarantool-ecosystem.yml create mode 100644 .github/workflows/integration-tarantool.yml diff --git a/.github/workflows/integration-tarantool-ecosystem.yml b/.github/workflows/integration-tarantool-ecosystem.yml new file mode 100644 index 00000000..5ad71260 --- /dev/null +++ b/.github/workflows/integration-tarantool-ecosystem.yml @@ -0,0 +1,41 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# This workflow file was created to avoid this limitation. +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations +name: 'Integration / Tarantool ecosystem' + +on: + push: + branches-ignore: + - '**-notest' + - '**-nointegration' + - 'upstream-**' + tags-ignore: + - '**' + +concurrency: + # An update of a developer branch cancels the previously + # scheduled workflow run for this branch. However, the default + # branch, and long-term branch (tarantool/release/2.11, + # tarantool/release/2.10, etc) workflow runs are never canceled. + # + # We use a trick here: define the concurrency group as 'workflow + # run ID' + # 'workflow run attempt' because it is a unique + # combination for any run. So it effectively discards grouping. + # + # XXX: we cannot use `github.sha` as a unique identifier because + # pushing a tag may cancel a run that works on a branch push + # event. + group: ${{ startsWith(github.ref, 'refs/heads/tarantool/') + && format('{0}-{1}', github.run_id, github.run_attempt) + || format('{0}-{1}', github.workflow, github.ref) }} + cancel-in-progress: true + +jobs: + test-tarantool-integration: + uses: tarantool/tarantool/.github/workflows/integration.yml@release/2.11 + secrets: inherit + with: + submodule: luajit + revision: ${{ github.sha }} diff --git a/.github/workflows/integration-tarantool.yml b/.github/workflows/integration-tarantool.yml new file mode 100644 index 00000000..5ab84b93 --- /dev/null +++ b/.github/workflows/integration-tarantool.yml @@ -0,0 +1,118 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# This workflow file was created to avoid this limitation. +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations +name: 'Integration / Tarantool' + +on: + push: + branches-ignore: + - '**-notest' + - '**-nointegration' + - 'upstream-**' + tags-ignore: + - '**' + +concurrency: + # An update of a developer branch cancels the previously + # scheduled workflow run for this branch. However, the default + # branch, and long-term branch (tarantool/release/2.11, + # tarantool/release/2.10, etc) workflow runs are never canceled. + # + # We use a trick here: define the concurrency group as 'workflow + # run ID' + # 'workflow run attempt' because it is a unique + # combination for any run. So it effectively discards grouping. + # + # XXX: we cannot use `github.sha` as a unique identifier because + # pushing a tag may cancel a run that works on a branch push + # event. + group: ${{ startsWith(github.ref, 'refs/heads/tarantool/') + && format('{0}-{1}', github.run_id, github.run_attempt) + || format('{0}-{1}', github.workflow, github.ref) }} + cancel-in-progress: true + +jobs: + test-tarantool-coverage: + name: coverage + uses: tarantool/tarantool/.github/workflows/coverage.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug: + name: debug + uses: tarantool/tarantool/.github/workflows/debug.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug_aarch64: + name: debug aarch64 + uses: tarantool/tarantool/.github/workflows/debug_aarch64.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-debug_asan_clang: + name: debug ASAN clang + uses: tarantool/tarantool/.github/workflows/debug_asan_clang.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-default_gcc_centos_7: + name: gcc centos 7 + uses: tarantool/tarantool/.github/workflows/default_gcc_centos_7.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-out_of_source: + name: out of source + uses: tarantool/tarantool/.github/workflows/out_of_source.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release: + name: release + uses: tarantool/tarantool/.github/workflows/release.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_asan_clang: + name: release ASAN clang + uses: tarantool/tarantool/.github/workflows/release_asan_clang.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_clang: + name: release clang + uses: tarantool/tarantool/.github/workflows/release_clang.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_lto: + name: release lto + uses: tarantool/tarantool/.github/workflows/release_lto.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-release_lto_clang: + name: release lto clang + uses: tarantool/tarantool/.github/workflows/release_lto_clang.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} + + test-tarantool-static_build: + name: static build + uses: tarantool/tarantool/.github/workflows/static_build.yml@release/2.11 + with: + submodule: luajit + revision: ${{ github.sha }} diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index 031a153f..c6fa1105 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -1,3 +1,11 @@ +# XXX: A single call tree for reusable workflows can't exceed the +# total of 20 workflows, as stated in the documentation [1]. +# Some other jobs are started in separate workflow files to +# overcome the limitation. The jobs are Tarantool ecosystem +# integration testing (tarantool-ecosystem.yml) and exotic +# Tarantool builds (tarantool-exotic.yml). +# +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations name: Testing on: @@ -108,6 +116,7 @@ jobs: ${{ matrix.BUILDTYPE }} GC64:${{ matrix.GC64 }} needs: test-luajit + # XXX: Only LuaJIT-tests are running in this workflow. uses: tarantool/tarantool/.github/workflows/luajit-integration.yml@master with: CMAKE_EXTRA_PARAMS: > -- 2.44.0
Hi, Maxim! Thanks for the fixes! LGTM! -- Best regards, Sergey Kaplun
From: sergeyb@tarantool.org Thanks to Carlo Cabrera. (cherry picked from commit b98b37231bd2dcb79e10b0f974cefd91eb0d7b3a) Mach-O FAR object files generated by LuaJIT for arm64 had an incorrect format because FFI structure used for generation was wrong: `mach_fat_obj` instead of `mach_fat_obj_64`. Sergey Bronnikov: * added the description and a test for the problem Part of tarantool/tarantool#9595 --- src/jit/bcsave.lua | 14 +++++++++++++- ...-fix_generation_of_mach-o_object_files.test.lua | 4 +++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua index 7aec1555..069cf1a3 100644 --- a/src/jit/bcsave.lua +++ b/src/jit/bcsave.lua @@ -491,6 +491,18 @@ typedef struct { mach_nlist sym_entry; uint8_t space[4096]; } mach_fat_obj; +typedef struct { + mach_fat_header fat; + mach_fat_arch fat_arch[2]; + struct { + mach_header_64 hdr; + mach_segment_command_64 seg; + mach_section_64 sec; + mach_symtab_command sym; + } arch[2]; + mach_nlist_64 sym_entry; + uint8_t space[4096]; +} mach_fat_obj_64; ]] local symname = '_'..LJBC_PREFIX..ctx.modname local isfat, is64, align, mobj = false, false, 4, "mach_obj" @@ -499,7 +511,7 @@ typedef struct { elseif ctx.arch == "arm" then isfat, mobj = true, "mach_fat_obj" elseif ctx.arch == "arm64" then - is64, align, isfat, mobj = true, 8, true, "mach_fat_obj" + is64, align, isfat, mobj = true, 8, true, "mach_fat_obj_64" else check(ctx.arch == "x86", "unsupported architecture for OSX") end diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua index 0519e134..0a11f163 100644 --- a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua @@ -7,7 +7,7 @@ local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({ ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL, }) -test:plan(4) +test:plan(8) -- Test creates an object file in Mach-O format with LuaJIT bytecode -- and checks validness of the object file fields. @@ -267,5 +267,7 @@ end -- ARM build_and_check_mach_o(false) +-- ARM64 +build_and_check_mach_o(true) test:done(true) -- 2.34.1
From: sergeyb@tarantool.org Thanks to Carlo Cabrera. (cherry picked from commit 3065c910ad6027031aabe2dfd3c26a3d0f014b4f) Mach-O FAT object constructed by LuaJIT had an incorrect format. The problem is reproduced when target hardware platform has AVX512F and LuaJIT is compiled with enabled AVX512F instructions. The problem is arise because LuaJIT FFI code for Mach-O file generation in `bcsave.lua` relied on undefined behavior for conversions to `uint32_t`. AVX512F has the `vcvttsd2usi` instruction which converts `double`/`float` to `uint32_t/uint64_t`. Earlier architectures (SSE2, AVX2) are sorely lacking such an instruction, as they only support signed conversions. Unsigned conversions are done with a signed convert and range shifting - the exact algorithm depends on the compiler. A side-effect of these workarounds is that negative `double`/`float` often inadvertently convert 'as expected', even though this is invoking undefined behavior. Whereas `vcvttsd2usi` always returns 0x80000000 or 0x8000000000000000 for out-of-range inputs. The patch fixes the problem, however, the real issue remains unfixed. Sergey Bronnikov: * added the description and a test for the problem Part of tarantool/tarantool#9595 --- .github/workflows/exotic-builds-testing.yml | 5 +- src/jit/bcsave.lua | 6 +- .../lj-366-strtab-correct-size.test.lua | 10 +- ...generation_of_mach-o_object_files.test.lua | 271 ++++++++++++++++++ test/tarantool-tests/utils/tools.lua | 8 + 5 files changed, 287 insertions(+), 13 deletions(-) create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua diff --git a/.github/workflows/exotic-builds-testing.yml b/.github/workflows/exotic-builds-testing.yml index a9ba5fd5..df4bc2e9 100644 --- a/.github/workflows/exotic-builds-testing.yml +++ b/.github/workflows/exotic-builds-testing.yml @@ -32,6 +32,7 @@ jobs: fail-fast: false matrix: BUILDTYPE: [Debug, Release] + OS: [Linux, macOS] ARCH: [ARM64, x86_64] GC64: [ON, OFF] FLAVOR: [checkhook, dualnum, gdbjit, nojit, nounwind] @@ -50,13 +51,15 @@ jobs: FLAVORFLAGS: -DLUAJIT_USE_GDBJIT=ON - FLAVOR: nounwind FLAVORFLAGS: -DLUAJIT_NO_UNWIND=ON + - FLAVOR: avx512 + CMAKEFLAGS: -DCMAKE_C_FLAGS=skylake-avx512 -DCMAKE_C_COMPILER=gcc exclude: - ARCH: ARM64 GC64: OFF # DUALNUM is default for ARM64, no need for additional testing. - FLAVOR: dualnum ARCH: ARM64 - runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}'] + runs-on: [self-hosted, regular, Linux, '${{ matrix.ARCH }}', '${ matrix.OS }'] name: > LuaJIT ${{ matrix.FLAVOR }} (Linux/${{ matrix.ARCH }}) diff --git a/src/jit/bcsave.lua b/src/jit/bcsave.lua index a287d675..7aec1555 100644 --- a/src/jit/bcsave.lua +++ b/src/jit/bcsave.lua @@ -446,18 +446,18 @@ typedef struct { uint32_t value; } mach_nlist; typedef struct { - uint32_t strx; + int32_t strx; uint8_t type, sect; uint16_t desc; uint64_t value; } mach_nlist_64; typedef struct { - uint32_t magic, nfat_arch; + int32_t magic, nfat_arch; } mach_fat_header; typedef struct { - uint32_t cputype, cpusubtype, offset, size, align; + int32_t cputype, cpusubtype, offset, size, align; } mach_fat_arch; typedef struct { struct { diff --git a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua index 8a97a441..0bb92da6 100644 --- a/test/tarantool-tests/lj-366-strtab-correct-size.test.lua +++ b/test/tarantool-tests/lj-366-strtab-correct-size.test.lua @@ -138,14 +138,6 @@ local function create_obj_file(name) return elf_filename end --- Reads a file located in a specified path and returns its content. -local function read_file(path) - local file = assert(io.open(path), 'cannot open an object file') - local content = file:read('*a') - file:close() - return content -end - -- Parses a buffer in an ELF format and returns an offset and a size of strtab -- and symtab sections. local function read_elf(elf_content) @@ -172,7 +164,7 @@ end test:plan(3) local elf_filename = create_obj_file(MODULE_NAME) -local elf_content = read_file(elf_filename) +local elf_content = require('utils').tools.read_file(elf_filename) assert(#elf_content ~= 0, 'cannot read an object file') local strtab, symtab = read_elf(elf_content) diff --git a/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua new file mode 100644 index 00000000..0519e134 --- /dev/null +++ b/test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua @@ -0,0 +1,271 @@ +local tap = require('tap') +local test = tap.test('lj-865-fix_generation_of_mach-o_object_files'):skipcond({ + -- XXX: Tarantool doesn't use default LuaJIT loaders, and Lua + -- bytecode can't be loaded from the shared library. For more + -- info: https://github.com/tarantool/tarantool/issues/9671. + -- luacheck: no global + ['Test uses exotic type of loaders (see #9671)'] = _TARANTOOL, +}) + +test:plan(4) + +-- Test creates an object file in Mach-O format with LuaJIT bytecode +-- and checks validness of the object file fields. +-- +-- The original problem is reproduced with LuaJIT that built with +-- enabled AVX512F instructions. The support of AVX512F could be +-- checked in `/proc/cpuinfo` on Linux and +-- `sysctl hw.optional.avx512f` on Mac. AVX512F must be +-- implicitly enabled in a C compiler by passing CPU codename. +-- Please consult for available model architecture on GCC Online +-- Documentation [1] for available CPU codenames. To detect +-- CPU codename execute `gcc -march=native -Q --help=target | grep march`. +-- +-- 1. https://gcc.gnu.org/onlinedocs/gcc/x86-Options.html +-- +-- Manual steps for reproducing are the following: +-- +-- $ make CC=gcc TARGET_CFLAGS='skylake-avx512' -f Makefile.original +-- $ echo > test.lua +-- $ LUA_PATH="src/?.lua;;" luajit -b -o osx -a arm test.lua test.o +-- $ file test.o +-- empty.o: DOS executable (block device driver) + +local ffi = require('ffi') + +-- Format of the Mach-O is described in a document +-- "OS X ABI Mach-O File Format Reference", published by Apple company. +-- Copy of the (now removed) official documentation in [1]. +-- Yet another source of thruth is a XNU headers, see the definition +-- of C-structures in: [2] (`nlist_64`), [3] (`fat_arch` and `fat_header`). + +-- 1. https://github.com/aidansteele/osx-abi-macho-file-format-reference +-- 2. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/nlist.h +-- 3. https://github.com/apple-oss-distributions/xnu/blob/xnu-10002.1.13/EXTERNAL_HEADERS/mach-o/fat.h +-- 4. https://developer.apple.com/documentation/apple-silicon/addressing-architectural-differences-in-your-macos-code +-- +-- Using the same declarations as defined in <src/jit/bcsave.lua>. +ffi.cdef[[ +typedef struct +{ + uint32_t magic, cputype, cpusubtype, filetype, ncmds, sizeofcmds, flags; +} mach_header; + +typedef struct +{ + mach_header; uint32_t reserved; +} mach_header_64; + +typedef struct { + uint32_t cmd, cmdsize; + char segname[16]; + uint32_t vmaddr, vmsize, fileoff, filesize; + uint32_t maxprot, initprot, nsects, flags; +} mach_segment_command; + +typedef struct { + uint32_t cmd, cmdsize; + char segname[16]; + uint64_t vmaddr, vmsize, fileoff, filesize; + uint32_t maxprot, initprot, nsects, flags; +} mach_segment_command_64; + +typedef struct { + char sectname[16], segname[16]; + uint32_t addr, size; + uint32_t offset, align, reloff, nreloc, flags; + uint32_t reserved1, reserved2; +} mach_section; + +typedef struct { + char sectname[16], segname[16]; + uint64_t addr, size; + uint32_t offset, align, reloff, nreloc, flags; + uint32_t reserved1, reserved2, reserved3; +} mach_section_64; + +typedef struct { + uint32_t cmd, cmdsize, symoff, nsyms, stroff, strsize; +} mach_symtab_command; + +typedef struct { + int32_t strx; + uint8_t type, sect; + int16_t desc; + uint32_t value; +} mach_nlist; + +typedef struct { + uint32_t strx; + uint8_t type, sect; + uint16_t desc; + uint64_t value; +} mach_nlist_64; + +typedef struct +{ + uint32_t magic, nfat_arch; +} mach_fat_header; + +typedef struct +{ + uint32_t cputype, cpusubtype, offset, size, align; +} mach_fat_arch; + +typedef struct { + mach_fat_header fat; + mach_fat_arch fat_arch[2]; + struct { + mach_header hdr; + mach_segment_command seg; + mach_section sec; + mach_symtab_command sym; + } arch[2]; + mach_nlist sym_entry; + uint8_t space[4096]; +} mach_fat_obj; + +typedef struct { + mach_fat_header fat; + mach_fat_arch fat_arch[2]; + struct { + mach_header_64 hdr; + mach_segment_command_64 seg; + mach_section_64 sec; + mach_symtab_command sym; + } arch[2]; + mach_nlist_64 sym_entry; + uint8_t space[4096]; +} mach_fat_obj_64; +]] + +local function create_obj_file(name, arch) + local mach_o_path = os.tmpname() .. '.o' + local lua_path = os.getenv('LUA_PATH') + local lua_bin = require('utils').exec.luacmd(arg):match('%S+') + local cmd_fmt = 'LUA_PATH="%s" %s -b -n "%s" -o osx -a %s -e "print()" %s' + local cmd = (cmd_fmt):format(lua_path, lua_bin, name, arch, mach_o_path) + local ret = os.execute(cmd) + assert(ret == 0, 'cannot create an object file') + return mach_o_path +end + +-- Parses a buffer in an Mach-O format and returns +-- an fat magic number and nfat_arch. +local function read_mach_o(buf, is64) + local res = { + header = { + magic = 0, + nfat_arch = 0, + }, + fat_arch = { + }, + } + + -- Mach-O FAT object. + local mach_fat_obj_type = ffi.typeof(is64 and 'mach_fat_obj_64 *' or 'mach_fat_obj *') + local obj = ffi.cast(mach_fat_obj_type, buf) + + -- Mach-O FAT object header. + local mach_fat_header_type = ffi.typeof('mach_fat_header *') + local mach_fat_header = ffi.cast(mach_fat_header_type, obj.fat) + local be32 = bit.bswap -- Mach-O FAT is BE, target arch is LE. + res.header.magic = be32(mach_fat_header.magic) + res.header.nfat_arch = be32(mach_fat_header.nfat_arch) + + -- Mach-O FAT object archs. + local mach_fat_arch_type = ffi.typeof('mach_fat_arch *') + for i = 0, res.header.nfat_arch - 1 do + local fat_arch = ffi.cast(mach_fat_arch_type, obj.fat_arch[i]) + arch = { + cputype = be32(fat_arch.cputype), + cpusubtype = be32(fat_arch.cpusubtype), + } + table.insert(res.fat_arch, arch) + end + + return res +end + +-- Defined in <src/jit/bcsave.lua:bcsave_machobj>. +local sum_cputype = { + x86 = 7, + x64 = 0x01000007, + arm = 7 + 12, + arm64 = 0x01000007 + 0x0100000c, +} +local sum_cpusubtype = { + x86 = 3, + x64 = 3, + arm = 3 + 9, + arm64 = 3 + 0, +} + +-- The function builds Mach-O FAT object file and retrieves +-- its header fields (magic and nfat_arch) +-- and fields of the each arch (cputype, cpusubtype). +-- +-- Mach-O FAT object header could be retrieved with `otool` on macOS: +-- +-- $ otool -f empty.o +-- Fat headers +-- fat_magic 0xcafebabe +-- nfat_arch 2 +-- <snipped> +-- +-- CPU type and subtype could be retrieved with `lipo` on macOS: +-- +-- $ luajit -b -o osx -a arm empty.lua empty.o +-- $ lipo -archs empty.o +-- i386 armv7 +-- $ luajit -b -o osx -a arm64 empty.lua empty.o +-- $ lipo -archs empty.o +-- x86_64 arm64 +local function build_and_check_mach_o(is64) + local arch = is64 and 'arm64' or 'arm' + + -- FAT_MAGIC is an integer containing the value 0xCAFEBABE in + -- big-endian byte order format. On a big-endian host CPU, + -- this can be validated using the constant FAT_MAGIC; + -- on a little-endian host CPU, it can be validated using + -- the constant FAT_CIGAM. + -- + -- FAT_NARCH is an integer specifying the number of fat_arch + -- data structures that follow. This is the number of + -- architectures contained in this binary. + -- + -- See aforementioned "OS X ABI Mach-O File Format Reference". + -- + local FAT_MAGIC = '0xffffffffcafebabe' + local FAT_NARCH = 2 + + local MODULE_NAME = 'lango_team' + + local mach_o_obj_path = create_obj_file(MODULE_NAME, arch) + local mach_o_buf = require('utils').tools.read_file(mach_o_obj_path) + assert(mach_o_buf == nil or #mach_o_buf ~= 0, 'cannot read an object file') + + local mach_o = read_mach_o(mach_o_buf, is64) + + -- Teardown. + local retcode = os.remove(mach_o_obj_path) + assert(retcode == true, 'remove an object file') + + local magic_str = string.format('0x%02x', mach_o.header.magic) + test:is(magic_str, FAT_MAGIC, 'fat_magic is correct in Mach-O, ' .. arch) + test:is(mach_o.header.nfat_arch, FAT_NARCH, 'nfat_arch is correct in Mach-O, ' .. arch) + + local total_cputype = 0 + local total_cpusubtype = 0 + for i = 1, mach_o.header.nfat_arch do + total_cputype = total_cputype + mach_o.fat_arch[i].cputype + total_cpusubtype = total_cpusubtype + mach_o.fat_arch[i].cpusubtype + end + test:is(total_cputype, sum_cputype[arch], 'cputype is correct in Mach-O, ' .. arch) + test:is(total_cpusubtype, sum_cpusubtype[arch], 'cpusubtype is correct in Mach-O, ' .. arch) +end + +-- ARM +build_and_check_mach_o(false) + +test:done(true) diff --git a/test/tarantool-tests/utils/tools.lua b/test/tarantool-tests/utils/tools.lua index f35c6922..26b8c08d 100644 --- a/test/tarantool-tests/utils/tools.lua +++ b/test/tarantool-tests/utils/tools.lua @@ -12,4 +12,12 @@ function M.profilename(name) return (arg[0]:gsub('^(.+)/([^/]+)%.test%.lua$', replacepattern)) end +-- Reads a file located in a specified path and returns its content. +function M.read_file(path) + local file = assert(io.open(path), 'cannot open an object file') + local content = file:read('*a') + file:close() + return content +end + return M -- 2.34.1
From: Sergey Bronnikov <sergeyb@tarantool.org> Issues: - https://github.com/LuaJIT/LuaJIT/issues/649 - https://github.com/LuaJIT/LuaJIT/pull/863 - https://github.com/LuaJIT/LuaJIT/issues/865 - Epic: https://github.com/tarantool/tarantool/issues/9595 PR: https://github.com/tarantool/tarantool/pull/9814 Branch: https://github.com/tarantool/luajit/tree/ligurio/lj-865-fix_generation_of_mach-o_object_files Mike Pall (2): OSX/iOS/ARM64: Fix generation of Mach-O object files. OSX/iOS/ARM64: Fix bytecode embedding in Mach-O object file. .github/workflows/exotic-builds-testing.yml | 5 +- src/jit/bcsave.lua | 20 +- .../lj-366-strtab-correct-size.test.lua | 10 +- ...generation_of_mach-o_object_files.test.lua | 273 ++++++++++++++++++ test/tarantool-tests/utils/tools.lua | 8 + 5 files changed, 302 insertions(+), 14 deletions(-) create mode 100644 test/tarantool-tests/lj-865-fix_generation_of_mach-o_object_files.test.lua -- 2.34.1
There is no error-checking in profilers which results in raw Lua errors being reported in cases of non-existing paths or corrupt file structures. This patch adds a profiler-agnostic module for event parsing, which is going to be used to introduce user-friendly errors. It is impossible to enable it right now because it should be included in Tarantool's build procedure first. Part of tarantool/tarantool#9217 Part of tarantool/tarantool#5994 --- Makefile.original | 2 +- tools/CMakeLists.txt | 4 ++++ tools/utils/evread.lua | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 tools/utils/evread.lua diff --git a/Makefile.original b/Makefile.original index d0834fe6..4a1e1d9d 100644 --- a/Makefile.original +++ b/Makefile.original @@ -97,7 +97,7 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \ dis_x86.lua dis_x64.lua dis_arm.lua dis_arm64.lua \ dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \ dis_mips64.lua dis_mips64el.lua vmdef.lua -FILES_UTILSLIB= avl.lua bufread.lua symtab.lua +FILES_UTILSLIB= avl.lua bufread.lua evread.lua symtab.lua FILES_MEMPROFLIB= humanize.lua parse.lua process.lua FILES_SYSPROFLIB= parse.lua FILES_TOOLSLIB= memprof.lua sysprof.lua diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index a4adc15b..695c079a 100644 --- a/tools/CMakeLists.txt +++ b/tools/CMakeLists.txt @@ -18,6 +18,7 @@ else() memprof.lua utils/avl.lua utils/bufread.lua + utils/evread.lua utils/symtab.lua ) list(APPEND LUAJIT_TOOLS_DEPS tools-parse-memprof) @@ -36,6 +37,7 @@ else() install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua + ${CMAKE_CURRENT_SOURCE_DIR}/utils/evread.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua DESTINATION ${LUAJIT_DATAROOTDIR}/utils PERMISSIONS @@ -65,6 +67,7 @@ else() sysprof.lua utils/avl.lua utils/bufread.lua + utils/evread.lua utils/symtab.lua ) list(APPEND LUAJIT_TOOLS_DEPS tools-parse-sysprof) @@ -81,6 +84,7 @@ else() install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua + ${CMAKE_CURRENT_SOURCE_DIR}/utils/evread.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua DESTINATION ${LUAJIT_DATAROOTDIR}/utils PERMISSIONS diff --git a/tools/utils/evread.lua b/tools/utils/evread.lua new file mode 100644 index 00000000..394a4a03 --- /dev/null +++ b/tools/utils/evread.lua @@ -0,0 +1,32 @@ +local bufread = require('utils.bufread') +local symtab = require('utils.symtab') + +local function make_error_handler(fmt, inputfile) + return function(err) + io.stderr:write(string.format(fmt, inputfile)) + io.stderr:write(string.format('\n\t%s\n', err)) + os.exit(1, true) + end +end + +return function(parser, inputfile) + local _, reader = xpcall( + bufread.new, + make_error_handler('Failed to open %s.', inputfile), + inputfile + ) + + local _, symbols = xpcall( + symtab.parse, + make_error_handler('Failed to parse symtab from %s.', inputfile), + reader + ) + + local _, events = xpcall( + parser, + make_error_handler('Failed to parse profile data from %s.', inputfile), + reader, + symbols + ) + return events, symbols +end -- 2.44.0
Prior to this patch, memprof could report only the raw amount of bytes, which is hard to read. This patch adds the `--human-readable` CLI option to the memprof parser, so the memory is reported in KiB, MiB, or GiB, depending on what's the biggest reasonable unit. This patch also refactors the options mechanism for parser, so all of the options are passed into parser's modules as a single config table instead of being handled individually. Part of tarantool/tarantool#5994 --- .../gh-5994-memprof-human-readable.test.lua | 73 +++++++++++++++++++ tools/memprof.lua | 20 +++-- tools/memprof/humanize.lua | 46 +++++++++--- 3 files changed, 123 insertions(+), 16 deletions(-) create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua diff --git a/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua new file mode 100644 index 00000000..e34291be --- /dev/null +++ b/test/tarantool-tests/gh-5994-memprof-human-readable.test.lua @@ -0,0 +1,73 @@ +local tap = require('tap') +local test = tap.test('gh-5994-memprof-human-readable'):skipcond({ + ['Profile tools are implemented for x86_64 only'] = jit.arch ~= 'x86' and + jit.arch ~= 'x64', + ['Profile tools are implemented for Linux only'] = jit.os ~= 'Linux', + -- XXX: Tarantool integration is required to run this test properly. + -- luacheck: no global + ['No profile tools CLI option integration'] = _TARANTOOL, +}) + +local utils = require('utils') +local TMP_BINFILE_MEMPROF = utils.tools.profilename('memprofdata.tmp.bin') +local PARSE_CMD = utils.exec.luacmd(arg) .. ' -tm ' + +local function generate_output(bytes) + local res, err = misc.memprof.start(TMP_BINFILE_MEMPROF) + -- Should start successfully. + assert(res, err) + + -- luacheck: no unused + local _ = string.rep('_', bytes) + + res, err = misc.memprof.stop() + -- Should stop successfully. + assert(res, err) +end + +local TEST_SET = { + { + bytes = 2049, + match = '%dB', + hr = false, + name = 'non-human-readable mode is correct' + }, + { + bytes = 100, + match = '%dB', + hr = true, + name = 'human-readable mode: bytes' + }, + { + bytes = 2560, + match = '%d+%.%d%dKiB', + hr = true, + name = 'human-readable mode: float' + }, + { + bytes = 2048, + match = '%dKiB', + hr = true, + name = 'human-readable mode: KiB' + }, + { + bytes = 1024 * 1024, + match = '%dMiB', + hr = true, + name = 'human-readable mode: MiB' + }, + -- XXX: The test case for GiB is not implemented because it is + -- OOM-prone for non-GC64 builds. +} + +test:plan(#TEST_SET) + +for _, params in ipairs(TEST_SET) do + generate_output(params.bytes) + local cmd = PARSE_CMD .. (params.hr and ' --human-readable ' or '') + local output = io.popen(cmd .. TMP_BINFILE_MEMPROF):read('*all') + test:like(output, params.match, params.name) +end + +os.remove(TMP_BINFILE_MEMPROF) +test:done(true) diff --git a/tools/memprof.lua b/tools/memprof.lua index 955a1327..537cb869 100644 --- a/tools/memprof.lua +++ b/tools/memprof.lua @@ -22,6 +22,12 @@ local match, gmatch = string.match, string.gmatch -- Program options. local opt_map = {} +-- Default config for the memprof parser. +local config = { + leak_only = false, + human_readable = false, +} + function opt_map.help() stdout:write [[ luajit-parse-memprof - parser of the memory usage profile collected @@ -34,14 +40,18 @@ luajit-parse-memprof [options] memprof.bin Supported options are: --help Show this help and exit + --human-readable Use KiB/MiB/GiB notation instead of bytes --leak-only Report only leaks information ]] os.exit(0) end -local leak_only = false opt_map["leak-only"] = function() - leak_only = true + config.leak_only = true +end + +opt_map["human-readable"] = function() + config.human_readable = true end -- Print error and exit with error status. @@ -101,11 +111,11 @@ local function dump(inputfile) local reader = bufread.new(inputfile) local symbols = symtab.parse(reader) local events = memprof.parse(reader, symbols) - if not leak_only then - view.profile_info(events) + if not config.leak_only then + view.profile_info(events, config) end local dheap = process.form_heap_delta(events) - view.leak_info(dheap) + view.leak_info(dheap, config) view.aliases(symbols) -- XXX: The second argument is required to properly close Lua -- universe (i.e. invoke <lua_close> before exiting). diff --git a/tools/memprof/humanize.lua b/tools/memprof/humanize.lua index 5b289ce3..a0b1693a 100644 --- a/tools/memprof/humanize.lua +++ b/tools/memprof/humanize.lua @@ -5,7 +5,29 @@ local M = {} -function M.render(events) +local function human_readable_bytes(bytes) + local units = {"B", "KiB", "MiB", "GiB"} + local magnitude = 1 + + while bytes >= 1024 and magnitude < #units do + bytes = bytes / 1024 + magnitude = magnitude + 1 + end + local is_int = math.floor(bytes) == bytes + local fmt = is_int and "%d%s" or "%.2f%s" + + return string.format(fmt, bytes, units[magnitude]) +end + +local function format_bytes(bytes, config) + if config.human_readable then + return human_readable_bytes(bytes) + else + return string.format('%dB', bytes) + end +end + +function M.render(events, config) local ids = {} for id, _ in pairs(events) do @@ -18,11 +40,11 @@ function M.render(events) for i = 1, #ids do local event = events[ids[i]] - print(string.format("%s: %d events\t+%d bytes\t-%d bytes", + print(string.format("%s: %d events\t+%s\t-%s", event.loc, event.num, - event.alloc, - event.free + format_bytes(event.alloc, config), + format_bytes(event.free, config) )) local prim_loc = {} @@ -40,21 +62,21 @@ function M.render(events) end end -function M.profile_info(events) +function M.profile_info(events, config) print("ALLOCATIONS") - M.render(events.alloc) + M.render(events.alloc, config) print("") print("REALLOCATIONS") - M.render(events.realloc) + M.render(events.realloc, config) print("") print("DEALLOCATIONS") - M.render(events.free) + M.render(events.free, config) print("") end -function M.leak_info(dheap) +function M.leak_info(dheap, config) local leaks = {} for line, info in pairs(dheap) do -- Report "INTERNAL" events inconsistencies for profiling @@ -71,8 +93,10 @@ function M.leak_info(dheap) print("HEAP SUMMARY:") for _, l in pairs(leaks) do print(string.format( - "%s holds %d bytes: %d allocs, %d frees", - l.line, l.dbytes, dheap[l.line].nalloc, + "%s holds %s: %d allocs, %d frees", + l.line, + format_bytes(l.dbytes, config), + dheap[l.line].nalloc, dheap[l.line].nfree )) end -- 2.44.0
Both `humanize.profile_info` and `process.form_heap_delta` don't need the symtab, so the argument can be dropped. Part of tarantool/tarantool#5994 --- tools/memprof.lua | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/memprof.lua b/tools/memprof.lua index e23bbf24..955a1327 100644 --- a/tools/memprof.lua +++ b/tools/memprof.lua @@ -102,9 +102,9 @@ local function dump(inputfile) local symbols = symtab.parse(reader) local events = memprof.parse(reader, symbols) if not leak_only then - view.profile_info(events, symbols) + view.profile_info(events) end - local dheap = process.form_heap_delta(events, symbols) + local dheap = process.form_heap_delta(events) view.leak_info(dheap) view.aliases(symbols) -- XXX: The second argument is required to properly close Lua -- 2.44.0
The memprof parser used to have the `heap_chunk` data structure using numeric indices for its values, which is hardly readable and maintainable. This patch replaces these numeric indices with corresponding string keys. Part of tarantool/tarantool#5994 --- tools/memprof/parse.lua | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua index 42a601ef..cc66a23e 100644 --- a/tools/memprof/parse.lua +++ b/tools/memprof/parse.lua @@ -45,22 +45,30 @@ local function new_event(loc) } end +local function new_heap_chunk(size, id, loc) + return { + size = size, + id = id, + loc = loc, + } +end + local function link_to_previous(heap_chunk, e, nsize) -- Memory at this chunk was allocated before we start tracking. if heap_chunk then -- Save Lua code location (line) by address (id). - if not e.primary[heap_chunk[2]] then - e.primary[heap_chunk[2]] = { - loc = heap_chunk[3], + if not e.primary[heap_chunk.id] then + e.primary[heap_chunk.id] = { + loc = heap_chunk.loc, allocated = 0, freed = 0, count = 0, } end -- Save information about delta for memory heap. - local location_data = e.primary[heap_chunk[2]] + local location_data = e.primary[heap_chunk.id] location_data.allocated = location_data.allocated + nsize - location_data.freed = location_data.freed + heap_chunk[1] + location_data.freed = location_data.freed + heap_chunk.size location_data.count = location_data.count + 1 end end @@ -95,7 +103,7 @@ local function parse_alloc(reader, asource, events, heap, symbols) e.num = e.num + 1 e.alloc = e.alloc + nsize - heap[naddr] = {nsize, id, loc} + heap[naddr] = new_heap_chunk(nsize, id, loc) end local function parse_realloc(reader, asource, events, heap, symbols) @@ -117,7 +125,7 @@ local function parse_realloc(reader, asource, events, heap, symbols) link_to_previous(heap[oaddr], e, nsize) heap[oaddr] = nil - heap[naddr] = {nsize, id, loc} + heap[naddr] = new_heap_chunk(nsize, id, loc) end local function parse_free(reader, asource, events, heap, symbols) -- 2.44.0
Prior to this patch, memprof/process.lua wasn't added to the dependency list as a part of the memprof parser sources. Also, it wasn't installed into the system along with other memprof sources, which breaks the profile parser. Also, the sysprof parser sources weren't handled by the Makefile.original at all. The same applies to utils/avl.lua. This patch fixes that, so now it's possible to properly handle sysprof's parser. Part of tarantool/tarantool#5994 --- Makefile.original | 16 ++++++++++++---- tools/CMakeLists.txt | 4 ++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Makefile.original b/Makefile.original index e67b6aa8..d0834fe6 100644 --- a/Makefile.original +++ b/Makefile.original @@ -40,6 +40,7 @@ INSTALL_JITLIB= $(INSTALL_LJLIBD)/jit INSTALL_TOOLSLIB= $(INSTALL_LJLIBD) INSTALL_UTILSLIB= $(INSTALL_TOOLSLIB)/utils INSTALL_MEMPROFLIB= $(INSTALL_TOOLSLIB)/memprof +INSTALL_SYSPROFLIB= $(INSTALL_TOOLSLIB)/sysprof INSTALL_LMODD= $(INSTALL_SHARE)/lua INSTALL_LMOD= $(INSTALL_LMODD)/$(ABIVER) INSTALL_CMODD= $(INSTALL_LIB)/lua @@ -68,10 +69,12 @@ INSTALL_PC= $(INSTALL_PKGCONFIG)/$(INSTALL_PCNAME) INSTALL_DIRS= $(INSTALL_BIN) $(INSTALL_LIB) $(INSTALL_INC) $(INSTALL_MAN) \ $(INSTALL_PKGCONFIG) $(INSTALL_JITLIB) $(INSTALL_LMOD) $(INSTALL_CMOD) \ - $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_TOOLSLIB) + $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_SYSPROFLIB) \ + $(INSTALL_TOOLSLIB) UNINSTALL_DIRS= $(INSTALL_JITLIB) $(INSTALL_LJLIBD) $(INSTALL_INC) \ $(INSTALL_LMOD) $(INSTALL_LMODD) $(INSTALL_CMOD) $(INSTALL_CMODD) \ - $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_TOOLSLIB) + $(INSTALL_UTILSLIB) $(INSTALL_MEMPROFLIB) $(INSTALL_SYSPROFLIB) \ + $(INSTALL_TOOLSLIB) RM= rm -f MKDIR= mkdir -p @@ -95,8 +98,9 @@ FILES_JITLIB= bc.lua bcsave.lua dump.lua p.lua v.lua zone.lua \ dis_arm64be.lua dis_ppc.lua dis_mips.lua dis_mipsel.lua \ dis_mips64.lua dis_mips64el.lua vmdef.lua FILES_UTILSLIB= avl.lua bufread.lua symtab.lua -FILES_MEMPROFLIB= parse.lua humanize.lua -FILES_TOOLSLIB= memprof.lua +FILES_MEMPROFLIB= humanize.lua parse.lua process.lua +FILES_SYSPROFLIB= parse.lua +FILES_TOOLSLIB= memprof.lua sysprof.lua ifeq (,$(findstring Windows,$(OS))) HOST_SYS:= $(shell uname -s) @@ -140,6 +144,7 @@ install: $(INSTALL_DEP) cd src/jit && $(INSTALL_F) $(FILES_JITLIB) $(INSTALL_JITLIB) cd tools/utils && $(INSTALL_F) $(FILES_UTILSLIB) $(INSTALL_UTILSLIB) cd tools/memprof && $(INSTALL_F) $(FILES_MEMPROFLIB) $(INSTALL_MEMPROFLIB) + cd tools/sysprof && $(INSTALL_F) $(FILES_SYSPROFLIB) $(INSTALL_SYSPROFLIB) cd tools && $(INSTALL_F) $(FILES_TOOLSLIB) $(INSTALL_TOOLSLIB) @echo "==== Successfully installed LuaJIT $(VERSION) to $(PREFIX) ====" @echo "" @@ -162,6 +167,9 @@ uninstall: for file in $(FILES_MEMPROFLIB); do \ $(UNINSTALL) $(INSTALL_MEMPROFLIB)/$$file; \ done + for file in $(FILES_SYSPROFLIB); do \ + $(UNINSTALL) $(INSTALL_SYSPROFLIB)/$$file; \ + done for file in $(FILES_TOOLSLIB); do \ $(UNINSTALL) $(INSTALL_TOOLSLIB)/$$file; \ done diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index 1dfc39e4..a4adc15b 100644 --- a/tools/CMakeLists.txt +++ b/tools/CMakeLists.txt @@ -14,6 +14,7 @@ else() add_custom_target(tools-parse-memprof EXCLUDE_FROM_ALL DEPENDS memprof/humanize.lua memprof/parse.lua + memprof/process.lua memprof.lua utils/avl.lua utils/bufread.lua @@ -24,6 +25,7 @@ else() install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/memprof/humanize.lua ${CMAKE_CURRENT_SOURCE_DIR}/memprof/parse.lua + ${CMAKE_CURRENT_SOURCE_DIR}/memprof/process.lua DESTINATION ${LUAJIT_DATAROOTDIR}/memprof PERMISSIONS OWNER_READ OWNER_WRITE @@ -61,6 +63,7 @@ else() add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS sysprof/parse.lua sysprof.lua + utils/avl.lua utils/bufread.lua utils/symtab.lua ) @@ -76,6 +79,7 @@ else() COMPONENT tools-parse-sysprof ) install(FILES + ${CMAKE_CURRENT_SOURCE_DIR}/utils/avl.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/bufread.lua ${CMAKE_CURRENT_SOURCE_DIR}/utils/symtab.lua DESTINATION ${LUAJIT_DATAROOTDIR}/utils -- 2.44.0
This module became unused as a result of commit tarantool/tarantool@e2851883cd4c23d41bd9aec23fad06fd10d39c45, so it can be purged safely from the LuaJIT sources. Part of tarantool/tarantool#8700 --- tools/CMakeLists.txt | 6 ------ tools/sysprof/collapse.lua | 3 --- 2 files changed, 9 deletions(-) delete mode 100755 tools/sysprof/collapse.lua diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt index b951f767..1dfc39e4 100644 --- a/tools/CMakeLists.txt +++ b/tools/CMakeLists.txt @@ -60,9 +60,6 @@ if(LUAJIT_DISABLE_SYSPROF) else() add_custom_target(tools-parse-sysprof EXCLUDE_FROM_ALL DEPENDS sysprof/parse.lua - # TODO: This line is not deleted only for the sake of integrational - # testing. It should be deleted in the next series. - sysprof/collapse.lua sysprof.lua utils/bufread.lua utils/symtab.lua @@ -71,9 +68,6 @@ else() install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/parse.lua - # TODO: This line is not deleted only for the sake of integrational - # testing. It should be deleted in the next series. - ${CMAKE_CURRENT_SOURCE_DIR}/sysprof/collapse.lua DESTINATION ${LUAJIT_DATAROOTDIR}/sysprof PERMISSIONS OWNER_READ OWNER_WRITE diff --git a/tools/sysprof/collapse.lua b/tools/sysprof/collapse.lua deleted file mode 100755 index cac92f1a..00000000 --- a/tools/sysprof/collapse.lua +++ /dev/null @@ -1,3 +0,0 @@ --- FIXME: This file is not deleted only for the sake of --- integrational testing. It should be deleted in the --- next series. -- 2.44.0
Changes in v3: - Last patch is split into two parts: second part will go in another series. Branch: https://github.com/tarantool/luajit/tree/fckxorg/profile-parsers-refactoring-p1 PR: https://github.com/tarantool/tarantool/pull/9438 Issues: https://github.com/tarantool/tarantool/issues/5994 https://github.com/tarantool/tarantool/issues/9217 Maxim Kokryashkin (6): build: purge sysprof.collapse module build: fix tool components handling memprof: refactor `heap_chunk` data structure memprof: remove unused arguments memprof: introduce the `--human-readable` option profilers: introduce event reader module Makefile.original | 18 +++-- .../gh-5994-memprof-human-readable.test.lua | 73 +++++++++++++++++++ tools/CMakeLists.txt | 14 ++-- tools/memprof.lua | 22 ++++-- tools/memprof/humanize.lua | 46 +++++++++--- tools/memprof/parse.lua | 22 ++++-- tools/sysprof/collapse.lua | 3 - tools/utils/evread.lua | 32 ++++++++ 8 files changed, 192 insertions(+), 38 deletions(-) create mode 100644 test/tarantool-tests/gh-5994-memprof-human-readable.test.lua delete mode 100755 tools/sysprof/collapse.lua create mode 100644 tools/utils/evread.lua -- 2.44.0
Sergey,
thanks for the fixes! LGTM
On 3/13/24 15:35, Sergey Kaplun wrote:
> Sergey,
> Thanks for the review!
> Fixed your comment and force-pushed the branch.
>
> On 13.03.24, Sergey Bronnikov wrote:
>> Sergey,
>>
>> LGTM with minor comment below.
>>
>> On 3/13/24 12:37, Sergey Kaplun wrote:
> <snipped>
>
>>> ===================================================================
>>> diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
>>> index 91e2c603..468462d2 100644
>>> --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
>>> +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
>>> @@ -44,9 +44,11 @@ local LJ_MAX_JSLOTS = 250
>>> -- `maxslot` (the first free slot) to 249. Hence, the JIT slots
>>> -- are overflowing.
>>>
>>> -local chunk = 'local uv = {key = 1}\n'
>>> -chunk = chunk .. 'return function()\n'
>>> -chunk = chunk .. 'local r = retf()\n'
>>> +local chunk = [[
>>> +local uv = {key = 1}
>>> +return function()
>>> + local r = retf()
>>> + ]]
>> here brackets are on a new line with indentation and below brackets on
>> the same line with code.
>>
>> looks inconsistently.
> I've used such indentation to create a human-readable chunk:
> | return function()
> | local r = retf()
> | uv.key, ..., uv_key = nil
> | end
> Instead of:
> | return function()
> | local r = retf()
> | uv.key, ..., uv_key = nil
> | end
>
> Since nobody wants to read this chunk as is, I've removed 2 spaces to
> avoid confusion. See the iterative patch below. Branch is force-pushed.
>
> ===================================================================
> diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> index b454003e..cfaecbce 100644
> --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua
> @@ -51,7 +51,7 @@ local chunk = [[
> local uv = {key = 1}
> return function()
> local r = retf()
> - ]]
> +]]
>
> -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount.
> -- 1 slot is reserved (`r` variable), 1 pair is set outside the
> ===================================================================
>
>>>
>>> -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount.
>>> -- 1 slot is reserved (`r` variable), 1 pair is set outside the
>>> @@ -56,8 +58,8 @@ chunk = chunk .. 'local r = retf()\n'
>>> for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do
>>> chunk = chunk .. ('uv.key, ')
>>> end
>>> -chunk = chunk .. 'uv.key = nil\n'
>>> -chunk = chunk .. 'end\n'
>>> +chunk = chunk .. [[uv.key = nil
>>> +end]]
>>>
>>> local get_func = assert(loadstring(chunk))
>>> local function_max_framesize = get_func()
>>> ===================================================================
>>>
> <snipped>
>
Sergey, Thanks for the review! Fixed your comment and force-pushed the branch. On 13.03.24, Sergey Bronnikov wrote: > Sergey, > > LGTM with minor comment below. > > On 3/13/24 12:37, Sergey Kaplun wrote: <snipped> > > =================================================================== > > diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > > index 91e2c603..468462d2 100644 > > --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > > +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > > @@ -44,9 +44,11 @@ local LJ_MAX_JSLOTS = 250 > > -- `maxslot` (the first free slot) to 249. Hence, the JIT slots > > -- are overflowing. > > > > -local chunk = 'local uv = {key = 1}\n' > > -chunk = chunk .. 'return function()\n' > > -chunk = chunk .. 'local r = retf()\n' > > +local chunk = [[ > > +local uv = {key = 1} > > +return function() > > + local r = retf() > > + ]] > > here brackets are on a new line with indentation and below brackets on > the same line with code. > > looks inconsistently. I've used such indentation to create a human-readable chunk: | return function() | local r = retf() | uv.key, ..., uv_key = nil | end Instead of: | return function() | local r = retf() | uv.key, ..., uv_key = nil | end Since nobody wants to read this chunk as is, I've removed 2 spaces to avoid confusion. See the iterative patch below. Branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua index b454003e..cfaecbce 100644 --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua @@ -51,7 +51,7 @@ local chunk = [[ local uv = {key = 1} return function() local r = retf() - ]] +]] -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. -- 1 slot is reserved (`r` variable), 1 pair is set outside the =================================================================== > > > > > -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. > > -- 1 slot is reserved (`r` variable), 1 pair is set outside the > > @@ -56,8 +58,8 @@ chunk = chunk .. 'local r = retf()\n' > > for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do > > chunk = chunk .. ('uv.key, ') > > end > > -chunk = chunk .. 'uv.key = nil\n' > > -chunk = chunk .. 'end\n' > > +chunk = chunk .. [[uv.key = nil > > +end]] > > > > local get_func = assert(loadstring(chunk)) > > local function_max_framesize = get_func() > > =================================================================== > > <snipped> -- Best regards, Sergey Kaplun
Sergey, LGTM with minor comment below. On 3/13/24 12:37, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the review! > Fixed your comments and force-pushed the branch. > > On 12.03.24, Sergey Bronnikov wrote: >> Hi, Sergey >> >> >> thanks for the patch! >> >> On 3/12/24 08:26, Sergey Kaplun wrote: >>> From: Mike Pall <mike> >>> >>> Thanks to Sergey Kaplun. >>> >>> (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa) >>> >>> When compiling a stitched (or side) trace, there is no check for the >>> frame size of the current prototype during recording. Hence, when we >>> return (for example, after stitching) to the lower frame with a maximum >>> possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249) >>> slot for GC64 mode may be used. This leads to the corresponding assertion >>> failure in `rec_check_slots()`. >>> >>> This patch adds the corresponding check. >>> >>> Sergey Kaplun: >>> * added the description and the test for the problem >>> >>> Part of tarantool/tarantool#9595 > Updated commit message to the following, see the comment below. > > | Check frame size limit before returning to a lower frame. > | > | Thanks to Sergey Kaplun. > | > | (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa) > | > | When compiling a stitched (or side) trace, there is no check for the > | frame size of the current prototype during recording. Hence, when we > | return (for example, after stitching) to the lower frame with a maximum > | possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249) > | slot for GC64 mode may be used. This leads to the corresponding assertion > | failure in `rec_check_slots()`. > | > | This patch adds the corresponding check. The `LJ_MAX_JSLOTS` and > | `LJ_MAX_SLOTS` are equal by default, but their values may be manually > | changed for some custom builds. Hence, the check is not enabled only for > | `LJ_GC64` mode. > | > | Sergey Kaplun: > | * added the description and the test for the problem > | > | Part of tarantool/tarantool#9595 Thanks! >>> --- >>> >>> Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1173-frame-limit-lower-frame >>> Tarantool PR: https://github.com/tarantool/tarantool/pull/9791 >>> Related issues: >>> * https://github.com/tarantool/tarantool/issues/9595 >>> * https://github.com/LuaJIT/LuaJIT/issues/1173 >>> > <snipped> > >>> diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua >>> new file mode 100644 >>> index 00000000..91e2c603 >>> --- /dev/null >>> +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > <snipped> > >>> + >>> +local LJ_MAX_JSLOTS = 250 >> I would say in a comment that constant is from <src/lj_def.h>. >> >> Your test depends on this constant (if it will be changed the test will >> test nothing), >> >> how to make sure that LJ_MAX_JSLOTS is equal to LJ_MAX_JSLOTS in >> <src/lj_def.h>? > Honestly, I don't know any good way to do it now. This limit is > "hard-defined", but considering Mike's comment [1] may be changed by > hand by someone manually for their installation. For now, I suppose Is > should just leave a comment here. Also, by grepping, anyone who applies > the patch that changes the `LJ_MAX_JSLOTS` limit should see the > following definition and adjust it too. It is not perfect, but I don't know how to link macros in C and constant in Lua better. Thanks for the fix. > > See the iterative patch below. > > =================================================================== > diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > index 468462d2..b454003e 100644 > --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > @@ -17,6 +17,9 @@ local function retf() > end > _G.retf = retf > > +-- The maximum number of stack slots for a trace. Defined in the > +-- <src/lj_def.h>. Also, it equals `LJ_MAX_SLOTS` -- the maximum > +-- number of slots in a Lua function. > local LJ_MAX_JSLOTS = 250 > > -- Generate the following function: > =================================================================== > >>> + >>> +-- Generate the following function: > <snipped> > >>> + >>> +local chunk = 'local uv = {key = 1}\n' >>> +chunk = chunk .. 'return function()\n' >>> +chunk = chunk .. 'local r = retf()\n' >>> + >>> +-- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. >>> +-- 1 slot is reserved (`r` variable), 1 pair is set outside the >>> +-- cycle. 249 slots (the maximum available amount, see >>> +-- <src/lj_parse.c>, `bcreg_bump()` for details) are occupied in >>> +-- total. >>> +for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do >>> + chunk = chunk .. ('uv.key, ') >>> +end >>> +chunk = chunk .. 'uv.key = nil\n' >>> +chunk = chunk .. 'end\n' >> Why not to use multiline here and after the loop? > Good idea, thanks! > Fixed. See the iterative patch below. Branch is force-pushed. > > =================================================================== > diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > index 91e2c603..468462d2 100644 > --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > @@ -44,9 +44,11 @@ local LJ_MAX_JSLOTS = 250 > -- `maxslot` (the first free slot) to 249. Hence, the JIT slots > -- are overflowing. > > -local chunk = 'local uv = {key = 1}\n' > -chunk = chunk .. 'return function()\n' > -chunk = chunk .. 'local r = retf()\n' > +local chunk = [[ > +local uv = {key = 1} > +return function() > + local r = retf() > + ]] here brackets are on a new line with indentation and below brackets on the same line with code. looks inconsistently. > > -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. > -- 1 slot is reserved (`r` variable), 1 pair is set outside the > @@ -56,8 +58,8 @@ chunk = chunk .. 'local r = retf()\n' > for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do > chunk = chunk .. ('uv.key, ') > end > -chunk = chunk .. 'uv.key = nil\n' > -chunk = chunk .. 'end\n' > +chunk = chunk .. [[uv.key = nil > +end]] > > local get_func = assert(loadstring(chunk)) > local function_max_framesize = get_func() > =================================================================== > >>> +local get_func = assert(loadstring(chunk)) > <snipped> > >>> +test:done(true) > [1]: https://github.com/LuaJIT/LuaJIT/issues/1173#issuecomment-1987290608 >
Hi, Sergey! Thanks for the review! Fixed your comments and force-pushed the branch. On 12.03.24, Sergey Bronnikov wrote: > Hi, Sergey > > > thanks for the patch! > > On 3/12/24 08:26, Sergey Kaplun wrote: > > From: Mike Pall <mike> > > > > Thanks to Sergey Kaplun. > > > > (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa) > > > > When compiling a stitched (or side) trace, there is no check for the > > frame size of the current prototype during recording. Hence, when we > > return (for example, after stitching) to the lower frame with a maximum > > possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249) > > slot for GC64 mode may be used. This leads to the corresponding assertion > > failure in `rec_check_slots()`. > > > > This patch adds the corresponding check. > > > > Sergey Kaplun: > > * added the description and the test for the problem > > > > Part of tarantool/tarantool#9595 Updated commit message to the following, see the comment below. | Check frame size limit before returning to a lower frame. | | Thanks to Sergey Kaplun. | | (cherry picked from commit 302366a33853b730f1b7eb61d792abc4f84f0caa) | | When compiling a stitched (or side) trace, there is no check for the | frame size of the current prototype during recording. Hence, when we | return (for example, after stitching) to the lower frame with a maximum | possible frame size (249), the 251 = `baseslot` (2) + `maxslot` (249) | slot for GC64 mode may be used. This leads to the corresponding assertion | failure in `rec_check_slots()`. | | This patch adds the corresponding check. The `LJ_MAX_JSLOTS` and | `LJ_MAX_SLOTS` are equal by default, but their values may be manually | changed for some custom builds. Hence, the check is not enabled only for | `LJ_GC64` mode. | | Sergey Kaplun: | * added the description and the test for the problem | | Part of tarantool/tarantool#9595 > > --- > > > > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1173-frame-limit-lower-frame > > Tarantool PR: https://github.com/tarantool/tarantool/pull/9791 > > Related issues: > > * https://github.com/tarantool/tarantool/issues/9595 > > * https://github.com/LuaJIT/LuaJIT/issues/1173 > > <snipped> > > diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua > > new file mode 100644 > > index 00000000..91e2c603 > > --- /dev/null > > +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua <snipped> > > + > > +local LJ_MAX_JSLOTS = 250 > > I would say in a comment that constant is from <src/lj_def.h>. > > Your test depends on this constant (if it will be changed the test will > test nothing), > > how to make sure that LJ_MAX_JSLOTS is equal to LJ_MAX_JSLOTS in > <src/lj_def.h>? Honestly, I don't know any good way to do it now. This limit is "hard-defined", but considering Mike's comment [1] may be changed by hand by someone manually for their installation. For now, I suppose Is should just leave a comment here. Also, by grepping, anyone who applies the patch that changes the `LJ_MAX_JSLOTS` limit should see the following definition and adjust it too. See the iterative patch below. =================================================================== diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua index 468462d2..b454003e 100644 --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua @@ -17,6 +17,9 @@ local function retf() end _G.retf = retf +-- The maximum number of stack slots for a trace. Defined in the +-- <src/lj_def.h>. Also, it equals `LJ_MAX_SLOTS` -- the maximum +-- number of slots in a Lua function. local LJ_MAX_JSLOTS = 250 -- Generate the following function: =================================================================== > > > + > > +-- Generate the following function: <snipped> > > + > > +local chunk = 'local uv = {key = 1}\n' > > +chunk = chunk .. 'return function()\n' > > +chunk = chunk .. 'local r = retf()\n' > > + > > +-- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. > > +-- 1 slot is reserved (`r` variable), 1 pair is set outside the > > +-- cycle. 249 slots (the maximum available amount, see > > +-- <src/lj_parse.c>, `bcreg_bump()` for details) are occupied in > > +-- total. > > +for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do > > + chunk = chunk .. ('uv.key, ') > > +end > > +chunk = chunk .. 'uv.key = nil\n' > > +chunk = chunk .. 'end\n' > Why not to use multiline here and after the loop? Good idea, thanks! Fixed. See the iterative patch below. Branch is force-pushed. =================================================================== diff --git a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua index 91e2c603..468462d2 100644 --- a/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua +++ b/test/tarantool-tests/lj-1173-frame-limit-lower-frame.test.lua @@ -44,9 +44,11 @@ local LJ_MAX_JSLOTS = 250 -- `maxslot` (the first free slot) to 249. Hence, the JIT slots -- are overflowing. -local chunk = 'local uv = {key = 1}\n' -chunk = chunk .. 'return function()\n' -chunk = chunk .. 'local r = retf()\n' +local chunk = [[ +local uv = {key = 1} +return function() + local r = retf() + ]] -- Each `UGET` occupies 1 slot, `KNIL` occupies the same amount. -- 1 slot is reserved (`r` variable), 1 pair is set outside the @@ -56,8 +58,8 @@ chunk = chunk .. 'local r = retf()\n' for _ = 1, LJ_MAX_JSLOTS / 2 - 2 do chunk = chunk .. ('uv.key, ') end -chunk = chunk .. 'uv.key = nil\n' -chunk = chunk .. 'end\n' +chunk = chunk .. [[uv.key = nil +end]] local get_func = assert(loadstring(chunk)) local function_max_framesize = get_func() =================================================================== > > +local get_func = assert(loadstring(chunk)) <snipped> > > +test:done(true) [1]: https://github.com/LuaJIT/LuaJIT/issues/1173#issuecomment-1987290608 -- Best regards, Sergey Kaplun
Hi, Sergey! Thanks for the review! Fixed, branch is force-pushed. On Wed, Mar 13, 2024 at 10:18:02AM +0300, Sergey Kaplun wrote: > Hi, Max! > Patch LGTM, except a few nits regarding the commit message and > comments. > > Also, may you please provide two separate patches regarding backporting > to the tarantool/release/3.0 and tarantool/release/2.11 branches? > > On 11.03.24, Maksim Kokryashkin wrote: > > From: Maxim Kokryashkin <m.kokryashkin@tarantool.org> > > > > This patch makes use of reusable workflows from the > > Tarantool repository, so now LuaJIT CI tests integration > > in all relevant workflows. > > > > Also, this patch disables MacOS testing for Tarantool > > integration since those runs were made nightly in the > > Tarantool repo. > > > > As noted in the documentation [1], it is impossible for a single > > workflow call tree to include more than 20 workflows. To > > overcome this limitation, workflows are started in a bunch of > > separate files. This makes it impossible to depend on LuaJIT-only > > jobs for integration workflows such as `tarantool-ecosystem.yml` > > or `tarantool-exotic.yml` since it is impossible to make > > Nit: I suppose it should be "<integration-tarantool-ecosystem.yml> or > <integration-tarantool.yml>". > > > cross-dependencies between workflow files. Changed this part to the following: | As noted in the documentation [1], it is impossible for a single | workflow call tree to include more than 20 workflows. To overcome | this limitation, workflows are started in a bunch of separate | files. This makes it impossible to depend on LuaJIT-only jobs for | integration workflows such as <integration-tarantool.yml> or | <integration-tarantool-ecosystem.yml> since it is impossible to | make cross-dependencies between workflow files. > > > > <snipped> > > > --- > > > > Changes in v5: > > - Added secrets section for integration-tarantool-ecosystem.yml to > > resolve the issue with etcd-client integration. > > > > Branch: https://github.com/tarantool/luajit/tree/fckxorg/integration-testing > > > > .../integration-tarantool-ecosystem.yml | 41 ++++++ > > .github/workflows/integration-tarantool.yml | 118 ++++++++++++++++++ > > .github/workflows/testing.yml | 9 ++ > > 3 files changed, 168 insertions(+) > > create mode 100644 .github/workflows/integration-tarantool-ecosystem.yml > > create mode 100644 .github/workflows/integration-tarantool.yml > > > > diff --git a/.github/workflows/integration-tarantool-ecosystem.yml b/.github/workflows/integration-tarantool-ecosystem.yml > > new file mode 100644 > > index 00000000..ccafd601 > > --- /dev/null > > +++ b/.github/workflows/integration-tarantool-ecosystem.yml > > <snipped> > > > diff --git a/.github/workflows/integration-tarantool.yml b/.github/workflows/integration-tarantool.yml > > new file mode 100644 > > index 00000000..cf5c7fb5 > > --- /dev/null > > +++ b/.github/workflows/integration-tarantool.yml > > <snipped> > > > diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml > > index cb4ba57b..1581c8fc 100644 > > --- a/.github/workflows/testing.yml > > +++ b/.github/workflows/testing.yml > > @@ -1,3 +1,11 @@ > > +# XXX: A single call tree for reusable workflows can't exceed the > > +# total of 20 workflows, as stated in the documentation [1]. > > +# Some other jobs are started in separate workflow files to > > +# overcome the limitation. The jobs are Tarantool ecosystem > > +# integration testing (tarantool-ecosystem.yml) and exotic > > +# Tarantool builds (tarantool-exotic.yml). > > Nit: I suppose it should be "integration testing of Tarantool > <integration-tarantool.yml> and its ecosystem > <integration-tarantool-ecosystem.yml>". Fixed. Here is the diff: === diff --git a/.github/workflows/testing.yml b/.github/workflows/testing.yml index 1581c8fc..59761963 100644 --- a/.github/workflows/testing.yml +++ b/.github/workflows/testing.yml @@ -1,9 +1,9 @@ # XXX: A single call tree for reusable workflows can't exceed the # total of 20 workflows, as stated in the documentation [1]. # Some other jobs are started in separate workflow files to -# overcome the limitation. The jobs are Tarantool ecosystem -# integration testing (tarantool-ecosystem.yml) and exotic -# Tarantool builds (tarantool-exotic.yml). +# overcome the limitation. The jobs are integration testing of +# Tarantool <integration-tarantool.yml> and its ecosystem +# <integration-tarantool-ecosystem.yml>. # # [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations name: Testing === > > > +# > > +# [1]: https://docs.github.com/en/actions/using-workflows/reusing-workflows#limitations > > name: Testing > > > > on: > > @@ -108,6 +116,7 @@ jobs: > > <snipped> > > > > > -- > Best regards, > Sergey Kaplun
Max, thanks for review! Fixed and force-pushed. On 3/13/24 11:56, Maxim Kokryashkin wrote: > Hi, Sergey! > Thanks for the patch! > LGTM, except for a few nits regarding the commit message. > On Tue, Mar 12, 2024 at 04:51:02PM +0300, Sergey Bronnikov wrote: >> From: Sergey Bronnikov <sergeyb@tarantool.org> > Patch header contains a typo. > Typo: s/c:/ci:/ Fixed. >> Bump version of actions/checkout to v4. >> Bump fixes an annoying warning that appears in Github WebUI: > Typo: s/in/in the/ Fixed. >> Node.js 16 actions are deprecated. Please update the following actions >> to use Node.js 20: actions/checkout@v3. For more information see: >> https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. > It would be nicer to emphasize the message with | on each line: > | Node.js 16 actions are deprecated. Please update the following actions > | to use Node.js 20: actions/checkout@v3. For more information see: > | https://github.blog/changelog/2023-09-22-github-actions-transitioning-from-node-16-to-node-20/. Fixed. > >> --- >> .github/workflows/coverage.yml | 2 +- >> .github/workflows/exotic-builds-testing.yml | 2 +- >> .github/workflows/gnumake-builds-testing.yml | 2 +- >> .github/workflows/lint.yml | 2 +- >> .github/workflows/sanitizers-testing.yml | 2 +- >> .github/workflows/testing.yml | 2 +- >> 6 files changed, 6 insertions(+), 6 deletions(-) >> > <snipped>