Tarantool development patches archive
 help / color / mirror / Atom feed
From: Igor Munkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Timur Safin <tsafin@tarantool.org>
Cc: TML <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI
Date: Fri, 26 Feb 2021 01:08:32 +0300	[thread overview]
Message-ID: <20210225220832.GC9042@tarantool.org> (raw)
In-Reply-To: <03e801d70a7c$f9162700$eb427500$@tarantool.org>

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

  reply	other threads:[~2021-02-25 22:08 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-03 23:22 [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 1/3] build: fix lua.c file generation Igor Munkin via Tarantool-patches
2021-02-15 13:12   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:17     ` Igor Munkin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 2/3] build: adjust LuaJIT build system Igor Munkin via Tarantool-patches
2021-02-15 16:13   ` Sergey Kaplun via Tarantool-patches
2021-02-19 23:10     ` Igor Munkin via Tarantool-patches
2021-02-20  7:42       ` Timur Safin via Tarantool-patches
2021-02-03 23:22 ` [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI Igor Munkin via Tarantool-patches
2021-02-15 16:29   ` Sergey Kaplun via Tarantool-patches
2021-02-19 21:29     ` Igor Munkin via Tarantool-patches
     [not found]       ` <0b6601d7075c$64329060$2c97b120$@tarantool.org>
2021-02-20 10:18         ` Igor Munkin via Tarantool-patches
2021-02-24  7:16   ` Timur Safin via Tarantool-patches
2021-02-25 22:08     ` Igor Munkin via Tarantool-patches [this message]
2021-02-26 19:04       ` Timur Safin via Tarantool-patches
2021-02-26 19:09 ` [Tarantool-patches] [PATCH 0/3] Adjust LuaJIT build system Timur Safin via Tarantool-patches
2021-02-26 21:06   ` Igor Munkin via Tarantool-patches
2021-02-27 13:56 ` Sergey Kaplun via Tarantool-patches
2021-02-28 22:05 ` Igor Munkin via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210225220832.GC9042@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imun@tarantool.org \
    --cc=tsafin@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH 3/3] ci: enable LuaJIT tests in CI' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox