From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 36DB1445320 for ; Tue, 21 Jul 2020 14:52:38 +0300 (MSK) Date: Tue, 21 Jul 2020 14:52:36 +0300 From: Sergey Bronnikov Message-ID: <20200721115236.GB88846@pony.bronevichok.ru> References: <20200720194653.GI18920@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200720194653.GI18920@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 18/19] Add luacheck supression for luajit test List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org, alexander.turenko@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" > | > | 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 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 > > > > Part of #4681 > > > > Reviewed-by: Igor Munkin > > --- > > .luacheckrc | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/.luacheckrc b/.luacheckrc > > index 2bc2eaa33..21680c08c 100644 > > --- a/.luacheckrc > > +++ b/.luacheckrc > > > > > -- > > 2.26.2 > > > > -- > Best regards, > IM -- sergeyb@