Re: [Tarantool-patches] [PATCH 18/19] Add luacheck supression for luajit test

[Igor Munkin <imun@tarantool.org>]

Sergey,

On 21.07.20, Sergey Bronnikov wrote:
> Igor, thanks for review!
> 

<snipped>

> > By the way, I see another problem:
> > - Wait, it's all just luacheck failing on luajit tests?
> > | $ git co HEAD^
> > | $ git log --oneline -1 | cat
> > | c9653cbec Add luacheck supression for luajit test
> > | $ make luacheck || echo "Always has been"
> > | <snipped>
> > | Total: 95 warnings / 0 errors in 492 files
> > | make[3]: *** [CMakeFiles/luacheck.dir/build.make:58: luacheck] Error 1
> > | make[2]: *** [CMakeFiles/Makefile2:430: CMakeFiles/luacheck.dir/all] Error 2
> > | make[1]: *** [CMakeFiles/Makefile2:437: CMakeFiles/luacheck.dir/rule] Error 2
> > | make: *** [Makefile:316: luacheck] Error 2
> > | Always has been
> > 
> > I guess the agreement to make the series passing <luacheck> after
> > *every* patch still stands (like you made for src/ and extra/
> > directories). I doubt we should violate this rule for test/.
> 
> Actually changes in a patch series are atomic and problem described
> above is not related to atomicity of changes.

It definitely does relate to the subj.

> Paths in a luacheck config are interpreted relatively to the directory
> from which config was loaded, see [1]. When you run 'make luacheck' in
> separate build directory it uses absolute path to .luacheckrc and
> therefore absolute paths to .lua files:

One more time: I checked out the commit prior to the branch HEAD (i.e.
c9653cbec), configured Tarantool in-source build and ran <make luacheck>
available only in project root directory (otherwise the corresponding
error is reported by make util).

The root cause is not about the paths for out of source build (I didn't
even test this build type) but the exclude list, that doesn't contain
any line related to luajit test directory (i.e. test/luajit-tap). As a
result luacheck fails until the submodule is bumped, in other words
until the last commit. We already discussed this in the previous
series[1], but you ignored it. Again. Well, it's not a problem for me to
point out your attention to this. Again. This is about the 10th overall
series for luacheck, isn't it? How more iterations do we need for it?

> $ make luacheck VERBOSE=1
> ...
> Perform static analysis of Lua code
> ../.rocks/bin/luacheck --codes --config /home/s.bronnikov/tarantool/.luacheckrc /home/s.bronnikov/tarantool
> Checking /home/s.bronnikov/tarantool/build/extra/luarocks/hardcoded.lua
>          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ...
> $
> 
> in such case exclude and include paths specified in config become
> broken, luacheck simply ignores it and checks only .lua files in a build/
> directory.
> 
> I made a small change on top of branch to fix case when we run 'make
> luacheck' in a build dir:
> 
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -158,7 +158,8 @@ add_custom_target(ctags DEPENDS tags)
> 
>   add_custom_target(luacheck)
>    add_custom_command(TARGET luacheck
>    -    COMMAND ${LUACHECK} --codes --config "${PROJECT_SOURCE_DIR}/.luacheckrc" "${PROJECT_SOURCE_DIR}"
>    +    COMMAND ${LUACHECK} --codes --config .luacheckrc .
>    +    WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}"
>         COMMENT "Perform static analysis of Lua code"
> 		 )
> 
> CI passed with a patch, see [2].
> 
> 1. https://luacheck.readthedocs.io/en/stable/config.html
> 2. https://gitlab.com/tarantool/tarantool/-/pipelines/168992688

Spoiler alert: here is the missing line in .luacheckrc, that you've
removed in scope of post-review fixes for this patch[1].

================================================================================

diff --git a/.luacheckrc b/.luacheckrc
index 21680c08c..76807b716 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -50,6 +50,7 @@ exclude_files = {
     "test/vinyl/*.test.lua",
     "test/wal_off/*.test.lua",
     "test/xlog/*.test.lua",
+    "test/luajit-tap/*.test.lua",
     "third_party/**/*.lua",
     ".rocks/**/*.lua",
     ".git/**/*.lua",

================================================================================

With this line everything works fine. However I proposed to bump luajit
submodule prior to unmasking test/ directory, that looks more natural
and convenient to me. Does it look good to you?

> 

<snipped>

> 
> -- 
> sergeyb@

[1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/016332.html

-- 
Best regards,
IM