[Tarantool-patches] [PATCH 18/19] Add luacheck supression for luajit test
Sergey Bronnikov
sergeyb at tarantool.org
Tue Jul 21 14:52:36 MSK 2020
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 at tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb at tarantool.org>
> >
> > Part of #4681
> >
> > Reviewed-by: Igor Munkin <imun at 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@
More information about the Tarantool-patches
mailing list