Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI

[Timur Safin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hell, yeah! LGTM!

: -----Original Message-----
: From: Igor Munkin <imun@tarantool.org>
: Sent: Friday, February 26, 2021 1:09 AM
: To: Timur Safin <tsafin@tarantool.org>
: Cc: 'Sergey Kaplun' <skaplun@tarantool.org>; 'Alexander V. Tikhonov'
: <avtikhon@tarantool.org>; TML <tarantool-patches@dev.tarantool.org>
: Subject: Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI
: 
: Timur,
: 
: On 24.02.21, Timur Safin wrote:
: > This 1 thing is the only one that is bother me: it looks dependency of
: > Tarantool LuaJIT tests are not yet properly assigned between build
: > Targets we have.
: >
: > Let me put it how it looks like from my side:
: > - LuaJIT-test is phony, defined in third_party/luajit target and
: >   depends on tarantool-tests
: > - tarantool-tests are not phony (i.e. you could not run Tarantool-tests
: >   Twice, if you wish to)
: 
: Please, do not take it personally (at least since I've heard such
: "bothering" from several teammates), but I consider this as a disease.
: Why do you need to run the *passed* tests twice? To make *more* sure?
: Kinda Windows-way... Unfortunately, I can't create a dialog window with
: "Are you sure these tests passed?" message, so I removed the artefacts
: produced by .PHONY rules you mentioned above. Now you can run <test> and
: <luacheck> targets as much as you want.
: 
: > - third_party/luajit/test is also depending on LuaJIT-luacheck
: >   Which is not phone either, and is not runnable if you
: >   Run it outside of Tarantool build (because .luacheckrc is
: >   At the root of Tarantool repo, not LuaJIT repo).
: 
: This is not true, there is a LuaJIT specific .luacheckrc in LuaJIT repo.
: The issue you've shared with me relates to the case when <bindir> is
: located within <srcdir> -- so called "idiomatic build way". Since
: luacheck is run against all Lua files within <srcdir>, autogenerated
: jit/vmdef.lua (the single file producing luacheck warnings) is also
: checked. I explicitly excluded it from the file list to be checked. Now
: everything is fine.
: 
: > - Also LuaJIT-test is explicitly referred in the travis makefiles
: >   And not implicitly called by dependency of Tarantool testing.
: 
: LuaJIT-test is a dependency for Tarantool testing (i.e. <test> target),
: but Make is not used for running Tarantool tests in CI, so there is no
: other convenient way than using <make LuaJIT-test> in .travis.mk. I hope
: this will have been fixed in the future and it is definitely out of the
: scope of this issue.
: 
: >
: > This bothers me a lot, looks like dependency tree is not working
: > correctly at the moment. Could you please make luacheck and Tarantool-test
: > targets phony, and add LuaJIT-test as prerequisite of Tarantool
: > testing (in cmake)?
: 
: I hope the changes below are enough. Fixed in LuaJIT submodule,
: squashed, force-pushed to the branch.
: 
: ============================================================================
: ====
: 
: diff --git a/.gitignore b/.gitignore
: index 22921bf..2103a30 100644
: --- a/.gitignore
: +++ b/.gitignore
: @@ -18,7 +18,5 @@ cmake_install.cmake
:  cmake_uninstall.cmake
:  compile_commands.json
:  install_manifest.txt
: -luacheck.ok
:  luajit-parse-memprof
:  luajit.pc
: -tests.ok
: diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
: index bb0ff91..d166c9d 100644
: --- a/test/CMakeLists.txt
: +++ b/test/CMakeLists.txt
: @@ -6,33 +6,32 @@ cmake_minimum_required(VERSION 3.1 FATAL_ERROR)
:  find_program(LUACHECK luacheck)
:  if(LUACHECK)
:    set(LUACHECK_RC ${PROJECT_SOURCE_DIR}/.luacheckrc)
: -  set(LUACHECK_OK ${CMAKE_CURRENT_BINARY_DIR}/luacheck.ok)
:    file(GLOB_RECURSE LUACHECK_DEPS ${PROJECT_SOURCE_DIR}/*.lua)
: -  add_custom_command(
: +  add_custom_target(${PROJECT_NAME}-luacheck
: +    DEPENDS ${LUACHECK_RC} ${LUACHECK_DEPS}
: +  )
: +  add_custom_command(TARGET ${PROJECT_NAME}-luacheck
:      COMMENT "Running luacheck static analysis"
: -    OUTPUT ${LUACHECK_OK}
: -    DEPENDS ${LUACHECK} ${LUACHECK_RC} ${LUACHECK_DEPS}
:      COMMAND
:        ${LUACHECK} ${PROJECT_SOURCE_DIR}
:          --codes
:          --config ${LUACHECK_RC}
: -      && touch ${LUACHECK_OK}
: -      # XXX: Filenames in .luacheckrc are considered relative to
: -      # the working directory, hence luacheck should be run in the
: -      # project root directory.
: -      WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
: +        # XXX: jit/vmdef.lua is an autogenerated Lua source, so
: +        # there is no need to run luacheck against it. Hence
: +        # explicitly exclude this file from the list for check.
: +        --exclude-files ${LUAJIT_BINARY_DIR}/jit/vmdef.lua
: +    # XXX: Filenames in .luacheckrc are considered relative to
: +    # the working directory, hence luacheck should be run in the
: +    # project root directory.
: +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
:    )
:  else()
: -  add_custom_command(
: +  add_custom_target(${PROJECT_NAME}-luacheck)
: +  add_custom_command(TARGET ${PROJECT_NAME}-luacheck
:      COMMENT "`luacheck' is not found, so ${PROJECT_NAME}-luacheck target is
: dummy"
: -    OUTPUT luacheck.ok
: -    COMMAND touch luacheck.ok
: -    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
:    )
:  endif()
: 
: -add_custom_target(${PROJECT_NAME}-luacheck DEPENDS luacheck.ok)
: -
:  add_subdirectory(tarantool-tests)
: 
:  add_custom_target(${PROJECT_NAME}-test DEPENDS
: diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-
: tests/CMakeLists.txt
: index 52c3007..e9ac62a 100644
: --- a/test/tarantool-tests/CMakeLists.txt
: +++ b/test/tarantool-tests/CMakeLists.txt
: @@ -72,10 +72,11 @@ file(GLOB TEST_DEPS
: ${CMAKE_CURRENT_SOURCE_DIR}/*${LUA_TEST_SUFFIX})
: 
:  # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
:  # with dependecies are set in scope of BuildTestLib macro.
: -add_custom_command(
: -  COMMENT "Running Tarantool tests"
: -  OUTPUT tests.ok
: +add_custom_target(tarantool-tests
:    DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS} ${TEST_DEPS}
: +)
: +add_custom_command(TARGET tarantool-tests
: +  COMMENT "Running Tarantool tests"
:    COMMAND
:    env
:      LUA_PATH="${LUA_PATH}\;\;"
: @@ -85,8 +86,6 @@ add_custom_command(
:        --exec ${LUAJIT_TEST_BINARY}
:        --ext ${LUA_TEST_SUFFIX}
:        --failures --shuffle
: -    && touch tests.ok
:    WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
:  )
: 
: -add_custom_target(tarantool-tests DEPENDS tests.ok)
: 
: ============================================================================
: ====
: 
: >
: > Thanks,
: > Timur
: >
: 
: <snipped>
: 
: >
: 
: --
: Best regards,
: IM