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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Igor, thanks for review!

On 22:46 Mon 20 Jul , Igor Munkin wrote:
> Sergey,
>
> Here we go again... I wonder how exactly you tested LuaJIT related patch
> if you ended up suppressing the same warning both inline and within
> .luacheckrc. Consider the following:
> | $ git diff | cat
> | $ make luacheck | grep -F 'luajit'
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-3196-incorrect-string-length.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4427-ffi-sandwich.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4476-fix-string-find-recording.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4773-tonumber-fail-on-NUL-char.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-494-table-chain-infinite-loop.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-505-fold-no-strref-for-ptrdiff.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-524-fold-conv-respect-src-irt.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-flush-on-trace.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/or-232-unsink-64-kptr.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/utils.lua OK
> | $ git stash pop
> | $ git diff | cat
> | diff --git a/.luacheckrc b/.luacheckrc
> | index 21680c08c..2bc2eaa33 100644
> | --- a/.luacheckrc
> | +++ b/.luacheckrc
> | @@ -171,12 +171,6 @@ files["test/long_run-py/suite.lua"] = {
> |          "delete_insert",
> |      }
> |  }
> | -files["test/luajit-tap/or-232-unsink-64-kptr.test.lua"] = {
> | -    ignore = {
> | -        -- An empty if branch.
> | -        "542",
> | -    }
> | -}
> |  files["test/replication/replica_quorum.lua"] = {
> |      globals = {
> |          "INSTANCE_URI",
> | $ make luacheck | grep -F 'luajit'
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-3196-incorrect-string-length.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4427-ffi-sandwich.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4476-fix-string-find-recording.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/gh-4773-tonumber-fail-on-NUL-char.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-494-table-chain-infinite-loop.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-505-fold-no-strref-for-ptrdiff.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-524-fold-conv-respect-src-irt.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/lj-flush-on-trace.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/or-232-unsink-64-kptr.test.lua OK
> | Checking /home/imun/projects/tarantool-review/test/luajit-tap/utils.lua OK
>
> So what does this suppression fix? Actually nothing. Please drop this
> suppression from .luacheckrc (and thereby the patch itself).

Agree, missed it, supression is useless in config and I've removed it in
a branch.

> 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.
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:
$ 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

> On 16.07.20, sergeyb@tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb@tarantool.org>
> > 
> > Part of #4681
> > 
> > Reviewed-by: Igor Munkin <imun@tarantool.org>
> > ---
> >  .luacheckrc | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/.luacheckrc b/.luacheckrc
> > index 2bc2eaa33..21680c08c 100644
> > --- a/.luacheckrc
> > +++ b/.luacheckrc
> 
> <snipped>
> 
> > -- 
> > 2.26.2
> > 
> 
> -- 
> Best regards,
> IM

-- 
sergeyb@